[daniellansun]

https://issues.apache.org/jira/browse/GROOVY-12725

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.55072% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.4711%. Comparing base (d020012) to head (2df506b).
⚠️ Report is 69 commits behind head on master.

Files with missing lines	Patch %	Lines
...in/java/org/codehaus/groovy/vmplugin/v9/Java9.java	66.6667%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #3260        +/-   ##
==================================================
+ Coverage     74.4010%   74.4711%   +0.0701%     
- Complexity      44345      44874       +529     
==================================================
  Files            1699       1703         +4     
  Lines          150998     152063      +1065     
  Branches        28770      29144       +374     
==================================================
+ Hits           112344     113243       +899     
- Misses          28380      28409        +29     
- Partials        10274      10411       +137     

Files with missing lines	Coverage Δ
...roovy/vmplugin/internal/DefaultImportPackages.java	100.0000% <100.0000%> (ø)
...va/org/codehaus/groovy/control/ResolveVisitor.java	91.4254% <100.0000%> (+0.3194%)	⬆️
...in/java/org/codehaus/groovy/vmplugin/VMPlugin.java	12.5000% <ø> (ø)
...in/java/org/codehaus/groovy/vmplugin/v9/Java9.java	61.4286% <66.6667%> (+1.4286%)	⬆️

... and 80 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

JMH summary — classic (commit 729bae7)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group	Speedup	Calibrated	n
bench	1.275 ×	0.980 ×	134
core	1.170 ×	1.177 ×	108
grails	1.303 ×	1.245 ×	80

⚠️ 3 benchmarks at least 1.5× slower than the 90-day baseline:

• org.apache.groovy.bench.AryBench.groovy ( {"n":"1000000"} ) — 2.73× slower (calibrated)
• org.apache.groovy.bench.AryBench.groovyCS ( {"n":"1000000"} ) — 2.73× slower (calibrated)
• org.apache.groovy.bench.StaticMethodCallIndyBench.staticChain_groovyCS — 1.70× slower (calibrated)

⚠️ Runner speed differs ≥15% from the historical baseline hardware for: bench, core-hz, grails-ad. Raw speedups are not meaningful for those parts — use the calibrated column.

