Speed up Script Engine initialization

[timlueg]

Description

Small improvements to Painless Script Engine initialization to reduce OpenSearch startup time.

• Compile the whitespace pattern only once
• Reuse PainlessLookup instances for identical allowlist sequences
• Use sets to avoid repeated linear scans during lookup validation

Version	Time to Node "started" using ./gradlew run (best of 5)
Before	2012 ms
After	1815 ms

Improvement: ~197 ms, about 9.8%.

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 340db17)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Compile whitespace Pattern once and simplify blank-line check in AllowlistLoader Relevant files: modules/lang-painless/spi/src/main/java/org/opensearch/painless/spi/AllowlistLoader.java Sub-PR theme: Reuse PainlessLookup instances and use sets for lookup validation Relevant files: modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java modules/lang-painless/src/main/java/org/opensearch/painless/lookup/PainlessLookupBuilder.java

⚡ Recommended focus areas for review Cache Key Equality The deduplication of PainlessLookup instances relies on List.equals() for allowlistsToLookups map key comparison. This assumes that Allowlist objects implement meaningful equals() and hashCode() methods. If Allowlist uses default identity-based equality (from Object), two structurally identical allowlist lists will never match, making the cache ineffective. This should be verified before relying on this optimization. List<Allowlist> allowlists = List.copyOf(entry.getValue()); PainlessLookup lookup = allowlistsToLookups.computeIfAbsent(allowlists, PainlessLookupBuilder::buildFromAllowlists); Equality Check Change The original code checked canonicalClassNamesToClasses.values().containsAll(classesToPainlessClasses.keySet()) || classesToPainlessClasses.keySet().containsAll(canonicalClassNamesToClasses.values()) (both directions), which is equivalent to set equality. The new code uses canonicalClasses.equals(painlessClasses) where canonicalClasses is a HashSet and painlessClasses is the keySet() of a HashMap. This should be semantically equivalent, but it's worth verifying that the Set.equals() contract is correctly satisfied here, especially since painlessClasses is a live keySet() view. if (canonicalClasses.equals(painlessClasses) == false) { throw new IllegalArgumentException( "the values of canonical class names to classes " + "must have the same classes as the keys of classes to painless classes" );

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 340db17

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Cache key equality depends on element equals() Using List.copyOf as a map key relies on List.equals() for deduplication, which performs element-by-element equality. This means two different List instances with the same Allowlist objects (by reference) will be considered equal only if Allowlist properly implements equals(). If Allowlist does not override equals(), the cache will never hit and the optimization will have no effect. Verify that Allowlist implements equals() and hashCode(), or use an identity-based key strategy. modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java [97-98] +// Ensure Allowlist implements equals()/hashCode() for this cache to work correctly. +// If not, consider using IdentityHashMap or a wrapper with identity-based equality. List<Allowlist> allowlists = List.copyOf(entry.getValue()); PainlessLookup lookup = allowlistsToLookups.computeIfAbsent(allowlists, PainlessLookupBuilder::buildFromAllowlists); Suggestion importance[1-10]: 6 __ Why: This is a valid concern - if Allowlist doesn't override equals()/hashCode(), the cache optimization will never work. However, the 'improved_code' only adds a comment without actually fixing the issue, making it more of a verification suggestion than a concrete fix.	Low
General	Avoid using live map key set view classesToPainlessClasses.keySet() returns a live view of the map's keys. While it is only used for read-only containsAll/equals checks here, assigning it directly as painlessClasses could be misleading and fragile if the code is later modified. Consider wrapping it in a new HashSet<>() for consistency and safety, similar to the other two sets. modules/lang-painless/src/main/java/org/opensearch/painless/lookup/PainlessLookupBuilder.java [1930-1932] Set<Class<?>> javaClasses = new HashSet<>(javaClassNamesToClasses.values()); Set<Class<?>> canonicalClasses = new HashSet<>(canonicalClassNamesToClasses.values()); -Set<Class<?>> painlessClasses = classesToPainlessClasses.keySet(); +Set<Class<?>> painlessClasses = new HashSet<>(classesToPainlessClasses.keySet()); Suggestion importance[1-10]: 3 __ Why: While wrapping classesToPainlessClasses.keySet() in a new HashSet<>() adds consistency, the live view is only used for read-only checks in this method, so the practical risk is minimal. This is a minor style/safety improvement.	Low

---

Previous suggestions

Suggestions up to commit c651fa8

