Caching script factory instead of script instance

Author: eyalkoren

server/src/main/java/org/elasticsearch/ingest/ConditionalProcessor.java

@@ -11,7 +11,6 @@
 
 import org.elasticsearch.common.logging.DeprecationCategory;
 import org.elasticsearch.common.logging.DeprecationLogger;
-import org.elasticsearch.script.CtxMapWrapper;
 import org.elasticsearch.script.DynamicMap;
 import org.elasticsearch.script.IngestConditionalScript;
 import org.elasticsearch.script.Script;
@@ -57,8 +56,7 @@ public class ConditionalProcessor extends AbstractProcessor implements WrappingP
     private final Processor processor;
     private final IngestMetric metric;
     private final LongSupplier relativeTimeProvider;
-    private final IngestConditionalScript precompiledConditionScript;
-    private final CtxMapWrapper ctxMapWrapper;
+    private final IngestConditionalScript.Factory precompiledConditionalScriptFactory;
 
     ConditionalProcessor(String tag, String description, Script script, ScriptService scriptService, Processor processor) {
         this(tag, description, script, scriptService, processor, System::nanoTime);
@@ -78,15 +76,13 @@ public class ConditionalProcessor extends AbstractProcessor implements WrappingP
         this.processor = processor;
         this.metric = new IngestMetric();
         this.relativeTimeProvider = relativeTimeProvider;
-        this.ctxMapWrapper = new CtxMapWrapper();
 
         try {
-            final IngestConditionalScript.Factory factory = scriptService.compile(script, IngestConditionalScript.CONTEXT);
             if (ScriptType.INLINE.equals(script.getType())) {
-                precompiledConditionScript = factory.newInstance(script.getParams(), ctxMapWrapper);
+                precompiledConditionalScriptFactory = scriptService.compile(script, IngestConditionalScript.CONTEXT);
             } else {
                 // stored script, so will have to compile at runtime
-                precompiledConditionScript = null;
+                precompiledConditionalScriptFactory = null;
             }
         } catch (ScriptException e) {
             throw newConfigurationException(TYPE, tag, null, e);
@@ -144,17 +140,14 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
     }
 
     boolean evaluate(IngestDocument ingestDocument) {
-        IngestConditionalScript script = precompiledConditionScript;
-        if (script == null) {
-            IngestConditionalScript.Factory factory = scriptService.compile(condition, IngestConditionalScript.CONTEXT);
-            script = factory.newInstance(condition.getParams(), ctxMapWrapper);
-        }
-        ctxMapWrapper.setCtxMap(new UnmodifiableIngestData(new DynamicMap(ingestDocument.getSourceAndMetadata(), FUNCTIONS)));
-        try {
-            return script.execute();
-        } finally {
-            ctxMapWrapper.clearCtxMap();
-        }
+        IngestConditionalScript.Factory factory = precompiledConditionalScriptFactory;
+        if (factory == null) {
+            factory = scriptService.compile(condition, IngestConditionalScript.CONTEXT);
+        }
+        return factory.newInstance(
+            condition.getParams(),
+            new UnmodifiableIngestData(new DynamicMap(ingestDocument.getSourceAndMetadata(), FUNCTIONS))
+        ).execute();
     }
 
     public Processor getInnerProcessor() {

server/src/main/java/org/elasticsearch/script/CtxMapWrapper.java

@@ -15,14 +15,14 @@
 
 /**
  * A script used by {@link org.elasticsearch.ingest.ConditionalProcessor}.
- * To properly expose the {@link SourceMapFieldScript#field(String)} API, make sure to provide a valid {@link CtxMap} before execution
- * through the {@link CtxMapWrapper} passed to the constructor and make sure to clear it after use to avoid leaks.
  */
 public abstract class IngestConditionalScript extends SourceMapFieldScript {
 
     public static final String[] PARAMETERS = {};
 
-    /** The context used to compile {@link IngestConditionalScript} factories. */
+    /**
+     * The context used to compile {@link IngestConditionalScript} factories.
+     * */
     public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>(
         "processor_conditional",
         Factory.class,
@@ -32,22 +32,27 @@ public abstract class IngestConditionalScript extends SourceMapFieldScript {
         true
     );
 
-    /** The generic runtime parameters for the script. */
+    /**
+     * The generic runtime parameters for the script.
+     */
     private final Map<String, Object> params;
 
-    public IngestConditionalScript(Map<String, Object> params, CtxMapWrapper ctxMapWrapper) {
-        super(ctxMapWrapper);
+    public IngestConditionalScript(Map<String, Object> params, Map<String, Object> ctxMap) {
+        super(ctxMap);
         this.params = params;
     }
 
-    /** Return the parameters for this script. */
+    /**
+     * Return the parameters for this script.
+     * @return a map of parameters
+     */
     public Map<String, Object> getParams() {
         return params;
     }
 
     public abstract boolean execute();
 
     public interface Factory {
-        IngestConditionalScript newInstance(Map<String, Object> params, CtxMapWrapper ctxMapWrapper);
+        IngestConditionalScript newInstance(Map<String, Object> params, Map<String, Object> ctxMap);
     }
 }

server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java

@@ -18,18 +18,27 @@
  * These scripts provide {@code ctx} for backwards compatibility and expose {@link Metadata}.
  */
 public abstract class SourceMapFieldScript {
-    protected final CtxMapWrapper ctxMapWrapper;
+    protected final Map<String, Object> ctxMap;
 
-    public SourceMapFieldScript(CtxMapWrapper ctxMapWrapper) {
-        this.ctxMapWrapper = ctxMapWrapper;
+    public SourceMapFieldScript(Map<String, Object> ctxMap) {
+        this.ctxMap = ctxMap;
     }
 
-    /** Provides backwards compatibility access to ctx */
+    /**
+     * Provides backwards compatibility access to ctx
+     * @return the context map containing the source data
+     */
     public Map<String, Object> getCtx() {
-        return ctxMapWrapper.getCtxMap();
+        return ctxMap;
     }
 
+    /**
+     * Expose the {@link SourceMapField field} API
+     *
+     * @param path the path to the field in the source map
+     * @return a new {@link SourceMapField} instance for the specified path
+     */
     public SourceMapField field(String path) {
-        return new SourceMapField(path, ctxMapWrapper::getCtxMap);
+        return new SourceMapField(path, () -> ctxMap);
     }
 }

server/src/main/java/org/elasticsearch/script/SourceMapFieldScript.java

@@ -169,13 +169,13 @@ public void execute() {
         } else if (context.instanceClazz.equals(AggregationScript.class)) {
             return context.factoryClazz.cast(new MockAggregationScript(script));
         } else if (context.instanceClazz.equals(IngestConditionalScript.class)) {
-            IngestConditionalScript.Factory factory = (parameters, ctxMapWrapper) -> new IngestConditionalScript(
+            IngestConditionalScript.Factory factory = (parameters, ctxMap) -> new IngestConditionalScript(
                 parameters,
-                ctxMapWrapper
+                ctxMap
             ) {
                 @Override
                 public boolean execute() {
-                    return (boolean) script.apply(ctxMapWrapper.getCtxMap());
+                    return (boolean) script.apply(ctxMap);
                 }
             };
             return context.factoryClazz.cast(factory);