Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripting: Scriptengine can tell if compilation is needed #29280

Closed
wants to merge 1 commit into from
Closed

Scripting: Scriptengine can tell if compilation is needed #29280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions ...s/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

import com.github.mustachejava.Mustache;
import com.github.mustachejava.MustacheFactory;

import java.io.StringReader;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.logging.log4j.util.Supplier;
Expand All @@ -34,6 +32,7 @@
import org.elasticsearch.script.TemplateScript;

import java.io.Reader;
import java.io.StringReader;
import java.io.StringWriter;
import java.security.AccessController;
import java.security.PrivilegedAction;
Expand Down Expand Up @@ -64,6 +63,11 @@ public <T> T compile(String templateName, String templateSource, ScriptContext<T
if (context.instanceClazz.equals(TemplateScript.class) == false) {
throw new IllegalArgumentException("mustache engine does not know how to handle context [" + context.name + "]");
}
// early termination if there is nothing to compile
if (containsMustacheTemplates(templateSource) == false) {
TemplateScript.Factory compiled = params -> new NoRenderMustacheExecutableScript(templateSource, params);
return context.factoryClazz.cast(compiled);
}
final MustacheFactory factory = createMustacheFactory(options);
Reader reader = new StringReader(templateSource);
Mustache template = factory.compile(reader, "query-template");
Expand All @@ -83,6 +87,22 @@ public String getType() {
return NAME;
}

@Override
public boolean requiresCompilation(String templateSource) {
return containsMustacheTemplates(templateSource);
}

/**
* A simple check if this mustache templates requires compilation. If you pass JSON in here, this condition can
* be triggered by the JSON, even though no compilation would be needed otherwise
*
* @param template the mustache template source
* @return true if it contains mustache opening and closing tags, false otherwise
*/
private boolean containsMustacheTemplates(String template) {
return template.contains("{{") && template.contains("}}");
}

/**
* Used at query execution time by script service in order to execute a query template.
* */
Expand Down Expand Up @@ -118,4 +138,24 @@ public String execute() {
return writer.toString();
}
}

/**
* a small helper class, that returns the original template instead of trying to execute any compilation or merging of
* the supplied parameters
*/
// package private for testing
class NoRenderMustacheExecutableScript extends TemplateScript {

private final String templateSource;

NoRenderMustacheExecutableScript(String templateSource, Map<String, Object> params) {
super(params);
this.templateSource = templateSource;
}

@Override
public String execute() {
return templateSource;
}
}
}
13 changes: 12 additions & 1 deletion ...g-mustache/src/test/java/org/elasticsearch/script/mustache/MustacheScriptEngineTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.github.mustachejava.MustacheFactory;

import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.script.TemplateScript;
Expand All @@ -34,6 +35,7 @@
import java.util.Map;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;

/**
* Mustache based templating test
Expand Down Expand Up @@ -75,7 +77,7 @@ public void testSimpleParameterReplace() {

public void testSimple() throws IOException {
String templateString =
"{"
"{"
+ "\"source\":{\"match_{{template}}\": {}},"
+ "\"params\":{\"template\":\"all\"}"
+ "}";
Expand All @@ -101,6 +103,15 @@ public void testParseTemplateAsSingleStringWithConditionalClause() throws IOExce
assertThat(TemplateScript.execute(), equalTo("{ \"match_all\":{} }"));
}

public void testNoCompilation() throws Exception {
String templateString = "{ \"source\": \"no mustache templates here\" }";
XContentParser parser = createParser(JsonXContent.jsonXContent, templateString);
Script script = Script.parse(parser);
TemplateScript.Factory compiled = qe.compile(null, script.getIdOrCode(), TemplateScript.CONTEXT, Collections.emptyMap());
TemplateScript templateScript = compiled.newInstance(script.getParams());
assertThat(templateScript, instanceOf(MustacheScriptEngine.NoRenderMustacheExecutableScript.class));
}

public void testEscapeJson() throws IOException {
{
StringWriter writer = new StringWriter();
Expand Down
11 changes: 11 additions & 0 deletions server/src/main/java/org/elasticsearch/script/ScriptEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,15 @@ public interface ScriptEngine extends Closeable {

@Override
default void close() throws IOException {}

/**
* Usually scripts require compilation. However there can be special cases where scripts do not require any compilation to happen.
* For example, when the input is equal to the output when rendering a string based template without parameters.
*
* @param scriptSource the script
* @return true if it makes sense to put the executed script in the cache, false otherwise
*/
default boolean requiresCompilation(String scriptSource) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this terminology and workflow is confusing, since the name would imply compile won't be called, but that is exactly what we still do.

What if instead we were to encapsulate the cache and move to an argument to compile, which each script engine may choose to use? Inserting into the cache would then increment the counter and trigger the limit check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if instead we were to encapsulate the cache and move to an argument to compile, which each script engine may choose to use? Inserting into the cache would then increment the counter and trigger the limit check.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around a little bit today and had a quick chat with Martijn, but I failed to see a good solution. Here is the culprit: We actually have to take care of two checks, one before the compilation calling checkCompilationLimit and putting things into the cache after compilation. While it would be easy to add two consumers/functions to the compile method, I dont think this is a good approach, as I really dont want to put it on every single script engine implementation to call checkCompilationLimit().

I may have missed something, so eager to find the solution I missed out.

return true;
}
}
17 changes: 11 additions & 6 deletions server/src/main/java/org/elasticsearch/script/ScriptService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.script;

import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.ResourceNotFoundException;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.admin.cluster.storedscripts.DeleteStoredScriptRequest;
Expand All @@ -46,6 +45,7 @@
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.core.internal.io.IOUtils;

import java.io.Closeable;
import java.io.IOException;
Expand Down Expand Up @@ -322,6 +322,7 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
compiledScript = cache.get(cacheKey);

if (compiledScript == null) {
boolean requiresCompilation = scriptEngine.requiresCompilation(cacheKey.idOrCode);
try {
// Either an un-cached inline script or indexed script
// If the script type is inline the name will be the same as the code for identification in exceptions
Expand All @@ -331,7 +332,9 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
logger.trace("compiling script, type: [{}], lang: [{}], options: [{}]", type, lang, options);
}
// Check whether too many compilations have happened
checkCompilationLimit();
if (requiresCompilation) {
checkCompilationLimit();
}
compiledScript = scriptEngine.compile(id, idOrCode, context, options);
} catch (ScriptException good) {
// TODO: remove this try-catch completely, when all script engines have good exceptions!
Expand All @@ -340,10 +343,12 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
throw new GeneralScriptException("Failed to compile " + type + " script [" + id + "] using lang [" + lang + "]", exception);
}

// Since the cache key is the script content itself we don't need to
// invalidate/check the cache if an indexed script changes.
scriptMetrics.onCompilation();
cache.put(cacheKey, compiledScript);
if (requiresCompilation) {
scriptMetrics.onCompilation();
// Since the cache key is the script content itself we don't need to
// invalidate/check the cache if an indexed script changes.
cache.put(cacheKey, compiledScript);
}
}

return context.factoryClazz.cast(compiledScript);
Expand Down