Skip to content
This repository was archived by the owner on May 28, 2024. It is now read-only.

Remove string cache and some traits #100

Merged

Conversation

SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Oct 26, 2016

This change is Reviewable

@SimonSapin SimonSapin force-pushed the remove-string-cache-and-some-traits-and-split-bloom branch from 4034d93 to 98f0a16 Compare October 26, 2016 17:40
@SimonSapin
Copy link
Member Author

r? @nox

@SimonSapin SimonSapin force-pushed the remove-string-cache-and-some-traits-and-split-bloom branch from 98f0a16 to 7346186 Compare October 26, 2016 22:25
@SimonSapin SimonSapin changed the title Remove string cache and some traits and split bloom Remove string cache and some traits Oct 26, 2016
@SimonSapin
Copy link
Member Author

At first I split the bloom filter into its own crate so that implementing BloomHash requires a smaller dependency, but instead I removed the trait and use std::hash::Hash with a FNV hasher instead.

My guess is that hashing will be slightly more expensive than before, but we might get fewer hash collisions. (The previous BloomHash for Atom would just XOR the two halves of the u64 inside Atom, whose bit pattern is very much not random.) I tried both in Servo with ./mach run -r https://en.wikipedia.org/wiki/Servomechanism -p -x and did not see a significant difference either way in "Style Recalc" times.

This PR also weakens a test that check the number of false positives in a bloom filter. I believe this test only passed before because it hashes consecutive integers, which the previous BloomHash for usize would hash to themselves with no collision at all. In practice the bloom filter is used with atoms, not consecutive integers.

@nox
Copy link

nox commented Oct 27, 2016

@bors-servo r+

@bors-servo
Copy link

📌 Commit 7346186 has been approved by nox

@bors-servo
Copy link

⌛ Testing commit 7346186 with merge e50ee6a...

bors-servo pushed a commit that referenced this pull request Oct 27, 2016
…lit-bloom, r=nox

Remove string cache and some traits

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-selectors/100)
<!-- Reviewable:end -->
@bors-servo
Copy link

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit 7346186 into master Oct 27, 2016
@SimonSapin SimonSapin deleted the remove-string-cache-and-some-traits-and-split-bloom branch October 27, 2016 07:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants