davsclaus commented on code in PR #26303:
URL: https://github.com/apache/camel/pull/26303#discussion_r3989786935
##########
components/camel-groovy/src/main/java/org/apache/camel/language/groovy/GroovyLanguage.java:
##########
@@ -66,7 +80,9 @@ private GroovyLanguage(Map<String, GroovyClassService>
scriptCache, boolean load
}
public GroovyLanguage() {
- this(LRUCacheFactory.newLRUSoftCache(16, 1000, true), true);
+ // do not remove the class of an evicted script
(stopOnEviction=false): a GroovyExpression may still hold and run
+ // it. Classes are removed when the language stops or the cache is
cleared on reload.
+ this(LRUCacheFactory.newLRUSoftCache(16, 1000, false), true);
Review Comment:
Question rather than an objection: flipping `stopOnEviction` to `false`
means an evicted entry no longer gets `GroovyClassService.stop()` — i.e.
`InvokerHelper.removeClass(script)`, which drops the class's `MetaClass` from
`GroovySystem.getMetaClassRegistry()` and calls
`Introspector.flushFromCaches(clazz)`.
The reasoning in the comment is right — a `GroovyExpression` can now still
be holding and running an evicted class, so stopping it on eviction would be
wrong. I am fairly confident this is safe in practice (the metaclass registry
keys weakly on the `Class`, so the entry goes once the script class and its
`GroovyClassLoader` become unreachable), but it does move cleanup from
"explicit, at eviction" to "whenever GC gets round to it", and the
`Introspector` flush is skipped entirely.
Did you check what the retained set looks like for a long-running app that
churns distinct scripts through `GroovyLanguage.evaluate(script, bindings,
type)` — say a `camel-jbang` session or a route building expressions
dynamically — now that eviction no longer cleans up? If you have a heap
comparison it would be worth a line in the PR description; if it turns out to
matter, keying the cleanup off the expression's own lifecycle rather than the
cache would be the way out.
##########
components/camel-groovy/src/main/java/org/apache/camel/language/groovy/GroovyExpression.java:
##########
@@ -66,41 +84,227 @@ public <T> T evaluate(Exchange exchange, Class<T> type) {
@SuppressWarnings("unchecked")
protected Script instantiateScript(Exchange exchange, Map<String, Object>
globalVariables) {
- // Get the script from the cache, or create a new instance
- GroovyLanguage language = (GroovyLanguage)
exchange.getContext().resolveLanguage("groovy");
- Set<GroovyShellFactory> shellFactories =
exchange.getContext().getRegistry().findByType(GroovyShellFactory.class);
- GroovyShellFactory shellFactory = null;
+ Resolved r = resolve(exchange.getContext());
+ GroovyShellFactory shellFactory = r.shellFactory;
String fileName = null;
- if (shellFactories.size() == 1) {
- shellFactory = shellFactories.iterator().next();
+ if (shellFactory != null) {
fileName = shellFactory.getFileName(exchange);
globalVariables.putAll(shellFactory.getVariables(exchange));
}
- final String key = fileName != null ? fileName + text : text;
- Class<Script> scriptClass = language.getScriptFromCache(key);
- if (scriptClass == null) {
- // prefer to use classloader from groovy script compiler, and if
not fallback to app context
- ClassLoader cl =
exchange.getContext().getCamelContextExtension().getContextPlugin(GroovyScriptClassLoader.class);
- GroovyShell shell = shellFactory != null ?
shellFactory.createGroovyShell(exchange)
- : cl != null ? new GroovyShell(cl) : new GroovyShell();
- scriptClass = fileName != null
- ? shell.getClassLoader().parseClass(text, fileName) :
shell.getClassLoader().parseClass(text);
- language.addScriptToCache(key, scriptClass);
+
+ int generation = r.language.getGeneration();
+ CompiledScript c = compiled;
+ if (c == null || c.generation != generation || c.context != r.context
|| c.language != r.language
Review Comment:
`GroovyShellFactory.getFileName(exchange)` takes the exchange precisely so
an implementation can return a different name per message (it defaults to
`null`, so this only bites custom factories — but that is the interesting case).
When it does vary, `!Objects.equals(c.fileName, fileName)` is false on every
evaluation, so this branch is taken every time: a `getOrCompile` call, a `new
CompiledScript`, and a fresh
`MethodHandles.publicLookup().findConstructor(...)` per message. The language
cache still absorbs the compile, so nothing is *wrong*, but the per-expression
cache — the headline optimisation — never hits for exactly those users, and
they pick up the `findConstructor` lookup that `main` did not do.
Two thoughts, take either or neither:
- keep a small per-expression map keyed by `fileName` instead of a single
slot, so a factory cycling through a bounded set of names still hits; or
- keep the single slot but hold the `MethodHandle` next to the `Class` in
the language cache so a miss here does not re-do the lookup.
Mostly I would like a benchmark line for the varying-file-name case in the
description, so it is on record that it was considered rather than missed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
