Skip to content

Conversation

@joegallo
Copy link
Contributor

@joegallo joegallo commented Dec 9, 2025

Related to #138991

This is a bit of work-in-progress that I've developed on the way to an actual performance improvement -- these changes are mostly new tests, and little bits of tidying, but there are a couple of microoptimizations that I wanted to get out of the way. My primary goal here is to get this little bit of work off my machine, so I can focus on the remaining bit of work that'll actually get a us the real improvement. Ideally, by splitting this out, it'll be easier to review this relatively trivial part of the work separate from the 'real' follow up pull request.


5f51e5f fixes something that popped out at me on an allocation flamegraph for Document Level Security (DLS) -- since we re-compile the mustache template per query (at least currently), the allocation of a fresh string on each compilation in order to do a contains check seems a bit silly. The calling code is here:

if (isTypeEnabled(type) == false) {
throw new IllegalArgumentException("cannot execute [" + type + "] scripts");
}

Which itself calls into:

https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/script/ScriptService.java#L662-L664

So the previous code was freshly allocating a new lowercased string on every script compilation, and then hashing that string (because it's freshly allocated, it wouldn't have a cached hashcode). So this saves the lowercasing, allocation, and hashing, since it's just be a static already-lowercased-String with a cached hashcode. It's a pretty tiny improvement, though, but it's not nothing.


97622ba and f525f70 are an optimization that shows up in the allocation profile for set and append processors, but I noticed it while looking for improvements in the date processor (funnily enough). In the case of using a mustache template to look up the timezone to use for date parsing, we read a String and then render it, but in order to do that we end up going through an unhappy CollectionUtils.ensureNoSelfReferences check that allocates a fresh Collections.newSetFromMap(new IdentityHashMap<>()). This optimizes that out by deferring the allocation until we actually need it. XContentBuilder has its own similar ensureNoSelfReferences and that already does a lazy allocation of the identity set (albeit by a different means).

@joegallo joegallo requested a review from a team as a code owner December 9, 2025 23:00
@joegallo joegallo added >non-issue :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Core/Infra Meta label for core/infra team Team:Data Management Meta label for data/management team v9.3.0 labels Dec 9, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue Team:Core/Infra Meta label for core/infra team Team:Data Management Meta label for data/management team v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants