From e9e1095596ace4cd50c5a09a6860506add1eb216 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Fri, 27 Jul 2018 14:23:37 -0700 Subject: [PATCH 1/4] Painless: Add method type to method. (#32441) MethodType can be computed at compile-time rather than run-time. This removes the method that collects MethodType at run-time from a PainlessMethod since is it no longer necessary. --- .../elasticsearch/painless/FunctionRef.java | 6 +-- .../lookup/PainlessLookupBuilder.java | 13 +++-- .../painless/lookup/PainlessMethod.java | 54 ++----------------- .../painless/node/SFunction.java | 4 +- 4 files changed, 20 insertions(+), 57 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java index aa72724b930..7b7e8d25f39 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java @@ -93,12 +93,12 @@ public class FunctionRef { * @param numCaptures number of captured arguments */ public FunctionRef(Class expected, PainlessMethod interfaceMethod, PainlessMethod delegateMethod, int numCaptures) { - MethodType delegateMethodType = delegateMethod.getMethodType(); + MethodType delegateMethodType = delegateMethod.methodType; interfaceMethodName = interfaceMethod.name; factoryMethodType = MethodType.methodType(expected, delegateMethodType.dropParameterTypes(numCaptures, delegateMethodType.parameterCount())); - interfaceMethodType = interfaceMethod.getMethodType().dropParameterTypes(0, 1); + interfaceMethodType = interfaceMethod.methodType.dropParameterTypes(0, 1); // the Painless$Script class can be inferred if owner is null if (delegateMethod.target == null) { @@ -142,7 +142,7 @@ public class FunctionRef { interfaceMethodName = interfaceMethod.name; factoryMethodType = MethodType.methodType(expected, delegateMethodType.dropParameterTypes(numCaptures, delegateMethodType.parameterCount())); - interfaceMethodType = interfaceMethod.getMethodType().dropParameterTypes(0, 1); + interfaceMethodType = interfaceMethod.methodType.dropParameterTypes(0, 1); delegateClassName = CLASS_NAME; delegateInvokeType = H_INVOKESTATIC; diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java index 3675cc7cd0f..ba48fbea734 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java @@ -27,6 +27,7 @@ import org.elasticsearch.painless.spi.WhitelistMethod; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; @@ -356,10 +357,12 @@ public class PainlessLookupBuilder { "[[" + targetCanonicalClassName + "], " + typesToCanonicalTypeNames(typeParameters) + "] not found", iae); } + MethodType methodType = methodHandle.type(); + painlessConstructor = painlessMethodCache.computeIfAbsent( new PainlessMethodCacheKey(targetClass, CONSTRUCTOR_NAME, typeParameters), key -> new PainlessMethod(CONSTRUCTOR_NAME, targetClass, null, void.class, typeParameters, - asmConstructor, javaConstructor.getModifiers(), methodHandle) + asmConstructor, javaConstructor.getModifiers(), methodHandle, methodType) ); painlessClassBuilder.constructors.put(painlessMethodKey, painlessConstructor); @@ -516,10 +519,12 @@ public class PainlessLookupBuilder { "[" + methodName + "], " + typesToCanonicalTypeNames(typeParameters) + "] not found", iae); } + MethodType methodType = methodHandle.type(); + painlessMethod = painlessMethodCache.computeIfAbsent( new PainlessMethodCacheKey(targetClass, methodName, typeParameters), key -> new PainlessMethod(methodName, targetClass, null, returnType, - typeParameters, asmMethod, javaMethod.getModifiers(), methodHandle)); + typeParameters, asmMethod, javaMethod.getModifiers(), methodHandle, methodType)); painlessClassBuilder.staticMethods.put(painlessMethodKey, painlessMethod); } else if ((painlessMethod.name.equals(methodName) && painlessMethod.rtn == returnType && @@ -557,10 +562,12 @@ public class PainlessLookupBuilder { } } + MethodType methodType = methodHandle.type(); + painlessMethod = painlessMethodCache.computeIfAbsent( new PainlessMethodCacheKey(targetClass, methodName, typeParameters), key -> new PainlessMethod(methodName, targetClass, augmentedClass, returnType, - typeParameters, asmMethod, javaMethod.getModifiers(), methodHandle)); + typeParameters, asmMethod, javaMethod.getModifiers(), methodHandle, methodType)); painlessClassBuilder.methods.put(painlessMethodKey, painlessMethod); } else if ((painlessMethod.name.equals(methodName) && painlessMethod.rtn == returnType && diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessMethod.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessMethod.java index 3321de94a26..904cf06907f 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessMethod.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessMethod.java @@ -38,9 +38,10 @@ public class PainlessMethod { public final org.objectweb.asm.commons.Method method; public final int modifiers; public final MethodHandle handle; + public final MethodType methodType; public PainlessMethod(String name, Class target, Class augmentation, Class rtn, List> arguments, - org.objectweb.asm.commons.Method method, int modifiers, MethodHandle handle) { + org.objectweb.asm.commons.Method method, int modifiers, MethodHandle handle, MethodType methodType) { this.name = name; this.augmentation = augmentation; this.target = target; @@ -49,54 +50,7 @@ public class PainlessMethod { this.method = method; this.modifiers = modifiers; this.handle = handle; - } - - /** - * Returns MethodType for this method. - *

- * This works even for user-defined Methods (where the MethodHandle is null). - */ - public MethodType getMethodType() { - // we have a methodhandle already (e.g. whitelisted class) - // just return its type - if (handle != null) { - return handle.type(); - } - // otherwise compute it - final Class params[]; - final Class returnValue; - if (augmentation != null) { - // static method disguised as virtual/interface method - params = new Class[1 + arguments.size()]; - params[0] = augmentation; - for (int i = 0; i < arguments.size(); i++) { - params[i + 1] = PainlessLookupUtility.typeToJavaType(arguments.get(i)); - } - returnValue = PainlessLookupUtility.typeToJavaType(rtn); - } else if (Modifier.isStatic(modifiers)) { - // static method: straightforward copy - params = new Class[arguments.size()]; - for (int i = 0; i < arguments.size(); i++) { - params[i] = PainlessLookupUtility.typeToJavaType(arguments.get(i)); - } - returnValue = PainlessLookupUtility.typeToJavaType(rtn); - } else if ("".equals(name)) { - // constructor: returns the owner class - params = new Class[arguments.size()]; - for (int i = 0; i < arguments.size(); i++) { - params[i] = PainlessLookupUtility.typeToJavaType(arguments.get(i)); - } - returnValue = target; - } else { - // virtual/interface method: add receiver class - params = new Class[1 + arguments.size()]; - params[0] = target; - for (int i = 0; i < arguments.size(); i++) { - params[i + 1] = PainlessLookupUtility.typeToJavaType(arguments.get(i)); - } - returnValue = PainlessLookupUtility.typeToJavaType(rtn); - } - return MethodType.methodType(returnValue, params); + this.methodType = methodType; } public void write(MethodWriter writer) { @@ -118,7 +72,7 @@ public class PainlessMethod { // method since java 8 did not check, but java 9 and 10 do if (Modifier.isInterface(clazz.getModifiers())) { writer.visitMethodInsn(Opcodes.INVOKESTATIC, - type.getInternalName(), name, getMethodType().toMethodDescriptorString(), true); + type.getInternalName(), name, methodType.toMethodDescriptorString(), true); } else { writer.invokeStatic(type, method); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFunction.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFunction.java index 7c243e296c7..600ca95504a 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFunction.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFunction.java @@ -145,9 +145,11 @@ public final class SFunction extends AStatement { } } + int modifiers = Modifier.STATIC | Modifier.PRIVATE; org.objectweb.asm.commons.Method method = new org.objectweb.asm.commons.Method(name, MethodType.methodType( PainlessLookupUtility.typeToJavaType(rtnType), paramClasses).toMethodDescriptorString()); - this.method = new PainlessMethod(name, null, null, rtnType, paramTypes, method, Modifier.STATIC | Modifier.PRIVATE, null); + MethodType methodType = MethodType.methodType(PainlessLookupUtility.typeToJavaType(rtnType), paramClasses); + this.method = new PainlessMethod(name, null, null, rtnType, paramTypes, method, modifiers, null, methodType); } @Override From 139631c77d1a6bac002f82952d599a386122b1e3 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Fri, 27 Jul 2018 20:48:17 -0400 Subject: [PATCH 2/4] AwaitsFix IndexShardTests#testDocStats Relates #32449 --- .../test/java/org/elasticsearch/index/shard/IndexShardTests.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java index 1880b6b0954..d7b1be847c0 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2376,6 +2376,7 @@ public class IndexShardTests extends IndexShardTestCase { closeShards(sourceShard, targetShard); } + @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32449") public void testDocStats() throws IOException { IndexShard indexShard = null; try { From 6e98615cc1b324f9c2f543125c4cc7a3d01f3591 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Fri, 27 Jul 2018 23:14:24 -0400 Subject: [PATCH 3/4] TEST: Avoid deletion in FlushIT Due to the recent change in LUCENE-8263, a merge can be triggered if the deletion ration is higher than 33%. An in-progress merge can prevent a synced-flush from issuing. This commit avoids deletes by using different docIds. Closes #32436 --- .../src/test/java/org/elasticsearch/indices/flush/FlushIT.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/indices/flush/FlushIT.java b/server/src/test/java/org/elasticsearch/indices/flush/FlushIT.java index bdd01e88cca..4483ce0d606 100644 --- a/server/src/test/java/org/elasticsearch/indices/flush/FlushIT.java +++ b/server/src/test/java/org/elasticsearch/indices/flush/FlushIT.java @@ -286,7 +286,6 @@ public class FlushIT extends ESIntegTestCase { assertThat(fullResult.successfulShards(), equalTo(numberOfReplicas + 1)); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32436") public void testDoNotRenewSyncedFlushWhenAllSealed() throws Exception { internalCluster().ensureAtLeastNumDataNodes(between(2, 3)); final int numberOfReplicas = internalCluster().numDataNodes() - 1; @@ -311,7 +310,7 @@ public class FlushIT extends ESIntegTestCase { // Shards were updated, renew synced flush. final int moreDocs = between(1, 10); for (int i = 0; i < moreDocs; i++) { - index("test", "doc", Integer.toString(i)); + index("test", "doc", "more-" + i); } final ShardsSyncedFlushResult thirdSeal = SyncedFlushUtil.attemptSyncedFlush(logger, internalCluster(), shardId); assertThat(thirdSeal.successfulShards(), equalTo(numberOfReplicas + 1)); From 5b1ad8099bfd0738bb16331e6f6952b8726b849e Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Sat, 28 Jul 2018 07:41:30 -0400 Subject: [PATCH 4/4] TEST: testDocStats should always use forceMerge (#32450) Due to the recent change in LUCENE-8263, we need to adjust the deletion ration to between 10% to 33% to preserve the current behavior of the test. However, we may need another refinement if soft-deletes is enabled as the actual deletes are different because of delete tombstones. This commit prefers to always execute forceMerge instead of adjusting the deletion ratio so that this test can focus on testing docStats. Closes #32449 --- .../java/org/elasticsearch/index/shard/IndexShardTests.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java index d7b1be847c0..6d9e15c52af 100644 --- a/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java @@ -2376,15 +2376,12 @@ public class IndexShardTests extends IndexShardTestCase { closeShards(sourceShard, targetShard); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32449") public void testDocStats() throws IOException { IndexShard indexShard = null; try { indexShard = newStartedShard(); final long numDocs = randomIntBetween(2, 32); // at least two documents so we have docs to delete - // Delete at least numDocs/10 documents otherwise the number of deleted docs will be below 10% - // and forceMerge will refuse to expunge deletes - final long numDocsToDelete = randomIntBetween((int) Math.ceil(Math.nextUp(numDocs / 10.0)), Math.toIntExact(numDocs)); + final long numDocsToDelete = randomLongBetween(1, numDocs); for (int i = 0; i < numDocs; i++) { final String id = Integer.toString(i); indexDoc(indexShard, "_doc", id); @@ -2434,7 +2431,6 @@ public class IndexShardTests extends IndexShardTestCase { // merge them away final ForceMergeRequest forceMergeRequest = new ForceMergeRequest(); - forceMergeRequest.onlyExpungeDeletes(randomBoolean()); forceMergeRequest.maxNumSegments(1); indexShard.forceMerge(forceMergeRequest);