Repository navigation
Use a per-process hash for the dynamic set - #315
Open
SimonSapin wants to merge 1 commit into
Open
SimonSapin wants to merge 1 commit into
SimonSapin wants to merge 1 commit into
Conversation
## Background Interning a string goes through `impl From<Cow<str>> for Atom` which does: 1. If the string is short enough, make an inline atom 2. Otherwise if the string is part of the static set, make a static atom 3. Otherwise insert the string in the dynamic set and make a dynamic atom Step 2 involves hashing the string with a keyed hash function whose key is chosen at compile-time by the `phf` crate. phf key search uses a PRNG but with a fixed seed, so the key is deterministic for a given static set. Step 3 involves a separate hash set. To save CPU cycles, string-cache v0.11.0 and earlier re-use the hash already computed from step 2. The dynamic hash set is a custom implementation with a fixed size and no open addressing, so a 12-bit hash collision is enough to put multiple strings in the same bucket and hit the map’s worst case linear time insertion. As reported in #314, an HTML document adversarially crafted to have many tag or attribute names with hash collisions in a given version of Servo could take quadratic time to parse in that version, just for string interning. ## Mitigation This commit changes the dynamic set to not reuse the hash from the static set, but instead compute its own hash with a per-process instance the standard library’s default hasher `std::hash::RandomState`, another keyed hash function that picks random keys when initialized. The worst case for dynamic atom interning is still linear time to the number of items in the same bucket, but with a random hash key collisions are harder to craft. ## Benchmarking disclaimer All benchmarks results mentioned here are from a laptop. While benches were running the CPU temperature varied between 60°C and 99°C and fan speed beetween off and maximum, so it’s likely that the CPU speed was thermal-throttled some but not all of the time. Precise numbers should be considered unreliable but hopefully we can still discern broad trends. Still it’d be nice if someone wants to reproduce on hardware with more stable clock speed. ## Micro-benchmarks Filtering micro-benchmarks in this repository to those that do string interning: ``` git switch main cargo bench -p integration_tests --bench micro -- --save-baseline 2026-11-03 intern git switch dynamic-hash # this commit cargo bench -p integration_tests --bench micro -- --baseline 2026-11-03 intern ``` <details> <summary>Full criterion output</summary> ``` static_atom intern time: [11.537 ns 11.677 ns 11.857 ns] change: [−2.5985% −1.7356% −0.5713%] (p = 0.00 < 0.05) Change within noise threshold. Found 4 outliers among 100 measurements (4.00%) 4 (4.00%) high severe short_inline_atom intern time: [6.2984 ns 6.3004 ns 6.3027 ns] change: [+5.5323% +5.7550% +5.9245%] (p = 0.00 < 0.05) Performance has regressed. Found 8 outliers among 100 measurements (8.00%) 2 (2.00%) high mild 6 (6.00%) high severe medium_inline_atom intern time: [2.1287 ns 2.1320 ns 2.1357 ns] change: [+1.4034% +1.8931% +2.3395%] (p = 0.00 < 0.05) Performance has regressed. Found 8 outliers among 100 measurements (8.00%) 6 (6.00%) high mild 2 (2.00%) high severe min_dynamic_atom intern time: [38.216 ns 38.245 ns 38.279 ns] change: [+17.366% +17.463% +17.560%] (p = 0.00 < 0.05) Performance has regressed. Found 4 outliers among 100 measurements (4.00%) 3 (3.00%) high mild 1 (1.00%) high severe longer_dynamic_atom intern time: [49.913 ns 50.593 ns 51.457 ns] change: [+18.758% +20.419% +22.943%] (p = 0.00 < 0.05) Performance has regressed. Found 4 outliers among 100 measurements (4.00%) 4 (4.00%) high severe static_at_runtime intern time: [11.546 ns 11.555 ns 11.567 ns] change: [−1.2172% −1.1122% −1.0048%] (p = 0.00 < 0.05) Performance has improved. Found 12 outliers among 100 measurements (12.00%) 9 (9.00%) high mild 3 (3.00%) high severe intern rand 008 time: [60.782 ns 60.815 ns 60.851 ns] change: [+11.075% +11.158% +11.241%] (p = 0.00 < 0.05) Performance has regressed. intern rand 032 time: [62.676 ns 63.448 ns 64.446 ns] change: [+4.9985% +5.8914% +7.2063%] (p = 0.00 < 0.05) Performance has regressed. Found 7 outliers among 100 measurements (7.00%) 2 (2.00%) high mild 5 (5.00%) high severe intern rand 128 time: [92.238 ns 93.480 ns 95.054 ns] change: [+18.802% +20.677% +23.672%] (p = 0.00 < 0.05) Performance has regressed. Found 7 outliers among 100 measurements (7.00%) 2 (2.00%) high mild 5 (5.00%) high severe intern rand 512 time: [229.45 ns 229.58 ns 229.74 ns] change: [+41.554% +41.679% +41.801%] (p = 0.00 < 0.05) Performance has regressed. Found 9 outliers among 100 measurements (9.00%) 5 (5.00%) high mild 4 (4.00%) high severe ``` </details> Some number differences for static and inline atoms are deemed statistically significant by Criterion but since no relevant source code has changed I’m attributing this to thermal throttling. As expected interning dynamic atoms is now slower, from +11% time for 8 byte strings to +41% time for 512 bytes. However this is doing nothing but string interning. ## HTML parsing benchmarks On servo/html5ever#813, which adds an HTML input file with all tag and attribute named mofidied to be dynamic atoms. ``` # with string-cache from crates.io cargo bench -p markup5ever_rcdom --bench deserialize-serialize -- --save-baseline 2026-11-03 servo $EDITOR Cargo.toml # use patched string-cache, see below cargo bench -p markup5ever_rcdom --bench deserialize-serialize -- --baseline 2026-11-03 servo ``` To use a patched string-cache, add to html5ever’s root `Cargo.toml`: ```toml [patch.crates-io] string_cache = {path = "../string-cache"} # string-cache on this commit ``` <details> <summary>Full criterion output</summary> ``` deserialize "servo.org.html" time: [225.43 µs 225.55 µs 225.67 µs] change: [+1.7193% +1.8193% +1.9216%] (p = 0.00 < 0.05) Performance has regressed. Found 4 outliers among 100 measurements (4.00%) 1 (1.00%) low mild 2 (2.00%) high mild 1 (1.00%) high severe Benchmarking deserialize "servo.org-custom.html": Collecting 100 samples in esdeserialize "servo.org-custom.html" time: [302.33 µs 302.72 µs 303.08 µs] change: [+0.4744% +0.6366% +0.7950%] (p = 0.00 < 0.05) Change within noise threshold. Found 1 outliers among 100 measurements (1.00%) 1 (1.00%) low mild Benchmarking serialize "servo.org.html": Collecting 100 samples in estimated 5serialize "servo.org.html" time: [18.591 µs 18.643 µs 18.715 µs] change: [−1.0722% +0.8353% +2.7065%] (p = 0.39 > 0.05) No change in performance detected. Found 13 outliers among 100 measurements (13.00%) 10 (10.00%) low severe 3 (3.00%) high severe Benchmarking serialize "servo.org-custom.html": Collecting 100 samples in estiserialize "servo.org-custom.html" time: [27.822 µs 27.858 µs 27.890 µs] change: [−4.6615% −3.6017% −2.4905%] (p = 0.00 < 0.05) Performance has improved. Found 16 outliers among 100 measurements (16.00%) 16 (16.00%) low severe ``` </details> String interning only happens during HTML deserialize, and `servo.org.html` contains ~no dynamic atoms so only `deserialize "servo.org-custom.html"` is relevant here. Once again I attribute other changes to thermal throttling. `deserialize "servo.org-custom.html"` shows not siginificant change, which I believe indicates that the performance regression on string interning is small when part of HTML parsing. Signed-off-by: Simon Sapin <simon@igalia.com>
Contributor
If the spurious changes are due to thermals, how can the relevant (non-)changes be relied on to not be due to thermals? That does not really make sense IMHO. Even for laptops, it might be possible to disable frequency boosting, switch to a "performance" power profile and pin the benchmarks to a given core to produce more reliable results. I think at least having benchmarks unaffected by source changes also yield insignificant measurement differences would give more confidence in broader trends for the rest. Alternatively, the benchmarks could be run under emulation using Callgrind to compare instruction counts as a metric unaffected by thermal throttling. |
Member
Author
|
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
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.
Background
Interning a string goes through
impl From<Cow<str>> for Atomwhich does:Step 2 involves hashing the string with a keyed hash function whose key is chosen at compile-time by the
phfcrate. phf key search uses a PRNG but with a fixed seed, so the key is deterministic for a given static set.Step 3 involves a separate hash set. To save CPU cycles, string-cache v0.11.0 and earlier re-use the hash already computed from step 2. The dynamic hash set is a custom implementation with a fixed size and no open addressing, so a 12-bit hash collision is enough to put multiple strings in the same bucket and hit the map’s worst case linear time insertion.
As reported in #314, an HTML document adversarially crafted to have many tag or attribute names with hash collisions in a given version of Servo could take quadratic time to parse in that version, just for string interning.
Mitigation
This commit changes the dynamic set to not reuse the hash from the static set, but instead compute its own hash with a per-process instance the standard library’s default hasher
std::hash::RandomState, another keyed hash function that picks random keys when initialized.The worst case for dynamic atom interning is still linear time to the number of items in the same bucket, but with a random hash key collisions are harder to craft.
Benchmarking disclaimer
All benchmarks results mentioned here are from a laptop. While benches were running the CPU temperature varied between 60°C and 99°C and fan speed beetween off and maximum, so it’s likely that the CPU speed was thermal-throttled some but not all of the time.
Precise numbers should be considered unreliable but hopefully we can still discern broad trends. Still it’d be nice if someone wants to reproduce on hardware with more stable clock speed.
Micro-benchmarks
Filtering micro-benchmarks in this repository to those that do string interning:
Full criterion output
Some number differences for static and inline atoms are deemed statistically significant by Criterion but since no relevant source code has changed I’m attributing this to thermal throttling.
As expected interning dynamic atoms is now slower, from +11% time for 8 byte strings to +41% time for 512 bytes. However this is doing nothing but string interning.
HTML parsing benchmarks
On servo/html5ever#813, which adds an HTML input file with all tag and attribute named mofidied to be dynamic atoms.
To use a patched string-cache, add to html5ever’s root
Cargo.toml:Full criterion output
String interning only happens during HTML deserialize, and
servo.org.htmlcontains ~no dynamic atoms so onlydeserialize "servo.org-custom.html"is relevant here. Once again I attribute other changes to thermal throttling.deserialize "servo.org-custom.html"shows not siginificant change, which I believe indicates that the performance regression on string interning is small when part of HTML parsing.