ParparVM collections: fix an O(n) HashMap miss, and compact Hashtable - #5722
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b0ce25fa0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int capacity = cn1Meta.length; | ||
| // Double only when genuinely full; a table that is merely | ||
| // tombstone-heavy is rebuilt at the same size, which purges them. | ||
| int newCapacity = (elementCount * 2 >= capacity) ? capacity << 1 : capacity; |
There was a problem hiding this comment.
When a caller supplies a load factor below 0.5, reaching threshold does not satisfy this hard-coded half-capacity test, so rehash() rebuilds at the same size even when there are no tombstones. The rebuilt table remains at or above its unchanged threshold, causing every subsequent put to rehash all live entries until the map becomes half full; with very small positive load factors this makes bulk insertion quadratic and effectively ignores the requested load factor. Choose growth based on the configured threshold/live count, reserving same-capacity rebuilds for tables whose threshold was reached because of tombstones.
Useful? React with 👍 / 👎.
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
There was a problem hiding this comment.
💡 Codex Review
CodenameOne/vm/JavaAPI/src/java/util/Hashtable.java
Lines 407 to 408 in 44f97a8
Avoid allocating an Entry for every containsKey hit
When containsKey finds a present key, it delegates to getEntry, which constructs a new Entry solely so this method can compare it with null. This turns every successful membership check into an allocation, whereas the previous chained representation reused its stored entry and the compact probe can answer directly from the returned index. Hot paths using Hashtable.containsKey, such as the layout and theme maps, will therefore create avoidable garbage; check cn1FindSlot(key) >= 0 here instead.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
HashMap here is open addressed with linear probing, and cn1Marker spreads the hash with the JDK's h ^= h >>> 16 -- a spread designed for a CHAINED map, where colliding keys share a bucket and clustering costs nothing. Integer.hashCode() is the value itself, so a dense key range (ids from zero, epoch seconds, counters, indices) lands at slot == value: one contiguous run of occupied slots with no gap. Linear probing then walked that entire run for any probe that entered it and did not find its key. Measured average probe length for a MISS: 2547 slots at 20k keys, 16742 at 100k, 222721 at 1M -- O(n) per unsuccessful lookup, reaching get returning null, containsKey returning false, and put of a key not adjacent to the run. In wall time, 3M containsKey calls against a 100k map took 32.7 SECONDS against 49.9ms on HotSpot. A remove/insert churn shape took 7.8 seconds against 12.5ms. Hits stayed at exactly one probe throughout, which is why hashMapChurn -- get and put on keys that are PRESENT -- reported a healthy 1.12x the whole time. The fix is the probe SEQUENCE, not the spread. Scrambling the hash was tried first and rejected on measurement: it fixes the miss but destroys the sequential placement, and dense-key build and scan shapes regressed 1.8x-2.2x (HotSpot's HashMap gets that same locality from that same weak spread). Keeping the first probe at marker & mask and perturbing only the steps after it -- CPython's dict recurrence -- keeps the locality and still leaves the run at once. missHeavy 32698ms -> 44.9ms (728x) tombstones 7781ms -> 16.5ms (471x) stringKeys 33.6 -> 25.5ms largeTable 26.6 -> 33.3ms (the cost) vm/benchmarks geomean 1.005, i.e. unmoved Hashtable gets the same compact layout it never received: no Entry object per mapping, a power-of-two mask instead of the % integer division it did on every operation, and the same perturbed probe. Build 1.74x. Lookup only 1.09x, and that is the useful part -- with identical probe code Hashtable is still 3x HashMap on the same workload, and the difference is synchronized. This VM keeps monitors in an address-keyed side table rather than an object header word, so an uncontended accessor costs more than the whole lookup. Further work there belongs on the monitor, not the map. IdentityHashMap indexed with % (length / 2) on an unscrambled identity hash. Copying java.util.IdentityHashMap's hash function made it SLOWER, and the reason is worth recording: HotSpot's identityHashCode is a scrambled per-object value, ours is a truncated object ADDRESS, measured 32-byte aligned. The JDK's (h << 1) - (h << 8) is h * -254, an EVEN multiplier, so it preserves those five zero bits and adds a sixth -- 2045 distinct home slots out of 65536 and 12.73 probes per lookup. The old modulo survived only because a non-power-of-two modulus folds high bits back in as a side effect. Folding explicitly (h ^= h >>> 16) reaches 50000/65536 slots and 1.00 probes; adding a multiply on top makes it worse again, because sequential allocation means sequential addresses and one fold is already near a perfect hash. 2.65x on lookup and build. Its rehash() overflow guard also turned an overflowed length into 1 -- an odd array length, which would have split every key from its value. Tests: MapBench adds the map shapes hashMapChurn cannot reach (miss-heavy, String-keyed, large-table, tombstone-heavy, grow-dominated, identity-keyed). It is deliberately outside CommonWorkloads, which port_status.py pins at exactly ten ids. HtTorture and IdmTorture join the gauntlet; HtTorture was written and verified against the CHAINED Hashtable before the rewrite, and IdmTorture was verified non-vacuous by injecting a relocation off-by-one that it caught. IdmProbe is a diagnostic that must run on the target, because the identity-hash distribution is a property of the allocator. Also removes three dead constants from parparvm_runtime.js, two of which named HashMap fields that the compact layout deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check-copyright-headers fails any file a PR touches whose header is neither the Codename One nor the Oracle GPLv2 + Classpath text. HashMap, Hashtable and IdentityHashMap carry the Apache Software Foundation notice, because they are Apache Harmony derived -- byte-identical in provenance to TimeZone.java and LinkedHashMap.java, which the exclusions file already lists for exactly this reason (LinkedHashMap was added when #5658 last modified it). Excluding them is the correct fix and replacing the header is not: rewriting an Apache-2.0 notice as the Codename One GPL header would misstate the licence of third-party code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s allocation Two review findings, both real. The growth rule. All three compact maps chose between doubling and rebuilding in place with `elementCount * 2 >= capacity` -- a capacity test standing in for a threshold test, which silently assumes a load factor of 0.5 or more. Below that, the threshold is reached while the table is still less than half full, so the rebuild keeps the same capacity, the rebuilt table is immediately at its threshold again, and every subsequent put rebuilds the whole table. Inserting 20000 entries at a load factor of 0.25 did 19999 rebuilds and rehashed 200 million entries: 22.3 SECONDS, against 16.4ms once fixed. The two-argument constructors take any positive load factor, so `new HashMap<>(16, 0.25f)` reached it from ordinary code. `elementCount >= threshold` is the rule -- grow when the LIVE count has reached the threshold, rebuild at the same size only when tombstones are what pushed occupancy there. At 0.75 and 0.5 the two rules agree rebuild for rebuild, which is why nothing existing moved, and why nothing existing could have caught it: `MapBench.lowLoadFactorBuild` and `HtTorture`'s sparse case exist for this alone, and the benchmark was verified to reproduce the 22.3s before the fix. Fixed in Hashtable, HashMap and LinkedHashMap. Only Hashtable's was new here -- the other two carried it from #5327 -- but it is one rule with one fix, and leaving two of the three would have been arbitrary. containsKey. The compact Hashtable delegated to getEntry, which built an Entry purely so the caller could compare it against null, turning every successful membership test into an allocation; the chained representation it replaced handed back the entry it already had. keySet().contains() routes here. It now answers from the probe index. The other getEntry caller, entrySet().contains(), genuinely needs the Entry for its equality test and is unchanged. GAUNTLET GREEN. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
parparvm-map-probe-and-compact-hashtable
branch
from
September 6, 2026 18:04
44f97a8 to
bf080fe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf080fe015
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+760
to
+762
| if (cn1Occupied >= threshold) { | ||
| rehash(); | ||
| } |
There was a problem hiding this comment.
When a Hashtable subclass overrides the protected rehash() method without calling super.rehash()—which remained functionally safe with the previous chained representation—insertions can now fill every open-addressed slot. Since cn1FindSlot() terminates only upon finding META_EMPTY and has no full-cycle check, the next operation for a missing key loops forever. Perform the growth needed to preserve an empty slot through a non-overridable internal method, rather than relying on the subclass hook.
Useful? React with 👍 / 👎.
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale lives in the code comments and in
vm/CLAUDE.md; this is the summary.The bug
java.util.HashMapon ParparVM is open addressed with linear probing, andcn1Markerspreads the hash with the JDK'sh ^= h >>> 16-- a spread designed for a chained map, where colliding keys share a bucket and clustering costs nothing.Integer.hashCode()is the value itself, so a dense key range (ids from zero, epoch seconds, counters, indices) lands atslot == value: one contiguous run with no gap. Linear probing then walked that whole run for any probe that entered it and did not find its key.Average probe length for a miss:
O(n) per unsuccessful lookup, reaching
getreturning null,containsKeyreturning false, andputof a key not adjacent to the run. 3McontainsKeycalls against a 100k map took 32.7 seconds, against 49.9ms on HotSpot.Hits stayed at exactly one probe throughout, which is why
hashMapChurn-- get and put on keys that are present -- reported a healthy 1.12x the entire time it existed.The fix
The probe sequence, not the spread (CPython's dict recurrence). Scrambling the hash was tried first and rejected on measurement: it fixes the miss but destroys the sequential placement, and dense-key build/scan shapes regressed 1.8x-2.2x -- HotSpot's
HashMapgets that same locality from that same weak spread. Keeping the first probe atmarker & maskand perturbing only the steps after it keeps the locality and still leaves the run at once.Measured, interleaved best-of-N, all checksums bit-identical
vm/benchmarksgeomean moved 1.005, i.e. not at all.Hashtable
Given the compact layout it never received: no
Entryper mapping, a power-of-two mask instead of the%integer division it did on every operation, and the same perturbed probe. Build 1.74x.Lookup only 1.09x, and that is the useful part: with identical probe code
Hashtableis still 3xHashMapon the same workload, and the difference issynchronized. This VM keeps monitors in an address-keyed side table rather than an object header word, so an uncontended accessor costs more than the whole lookup. Further work there belongs on the monitor, not the map.IdentityHashMap
Do not port HotSpot's hash function to this VM. Copying
java.util.IdentityHashMap's made it measurably slower. HotSpot'sidentityHashCodeis a scrambled per-object value; ours is a truncated object address, measured 32-byte aligned. The JDK's(h << 1) - (h << 8)ish * -254-- an even multiplier, so it preserves those five zero bits and adds a sixth: 2045 distinct home slots out of 65536, 12.73 probes per lookup. The old%survived only because a non-power-of-two modulus folds high bits back in as a side effect.Folding explicitly (
h ^= h >>> 16) reaches 50000/65536 slots and 1.00 probes. Adding a multiply on top makes it worse again (2.36) -- sequential allocation means sequential addresses, so one fold is already near a perfect hash. 2.65x on lookup and build.Its
rehash()overflow guard also turned an overflowed length into1-- an odd array length, which would have split every key from its value.Tests
MapBenchadds the shapeshashMapChurncannot reach: miss-heavy, String-keyed, large-table, tombstone-heavy, grow-dominated, identity-keyed. Deliberately outsideCommonWorkloads, whichport_status.pypins at exactly ten ids.HtTorture(69 assertions) andIdmTorturejoin the gauntlet.HtTorturewas written and verified against the chainedHashtablebefore the rewrite -- a torture that only ever runs against new code proves nothing.IdmTorturewas verified non-vacuous by injecting a relocation off-by-one, which it caught.IdmProbeis a diagnostic, not a benchmark: it must run on the target, because the identity-hash distribution is a property of the allocator.GAUNTLET GREEN(all tortures bit-identical to HotSpot),vm/benchmarkschecksums bit-identical.Also removes three dead constants from
parparvm_runtime.js, two of which namedHashMapfields the compact layout deleted.Not addressed
LinkedHashMapinherits the compact layout but overridesget/put/remove/clearin Java rather than native (the natives are deliberately base-class-only), which measures 1.21x overHashMapon the same shape.🤖 Generated with Claude Code