Scripting: Scriptengine can tell if compilation is needed #29280
Conversation
A script engine can now tell the ScriptService if compilation of a certain script is needed. For most of the cases this will always be the case, but the mustache templating engine can check, if a compilation is needed, and if not just return the original template. This also has the side effect that if no compilation is required, caching is not required either, as it makes no sense to put the original string into a cache. The goal of this commit is to prevent hitting the compilation limit, if a user is using a lot of templated fields (for example in a watch), but is not putting any template in there that requires compilation.
spinscale
added
>enhancement
:Core/Infra/Scripting
hub-cap
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
| * @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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
👍
There was a problem hiding this comment.
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.
|
I will be closing this for now and revisit later. This is a not a clean solution to the problem. For now the increasing of compilation limit and either increasing the script cache or fully disabling it to prevent churn is the way to go. |
A script engine can now tell the ScriptService if compilation of a
certain script is needed. For most of the cases this will always be the
case, but the mustache templating engine can check, if a compilation is
needed, and if not just return the original template.
This also has the side effect that if no compilation is required,
caching is not required either, as it makes no sense to put the original
string into a cache.
The goal of this is to prevent hitting the compilation limit, if
a user is using a lot of templated fields (for example in a watch), but
is not putting any template in there that requires compilation.
Note: This is still missing some tests in the ScriptService, but as the code changes are so far small, we can take this as a first discussion and then go further from here, if the general approach makes sense.