Category	Suggestion	Impact
General	Simplify cache lookup with computeIfAbsent The cache key uses List.copyOf which relies on List.equals() for map lookup. This works correctly for equality-based deduplication, but List equality depends on Allowlist.equals() being properly implemented. If Allowlist does not override equals()/hashCode(), two logically identical lists will not match in the HashMap, defeating the caching purpose. Consider using computeIfAbsent with a proper key strategy, or verify that Allowlist implements equals/hashCode. modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java [97-102] List<Allowlist> allowlists = List.copyOf(entry.getValue()); -PainlessLookup lookup = allowlistsToLookups.get(allowlists); -if (lookup == null) { - lookup = PainlessLookupBuilder.buildFromAllowlists(allowlists); - allowlistsToLookups.put(allowlists, lookup); -} +PainlessLookup lookup = allowlistsToLookups.computeIfAbsent( + allowlists, + PainlessLookupBuilder::buildFromAllowlists +); Suggestion importance[1-10]: 4 __ Why: The computeIfAbsent refactoring is a minor style improvement that doesn't change behavior. The more important concern raised (about Allowlist.equals()/hashCode()) is valid but speculative without knowing the Allowlist implementation, and the improved_code only addresses the style change, not the underlying concern.	Low
	Ensure consistent set type for all comparisons painlessClasses is assigned directly as classesToPainlessClasses.keySet(), which is a live view of the map. While it is only used for read-only containsAll/equals checks here, wrapping it in a HashSet (like the other two sets) would make the intent consistent and avoid any accidental mutation side effects if the code evolves. More importantly, HashSet.containsAll and equals on a HashSet are O(n) whereas Collection.containsAll on a map's values() collection can be O(n²); keeping painlessClasses as a HashSet ensures consistent O(n) performance. modules/lang-painless/src/main/java/org/opensearch/painless/lookup/PainlessLookupBuilder.java [1930-1932] Set<Class<?>> javaClasses = new HashSet<>(javaClassNamesToClasses.values()); Set<Class<?>> canonicalClasses = new HashSet<>(canonicalClassNamesToClasses.values()); -Set<Class<?>> painlessClasses = classesToPainlessClasses.keySet(); +Set<Class<?>> painlessClasses = new HashSet<>(classesToPainlessClasses.keySet()); Suggestion importance[1-10]: 3 __ Why: classesToPainlessClasses.keySet() already returns a Set backed by a HashMap, so containsAll and equals operations are already O(n). Wrapping it in a new HashSet is a minor consistency improvement with negligible practical impact.	Low

Suggestions up to commit 08f465b

Category	Suggestion	Impact
Possible issue	Cache effectiveness depends on correct equals/hashCode The cache key relies on List.equals(), which compares element-by-element using Allowlist.equals(). If Allowlist does not override equals() and hashCode(), two logically identical lists will never match in the HashMap, making the cache ineffective. Verify that Allowlist properly overrides equals() and hashCode(), or use a different cache key strategy (e.g., based on identity or a canonical key derived from allowlist contents). modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java [97-102] +// Ensure Allowlist overrides equals() and hashCode() for this cache to work correctly. +// If not, consider using IdentityHashMap or a different keying strategy. List<Allowlist> allowlists = List.copyOf(entry.getValue()); PainlessLookup lookup = allowlistsToLookups.get(allowlists); if (lookup == null) { lookup = PainlessLookupBuilder.buildFromAllowlists(allowlists); allowlistsToLookups.put(allowlists, lookup); } Suggestion importance[1-10]: 5 __ Why: The concern about Allowlist.equals() and hashCode() is valid for cache correctness, but the suggestion's improved_code only adds a comment without actually fixing the potential issue. This is more of a verification request than an actionable fix.	Low
General	Verify Java version compatibility for isBlank The original code checked line.replaceAll("\s+", "").equals(""), which is equivalent to checking if the string contains only whitespace characters (including tabs, newlines, etc.). String.isBlank() is semantically equivalent and is a correct replacement here, but note that isBlank() was introduced in Java 11. Ensure the project's minimum Java version supports this method to avoid runtime issues. modules/lang-painless/spi/src/main/java/org/opensearch/painless/spi/AllowlistLoader.java [529] +if (line.isBlank()) { - Suggestion importance[1-10]: 2 __ Why: The existing_code and improved_code are identical, making this purely a verification suggestion. OpenSearch already requires Java 11+, so isBlank() compatibility is not a real concern here.	Low

[github-actions]

Persistent review updated to latest commit c651fa8

[github-actions]

✅ Gradle check result for c651fa8: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.49%. Comparing base (fead3a9) to head (340db17).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...nsearch/painless/lookup/PainlessLookupBuilder.java	50.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21463      +/-   ##
============================================
+ Coverage     73.34%   73.49%   +0.15%     
- Complexity    74353    74481     +128     
============================================
  Files          5967     5967              
  Lines        338227   338231       +4     
  Branches      48754    48753       -1     
============================================
+ Hits         248061   248586     +525     
+ Misses        70399    69825     -574     
- Partials      19767    19820      +53     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cwperks]

@timlueg this is great! Thank you for looking into this. We've run into issues with bootstrap time on the CI checks of the security repo and this looks like it will help.

[github-actions]

Persistent review updated to latest commit 340db17

[timlueg]

@cwperks then the CI might like this PR even more :) but it's a bit less straightforward #21473

[github-actions]

❌ Gradle check result for 340db17: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 340db17: SUCCESS