[
https://issues.apache.org/jira/browse/GROOVY-12725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140241#comment-18140241
]
ASF GitHub Bot commented on GROOVY-12725:
-----------------------------------------
github-actions[bot] commented on PR #3260:
URL: https://github.com/apache/groovy/pull/3260#issuecomment-6706708660
### JMH summary — classic (commit `a693309`)
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.647 × | 0.939 × | 124 |
| core | 1.154 × | 1.044 × | 108 |
| grails | 1.581 × | 1.452 × | 80 |
> ⚠️ **2 benchmarks at least 1.5× slower than the 90-day baseline:**
> - `org.apache.groovy.bench.AryBench.groovyCS ( {"n":"10"} )` — 1.72×
slower (calibrated)
> -
`org.apache.groovy.bench.CachedMethodInvokerBench.generate_reflective_fresh (
{"prefix":"abc","receiver":"abcdef"} )` — 1.54× slower (calibrated)
> ⚠️ Runner speed differs ≥15% from the historical baseline hardware for:
bench, core-ag, grails-ad, grails-ez. Raw speedups are not meaningful for those
parts — use the calibrated column.
<sub>Runner calibration (this run vs baseline hardware): bench 1.72× (27
rulers) · core-ag 1.25× (3 rulers) · core-hz 0.89× (3 rulers) · grails-ad 0.85×
(3 rulers) · grails-ez 1.33× (3 rulers)</sub>
<sub>Baseline: <code>dev/bench/jmh/<part>/classic/data.js</code> on
gh-pages, trailing 90 days. <a
href="https://apache.github.io/groovy/dev/bench/jmh/summary.html";>Daily
dashboard</a> · <a
href="https://apache.github.io/groovy/dev/bench/jmh/";>Per-suite raw
data</a></sub>
<!
> Preserve default-import package precedence and thread-safety in ResolveVisitor
> ------------------------------------------------------------------------------
>
> Key: GROOVY-12725
> URL: https://issues.apache.org/jira/browse/GROOVY-12725
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Assignee: Daniel Sun
> Priority: Major
>
> h2. Overview
> {{ResolveVisitor}} maintains a process-wide cache
> ({{DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE}}) mapping simple class names to
> the set of default-imported package prefixes ({{java.lang.}}, {{java.io.}},
> {{java.net.}}, {{java.util.}}, {{groovy.lang.}}, {{groovy.util.}}) that
> contain a class with that simple name. Three issues in how this cache is
> initialized and updated can cause package precedence inversion, lost package
> mappings for colliding class names, or data races during concurrent
> compilation.
> h2. Problem Description
> h3. 1. Overwritten package mappings in {{Java9.getDefaultImportClasses}}
> {{Java9.getDefaultImportClasses(String[] packageNames)}} previously populated
> its result map via {{result.putAll(doFindClasses(...))}} across
> {{groovy-core}} locations and {{jrt:/modules/java.base/}}. When the same
> simple class name exists in both a Groovy default-imported package and a Java
> default-imported package (for example, {{Proxy}} in both {{java.net.Proxy}}
> and {{groovy.util.Proxy}}), the later {{putAll}} call overwrote the earlier
> entry instead of merging both package prefixes. In addition, the stored value
> sets were mutable, unordered {{HashSet}} instances rather than unmodifiable
> sets ordered by {{ResolveVisitor.DEFAULT_IMPORTS}}.
> {code:java}
> URI gcjLocation =
> DefaultGroovyMethods.getLocation(gcl.loadClass("groovy.lang.GroovySystem")).toURI();
> result.putAll(doFindClasses(gcjLocation, "groovy", groovyPackages));
> ...
> result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java",
> javaPackages));
> {code}
> h3. 2. Non-atomic {{Map}} default methods in {{UnlimitedConcurrentCache}}
> {{UnlimitedConcurrentCache}} wraps a {{ConcurrentHashMap}} and implements
> {{EvictableCache}} (which extends {{java.util.Map}}), but did not override
> the atomic {{Map}} methods introduced in Java 8 ({{compute}},
> {{computeIfAbsent}}, {{computeIfPresent}}, {{merge}}, {{putIfAbsent}},
> {{remove(Object, Object)}}, and {{replace}}). Consequently, calling
> {{DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE.computeIfAbsent(...)}} invoked the
> non-atomic {{Map}} default method rather than
> {{ConcurrentHashMap.computeIfAbsent}}.
> h3. 3. In-place {{HashSet}} mutation and lost {{DEFAULT_IMPORTS}} ordering in
> {{ResolveVisitor.resolveFromDefaultImports}}
> On a cache miss during {{resolveFromDefaultImports}}, {{ResolveVisitor}}
> obtained a mutable {{HashSet}} from
> {{DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE}} and mutated it in place:
> {code:java}
> Set<String> packagePrefixSet =
> DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE.computeIfAbsent(typeName, key -> new
> HashSet<>(2));
> packagePrefixSet.add(packagePrefix);
> {code}
> * Concurrent compiler threads iterating over the cached {{Set<String>}} could
> race with in-place {{HashSet.add}} mutations.
> * Because {{HashSet}} iteration order is arbitrary and does not follow
> {{ResolveVisitor.DEFAULT_IMPORTS}}, discovering a class in a lower-precedence
> package (e.g. {{groovy.util.}}) before a higher-precedence package (e.g.
> {{groovy.lang.}}) could cause subsequent lookups to resolve the
> lower-precedence class even when both are available on the classpath.
> h2. Solution
> * Merge package sets across all scanned locations in
> {{Java9.getDefaultImportClasses}} instead of overwriting entries with
> {{Map.putAll}}, and store unmodifiable sets ordered according to
> {{packageNames}} ({{ResolveVisitor.DEFAULT_IMPORTS}}).
> * Delegate atomic {{Map}} operations ({{compute}}, {{computeIfAbsent}},
> {{computeIfPresent}}, {{merge}}, {{putIfAbsent}}, {{remove(Object, Object)}},
> {{replace}}, {{replaceAll}}, {{getOrDefault}}, and {{forEach}}) in
> {{UnlimitedConcurrentCache}} directly to the underlying {{ConcurrentHashMap}}.
> * Update {{ResolveVisitor.DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE}}
> atomically via {{compute(...)}} using unmodifiable sets ordered according to
> {{DEFAULT_IMPORTS}}, preventing concurrent mutation races and preserving
> default-import package precedence.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
