Fix nondeterministic dedup caused by stale StyleRuleKey#1236
Open
7rulnik wants to merge 1 commit into
Open
Conversation
`StyleRuleKey` stored a precomputed hash but its `PartialEq` re-read the
live `Vec<CssRule>` by index. After `merge_style_rules` mutated an
indexed rule in place (extending its declarations or selectors), the
stored hash went stale while `Eq` saw the new content, violating the
`Hash`/`Eq` invariant.
The bug surfaces two ways:
- **Cross-process nondeterminism.** With per-process `ahash` keys, the
"stale-key lookup hits an unrelated rule" outcome varies between
processes, so the same input produces different minified output.
- **Deterministic correctness.** A later rule whose snapshot equals an
earlier rule's *pre-merge* snapshot can mark the merged rule as
`Ignored`, silently dropping merged-in declarations. Or a later rule
whose post-merge content matches a rule's *pre-merge* hash bucket
can fail to dedup. Example:
.a { color: red }
.a { background: blue } /* merges into rules[0] */
.b { background: blue }
.a { color: red } /* used to wrongly remove rules[0] */
The fix replaces the `HashMap<StyleRuleKey, usize>` (where the key
embedded a precomputed hash beside a live-Vec borrow) with a hash bucket
map: `HashMap<u64, SmallVec<[usize; 1]>>`. The map key is just the
immutable dedup hash; bucket entries carry rule indices, and equality
is verified against the live rule via `is_duplicate` on lookup. This
removes the divergent-Hash/Eq invariant entirely and avoids the per-rule
`SelectorList::clone()` and `PropertyId` collection that an earlier
snapshot-based fix attempt required.
Bookkeeping:
- On insert: append `idx` to the bucket for `rule.dedup_hash()`.
- On dedup lookup: scan the bucket, call `is_duplicate` against the
candidate rule, take the first match.
- On `merge_style_rules` success: if the rule's hash changed (sel_eq /
decl_eq selector-extend paths), move `idx` from the old bucket to
the new. The vendor-prefix-only branch leaves selectors and property
ids unchanged and short-circuits as a no-op.
- On `rules.pop()` in the cascade loop: drop the popped rule's bucket
entry so it can't false-positive against a future rule pushed at
the same index.
Adds four regression tests:
- `test_dedup_finds_duplicate_after_merge_collapses_longhands` —
fails on upstream; missed dedup after sel_eq + shorthand collapse.
- `test_dedup_finds_duplicate_after_cascading_merge` —
fails on upstream; missed dedup after cascading sel_eq merges.
- `test_dedup_after_merge_preserves_merged_declarations` —
guards a partial fix that snapshots keys but skips map refresh.
- `test_cascade_pop_clears_orphaned_style_rules_entry` —
guards the cascade-pop arm of the partial-fix regression.
Performance: within noise of upstream on the captured selene chunks
(7805-line and 8717-line inputs both within ±5% across hyperfine runs).
The randomized per-process `AHasher::default()` keys are no longer
correctness-relevant; they only affect bucket placement, not which
rules are considered duplicates.
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.
Fixes #1235.
Approach
Replace
HashMap<StyleRuleKey, usize>withHashMap<u64, SmallVec<[usize; 1]>>— buckets of rule indices keyed byrule.hash_key(). Equality is verified against the live rule viais_duplicateon lookup. The merge and cascade-pop sites move/drop the index when a rule's content changes, so entries can't go stale.This removes the divergent
Hash/Eqinvariant entirely and avoids the per-ruleSelectorList::clone()+PropertyIdcollection that snapshot-based fix attempts require.Tests
Five regression tests covering each merge/cascade arm:
test_dedup_finds_duplicate_after_merge_collapses_longhands— fails on master.test_dedup_finds_duplicate_after_cascading_merge— fails on master.test_dedup_after_merge_preserves_merged_declarationsandtest_cascade_pop_clears_orphaned_style_rules_entry— guard a partial fix that snapshots keys but skips map refresh.test_css_module_dedup_skips_cross_file_candidate— covers the CSS-modules cross-file gate against the new bucket-scan lookup.Performance
Within hyperfine noise on two real-world CSS chunks (7805 and 8717 lines, 500 minify iters × 15 runs each):
Deltas land within run-to-run variance and in opposite directions on the two inputs.