Skip to content

Commit

Permalink
Lower Painless Static Memory Footprint (#45487)
Browse files Browse the repository at this point in the history 
* Painless generates a ton of duplicate strings and empty `Hashmap` instances wrapped as unmodifiable
* This change brings down the static footprint of Painless on an idle node by 20MB (after running the PMC benchmark against said node)
   * Since we were looking into ways of optimizing for smaller node sizes I think this is a worthwhile optimization
  • Loading branch information
@original-brownbear
original-brownbear authored Aug 13, 2019
1 parent 559e297 commit 4b1bf7d
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 37 deletions.
17 changes: 8 additions & 9 deletions modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessClass.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
package org.elasticsearch.painless.lookup;

import java.lang.invoke.MethodHandle;
import java.util.Collections;
import java.util.Map;
import java.util.Objects;

Expand All @@ -44,16 +43,16 @@ public final class PainlessClass {
Map<String, PainlessMethod> runtimeMethods,
Map<String, MethodHandle> getterMethodHandles, Map<String, MethodHandle> setterMethodHandles) {

this.constructors = Collections.unmodifiableMap(constructors);
this.staticMethods = Collections.unmodifiableMap(staticMethods);
this.methods = Collections.unmodifiableMap(methods);
this.staticFields = Collections.unmodifiableMap(staticFields);
this.fields = Collections.unmodifiableMap(fields);
this.constructors = Map.copyOf(constructors);
this.staticMethods = Map.copyOf(staticMethods);
this.methods = Map.copyOf(methods);
this.staticFields = Map.copyOf(staticFields);
this.fields = Map.copyOf(fields);
this.functionalInterfaceMethod = functionalInterfaceMethod;

this.getterMethodHandles = Collections.unmodifiableMap(getterMethodHandles);
this.setterMethodHandles = Collections.unmodifiableMap(setterMethodHandles);
this.runtimeMethods = Collections.unmodifiableMap(runtimeMethods);
this.getterMethodHandles = Map.copyOf(getterMethodHandles);
this.setterMethodHandles = Map.copyOf(setterMethodHandles);
this.runtimeMethods = Map.copyOf(runtimeMethods);
}

@Override
Expand Down
11 changes: 5 additions & 6 deletions modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
package org.elasticsearch.painless.lookup;

import java.lang.invoke.MethodHandle;
import java.util.Collections;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
Expand Down Expand Up @@ -59,12 +58,12 @@ public final class PainlessLookup {
Objects.requireNonNull(painlessMethodKeysToPainlessInstanceBindings);

this.javaClassNamesToClasses = javaClassNamesToClasses;
this.canonicalClassNamesToClasses = Collections.unmodifiableMap(canonicalClassNamesToClasses);
this.classesToPainlessClasses = Collections.unmodifiableMap(classesToPainlessClasses);
this.canonicalClassNamesToClasses = Map.copyOf(canonicalClassNamesToClasses);
this.classesToPainlessClasses = Map.copyOf(classesToPainlessClasses);

this.painlessMethodKeysToImportedPainlessMethods = Collections.unmodifiableMap(painlessMethodKeysToImportedPainlessMethods);
this.painlessMethodKeysToPainlessClassBindings = Collections.unmodifiableMap(painlessMethodKeysToPainlessClassBindings);
this.painlessMethodKeysToPainlessInstanceBindings = Collections.unmodifiableMap(painlessMethodKeysToPainlessInstanceBindings);
this.painlessMethodKeysToImportedPainlessMethods = Map.copyOf(painlessMethodKeysToImportedPainlessMethods);
this.painlessMethodKeysToPainlessClassBindings = Map.copyOf(painlessMethodKeysToPainlessClassBindings);
this.painlessMethodKeysToPainlessInstanceBindings = Map.copyOf(painlessMethodKeysToPainlessInstanceBindings);
}

public Class<?> javaClassNameToClass(String javaClassName) {
Expand Down
40 changes: 20 additions & 20 deletions .../lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ public void addPainlessClass(Class<?> clazz, boolean importClassName) {
Class<?> existingClass = javaClassNamesToClasses.get(clazz.getName());

if (existingClass == null) {
javaClassNamesToClasses.put(clazz.getName(), clazz);
javaClassNamesToClasses.put(clazz.getName().intern(), clazz);
} else if (existingClass != clazz) {
throw new IllegalArgumentException("class [" + canonicalClassName + "] " +
"cannot represent multiple java classes with the same name from different class loaders");
Expand All @@ -281,7 +281,7 @@ public void addPainlessClass(Class<?> clazz, boolean importClassName) {
if (existingPainlessClassBuilder == null) {
PainlessClassBuilder painlessClassBuilder = new PainlessClassBuilder();

canonicalClassNamesToClasses.put(canonicalClassName, clazz);
canonicalClassNamesToClasses.put(canonicalClassName.intern(), clazz);
classesToPainlessClassBuilders.put(clazz, painlessClassBuilder);
}

Expand All @@ -302,7 +302,7 @@ public void addPainlessClass(Class<?> clazz, boolean importClassName) {
"inconsistent no_import parameter found for class [" + canonicalClassName + "]");
}

canonicalClassNamesToClasses.put(importedCanonicalClassName, clazz);
canonicalClassNamesToClasses.put(importedCanonicalClassName.intern(), clazz);
}
} else if (importedClass != clazz) {
throw new IllegalArgumentException("imported class [" + importedCanonicalClassName + "] cannot represent multiple " +
Expand Down Expand Up @@ -394,7 +394,7 @@ public void addPainlessConstructor(Class<?> targetClass, List<Class<?>> typePara

if (existingPainlessConstructor == null) {
newPainlessConstructor = painlessConstructorCache.computeIfAbsent(newPainlessConstructor, key -> key);
painlessClassBuilder.constructors.put(painlessConstructorKey, newPainlessConstructor);
painlessClassBuilder.constructors.put(painlessConstructorKey.intern(), newPainlessConstructor);
} else if (newPainlessConstructor.equals(existingPainlessConstructor) == false){
throw new IllegalArgumentException("cannot add constructors with the same arity but are not equivalent for constructors " +
"[[" + targetCanonicalClassName + "], " + typesToCanonicalTypeNames(typeParameters) + "] and " +
Expand Down Expand Up @@ -568,9 +568,9 @@ public void addPainlessMethod(Class<?> targetClass, Class<?> augmentedClass,
newPainlessMethod = painlessMethodCache.computeIfAbsent(newPainlessMethod, key -> key);

if (isStatic) {
painlessClassBuilder.staticMethods.put(painlessMethodKey, newPainlessMethod);
painlessClassBuilder.staticMethods.put(painlessMethodKey.intern(), newPainlessMethod);
} else {
painlessClassBuilder.methods.put(painlessMethodKey, newPainlessMethod);
painlessClassBuilder.methods.put(painlessMethodKey.intern(), newPainlessMethod);
}
} else if (newPainlessMethod.equals(existingPainlessMethod) == false) {
throw new IllegalArgumentException("cannot add methods with the same name and arity but are not equivalent for methods " +
Expand Down Expand Up @@ -671,7 +671,7 @@ public void addPainlessField(Class<?> targetClass, String fieldName, Class<?> ty

if (existingPainlessField == null) {
newPainlessField = painlessFieldCache.computeIfAbsent(newPainlessField, key -> key);
painlessClassBuilder.staticFields.put(painlessFieldKey, newPainlessField);
painlessClassBuilder.staticFields.put(painlessFieldKey.intern(), newPainlessField);
} else if (newPainlessField.equals(existingPainlessField) == false) {
throw new IllegalArgumentException("cannot add fields with the same name but are not equivalent for fields " +
"[[" + targetCanonicalClassName + "], [" + fieldName + "], [" +
Expand All @@ -695,7 +695,7 @@ public void addPainlessField(Class<?> targetClass, String fieldName, Class<?> ty

if (existingPainlessField == null) {
newPainlessField = painlessFieldCache.computeIfAbsent(newPainlessField, key -> key);
painlessClassBuilder.fields.put(painlessFieldKey, newPainlessField);
painlessClassBuilder.fields.put(painlessFieldKey.intern(), newPainlessField);
} else if (newPainlessField.equals(existingPainlessField) == false) {
throw new IllegalArgumentException("cannot add fields with the same name but are not equivalent for fields " +
"[[" + targetCanonicalClassName + "], [" + fieldName + "], [" +
Expand Down Expand Up @@ -768,7 +768,7 @@ public void addImportedPainlessMethod(Class<?> targetClass, String methodName, C
Class<?> existingTargetClass = javaClassNamesToClasses.get(targetClass.getName());

if (existingTargetClass == null) {
javaClassNamesToClasses.put(targetClass.getName(), targetClass);
javaClassNamesToClasses.put(targetClass.getName().intern(), targetClass);
} else if (existingTargetClass != targetClass) {
throw new IllegalArgumentException("class [" + targetCanonicalClassName + "] " +
"cannot represent multiple java classes with the same name from different class loaders");
Expand Down Expand Up @@ -845,7 +845,7 @@ public void addImportedPainlessMethod(Class<?> targetClass, String methodName, C

if (existingImportedPainlessMethod == null) {
newImportedPainlessMethod = painlessMethodCache.computeIfAbsent(newImportedPainlessMethod, key -> key);
painlessMethodKeysToImportedPainlessMethods.put(painlessMethodKey, newImportedPainlessMethod);
painlessMethodKeysToImportedPainlessMethods.put(painlessMethodKey.intern(), newImportedPainlessMethod);
} else if (newImportedPainlessMethod.equals(existingImportedPainlessMethod) == false) {
throw new IllegalArgumentException("cannot add imported methods with the same name and arity " +
"but do not have equivalent methods " +
Expand Down Expand Up @@ -913,7 +913,7 @@ public void addPainlessClassBinding(Class<?> targetClass, String methodName, Cla
Class<?> existingTargetClass = javaClassNamesToClasses.get(targetClass.getName());

if (existingTargetClass == null) {
javaClassNamesToClasses.put(targetClass.getName(), targetClass);
javaClassNamesToClasses.put(targetClass.getName().intern(), targetClass);
} else if (existingTargetClass != targetClass) {
throw new IllegalArgumentException("class [" + targetCanonicalClassName + "] " +
"cannot represent multiple java classes with the same name from different class loaders");
Expand Down Expand Up @@ -1040,7 +1040,7 @@ public void addPainlessClassBinding(Class<?> targetClass, String methodName, Cla

if (existingPainlessClassBinding == null) {
newPainlessClassBinding = painlessClassBindingCache.computeIfAbsent(newPainlessClassBinding, key -> key);
painlessMethodKeysToPainlessClassBindings.put(painlessMethodKey, newPainlessClassBinding);
painlessMethodKeysToPainlessClassBindings.put(painlessMethodKey.intern(), newPainlessClassBinding);
} else if (newPainlessClassBinding.equals(existingPainlessClassBinding) == false) {
throw new IllegalArgumentException("cannot add class bindings with the same name and arity " +
"but do not have equivalent methods " +
Expand Down Expand Up @@ -1104,7 +1104,7 @@ public void addPainlessInstanceBinding(Object targetInstance, String methodName,
Class<?> existingTargetClass = javaClassNamesToClasses.get(targetClass.getName());

if (existingTargetClass == null) {
javaClassNamesToClasses.put(targetClass.getName(), targetClass);
javaClassNamesToClasses.put(targetClass.getName().intern(), targetClass);
} else if (existingTargetClass != targetClass) {
throw new IllegalArgumentException("class [" + targetCanonicalClassName + "] " +
"cannot represent multiple java classes with the same name from different class loaders");
Expand Down Expand Up @@ -1170,7 +1170,7 @@ public void addPainlessInstanceBinding(Object targetInstance, String methodName,

if (existingPainlessInstanceBinding == null) {
newPainlessInstanceBinding = painlessInstanceBindingCache.computeIfAbsent(newPainlessInstanceBinding, key -> key);
painlessMethodKeysToPainlessInstanceBindings.put(painlessMethodKey, newPainlessInstanceBinding);
painlessMethodKeysToPainlessInstanceBindings.put(painlessMethodKey.intern(), newPainlessInstanceBinding);
} else if (newPainlessInstanceBinding.equals(existingPainlessInstanceBinding) == false) {
throw new IllegalArgumentException("cannot add instances bindings with the same name and arity " +
"but do not have equivalent methods " +
Expand Down Expand Up @@ -1269,7 +1269,7 @@ private void copyPainlessClassMembers(Class<?> originalClass, Class<?> targetCla

if (existingPainlessMethod == null || existingPainlessMethod.targetClass != newPainlessMethod.targetClass &&
existingPainlessMethod.targetClass.isAssignableFrom(newPainlessMethod.targetClass)) {
targetPainlessClassBuilder.methods.put(painlessMethodKey, newPainlessMethod);
targetPainlessClassBuilder.methods.put(painlessMethodKey.intern(), newPainlessMethod);
}
}

Expand All @@ -1281,7 +1281,7 @@ private void copyPainlessClassMembers(Class<?> originalClass, Class<?> targetCla
if (existingPainlessField == null ||
existingPainlessField.javaField.getDeclaringClass() != newPainlessField.javaField.getDeclaringClass() &&
existingPainlessField.javaField.getDeclaringClass().isAssignableFrom(newPainlessField.javaField.getDeclaringClass())) {
targetPainlessClassBuilder.fields.put(painlessFieldKey, newPainlessField);
targetPainlessClassBuilder.fields.put(painlessFieldKey.intern(), newPainlessField);
}
}
}
Expand Down Expand Up @@ -1445,14 +1445,14 @@ public BridgeLoader run() {
MethodHandle bridgeHandle = MethodHandles.publicLookup().in(bridgeClass).unreflect(bridgeClass.getMethods()[0]);
bridgePainlessMethod = new PainlessMethod(bridgeMethod, bridgeClass,
painlessMethod.returnType, bridgeTypeParameters, bridgeHandle, bridgeMethodType);
painlessClassBuilder.runtimeMethods.put(painlessMethodKey, bridgePainlessMethod);
painlessClassBuilder.runtimeMethods.put(painlessMethodKey.intern(), bridgePainlessMethod);
painlessBridgeCache.put(painlessMethod, bridgePainlessMethod);
} catch (Exception exception) {
throw new IllegalStateException(
"internal error occurred attempting to generate a bridge method [" + bridgeClassName + "]", exception);
}
} else {
painlessClassBuilder.runtimeMethods.put(painlessMethodKey, bridgePainlessMethod);
painlessClassBuilder.runtimeMethods.put(painlessMethodKey.intern(), bridgePainlessMethod);
}
}

Expand Down Expand Up @@ -1486,8 +1486,8 @@ private void cacheRuntimeHandles(PainlessClassBuilder painlessClassBuilder) {
}

for (PainlessField painlessField : painlessClassBuilder.fields.values()) {
painlessClassBuilder.getterMethodHandles.put(painlessField.javaField.getName(), painlessField.getterMethodHandle);
painlessClassBuilder.setterMethodHandles.put(painlessField.javaField.getName(), painlessField.setterMethodHandle);
painlessClassBuilder.getterMethodHandles.put(painlessField.javaField.getName().intern(), painlessField.getterMethodHandle);
painlessClassBuilder.setterMethodHandles.put(painlessField.javaField.getName().intern(), painlessField.setterMethodHandle);
}
}
}
3 changes: 1 addition & 2 deletions modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessMethod.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodType;
import java.lang.reflect.Method;
import java.util.Collections;
import java.util.List;
import java.util.Objects;

Expand All @@ -41,7 +40,7 @@ public PainlessMethod(Method javaMethod, Class<?> targetClass, Class<?> returnTy
this.javaMethod = javaMethod;
this.targetClass = targetClass;
this.returnType = returnType;
this.typeParameters = Collections.unmodifiableList(typeParameters);
this.typeParameters = List.copyOf(typeParameters);
this.methodHandle = methodHandle;
this.methodType = methodType;
}
Expand Down

0 comments on commit 4b1bf7d

Please sign in to comment.