_{Runner calibration (this run vs baseline hardware): bench 1.29× (27 rulers) · core-ag 0.87× (3 rulers) · core-hz 1.25× (3 rulers) · grails-ad 1.23× (3 rulers) · grails-ez 0.92× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

[github-actions]

JMH summary — indy (commit 729bae7)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group	Speedup	Calibrated	n
bench	1.115 ×	0.952 ×	134
core	2.180 ×	2.176 ×	108
grails	1.407 ×	1.313 ×	80

⚠️ 3 benchmarks at least 1.5× slower than the 90-day baseline:

• org.apache.groovy.bench.AryBench.groovyCS ( {"n":"1000000"} ) — 2.17× slower (calibrated)
• org.apache.groovy.bench.StaticMethodCallIndyBench.staticChain_groovyCS — 1.57× slower (calibrated)
• org.apache.groovy.bench.StaticMethodCallIndyBench.instanceChain_groovy — 1.55× slower (calibrated)

⚠️ Runner speed differs ≥15% from the historical baseline hardware for: bench, core-hz, grails-ez. Raw speedups are not meaningful for those parts — use the calibrated column.

_{Runner calibration (this run vs baseline hardware): bench 1.16× (27 rulers) · core-ag 0.89× (3 rulers) · core-hz 1.23× (3 rulers) · grails-ad 0.93× (3 rulers) · grails-ez 1.20× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

[blackdrag]

why not work with a LinkedHashSet from the beginning? And why transform it to a unmodifiableSet?

[daniellansun]

ordered() now inserts into a LinkedHashSet while it walks precedence. Match order is insertion order, and there is no ArrayList to copy afterwards.

The unmodifiable wrapper stays. Collections.unmodifiableSet does not copy; it only wraps that set. Callers publish the result: ResolveVisitor stores it in a ConcurrentHashMap, and until a later hit replaces the entry the same instance is the startup index (Groovy12725 checks that with assertSame). A reader on another thread must not observe a set being edited, and the tests expect add to throw. An update replaces the map entry. An empty result is still Collections.emptySet(), and a single match is still Collections.singleton, which is already immutable.

Package precedence and the replace-rather-than-mutate cache behaviour are unchanged.

[blackdrag]

The unmodifiable wrapper stays. Collections.unmodifiableSet does not copy; it only wraps that set. Callers publish the result: ResolveVisitor stores it in a ConcurrentHashMap, and until a later hit replaces the entry the same instance is the startup index (Groovy12725 checks that with assertSame). A reader on another thread must not observe a set being edited, and the tests expect add to throw. An update replaces the map entry. An empty result is still Collections.emptySet(), and a single match is still Collections.singleton, which is already immutable.

I assume you mean src/test/groovy/org/apache/groovy/vmplugin/internal/DefaultImportPackagesTest.groovy. This test does not establish multi threading behaviour. It tests an API and can maybe be used as regression test to ensure the ordering logic is kept. But it does not explain why this is needed. What is the goal? To catch the mistake during development? It most likely is not to enable multithreading behaviour. Also unmodifiable is not the same as immutable. A unmodifiableSet may still have publication issues based on the underlying collection. Again... I am not saying this is wrong. Just want a more clear picture why here exactly like this and not different. Just pointing to the test with the argument of thread safety and then having a test that does not test that if not convincing.

[daniellansun]

You are right on both counts. DefaultImportPackagesTest (and the add throws checks in Groovy12725) are API / regression tests: they lock the iteration order and the fact that a caller cannot add to a returned set. They do not establish concurrent behaviour, and I should not have pointed at them as if they did.

The concurrency design is not the wrapper itself. It is that an update replaces the ConcurrentHashMap entry instead of mutating the set a reader may already be iterating. compute in ResolveVisitor.resolveOneDefaultImport calls including(), which returns either the same instance or a new set; it never adds to a set that has already been published. The map provides the safe publication of that reference. unmodifiableSet does not invent that race freedom, and it is also not the same as an immutable set: if the backing LinkedHashSet were still written after the wrap, the view would change and you could have a publication problem.

What the wrapper is for: the same set instance is shared. Until a later hit replaces a cache entry, the cache holds the startup index sets (assertSame in Groovy12725). A caller who could mutate a cache value would also mutate that snapshot. The wrapper is a defensive contract against that, and the add tests are there to catch a later change that starts returning a mutable set or updating in place. They are not a thread-safety proof.

I used Collections.unmodifiableSet rather than Set.copyOf / Set.of because those do not keep the LinkedHashSet insertion order, which is the precedence order. The mutable set is a local in ordered(); it is not retained or written after the wrap, so the published view does not change. Empty and single-element results are already Collections.emptySet() and Collections.singleton.

[blackdrag]

There are two aspects I would like to see addressed before merging:

1. Verify cache behavior across different classloaders

DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE is static and shared across compiler instances, whereas class visibility is determined by the classloader used for each compilation.

The new partial-cache algorithm appears to account for this by checking higher-precedence packages before accepting a previously discovered lower-precedence package. However, the regression tests currently do not establish that resolution remains independent of compilation history across different classloaders.

I suggest adding a test along these lines:

• Classloader A can resolve groovy.util.X, but not groovy.lang.X.
• Compile a reference to X using A.
• Classloader B can resolve groovy.lang.X, but not groovy.util.X.
• Compile a reference to X using B.
• Compile another reference using A.

The expected resolutions are respectively groovy.util.X, groovy.lang.X and groovy.util.X.

This would verify that the globally shared cache does not cause one compilation's classloader visibility to influence another compilation's resolution.

2. Make the cache-state invariant explicit

The distinction between complete and partial cache entries currently relies on object identity:

known == indexed

This works with the current implementation because startup index entries are shared with the cache, while subsequent updates replace cache values rather than modifying them.

However, object identity now carries semantic meaning: it determines which resolution algorithm is used. This is an important invariant for anyone modifying this code in the future.

I suggest documenting explicitly that startup entries represent the complete index, dynamically discovered entries are only partial information, and cache updates must preserve this distinction.

I would also consider a concurrency regression test for simultaneous resolutions of the same name. The replacement-based update strategy using ConcurrentHashMap.compute looks appropriate, but the current tests do not exercise concurrent readers and writers.

Overall, I agree with the implementation direction. These suggestions are primarily about establishing and preserving the correctness properties on which the optimization depends.

[daniellansun]

Thanks for the suggestions. I added two regression tests in Groovy12725, and documented the complete-versus-partial cache invariant next to DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE and the known == indexed check.

defaultImportCacheIsIndependentAcrossClassLoaders uses a synthetic name CrossLoaderProbe12725. Loader A can resolve it only from groovy.util, loader B only from groovy.lang. Compiling on A, then B, then A again resolves groovy.util, then groovy.lang, then groovy.util. After those three compilations the shared cache holds both packages, in precedence order.

The new comments describe a startup cache entry as the complete index, and a replaced entry as only partial information. The distinction is object identity, because updates replace the map entry rather than mutating a shared startup set.

I also added concurrentResolutionsOfTheSameNameHonorEachClassLoader: eight threads, four rounds, started together with a CyclicBarrier. Even threads see only groovy.util, odd threads only groovy.lang. Each resolution matches that loader, and the final cache contains both packages as an immutable set.

The resolution algorithm itself is unchanged.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
99.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 2df506b
▶️ Tests: 131803 executed
⚪️ Checks: 27/27 completed

---

Learn more about TestLens at testlens.app/docs.