selectors: Get rid of hashing overhead using the precomputed hash atoms have. #16070

emilio · 2017-03-21T21:52:39Z

I realized of this when @bzbarsky mentioned the bloom filter in https://bugzilla.mozilla.org/show_bug.cgi?id=1348935#c7.

Right now we hash (the hash) all the time, when we can do better.

This requires a change in string-cache, which is at servo/string-cache#183.

This change is

highfive · 2017-03-21T21:52:42Z

Heads up! This PR modifies the following files:

@bholley: components/style/Cargo.toml, components/style/lib.rs, components/style/gecko_string_cache/mod.rs, components/style/gecko_string_cache/namespace.rs, components/style/matching.rs

highfive · 2017-03-21T21:52:43Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

emilio · 2017-03-21T21:53:18Z

@bholley: Feedback on this? I'm not too happy enforcing PrecomputedHash everywhere, but it can be made a feature in the future if it's inconvenient for some reason.

emilio · 2017-03-21T21:59:26Z

Inconvenient for rust-selectors downstream users, I mean, for us it's fine.

mbrubeck · 2017-03-21T22:13:19Z

.travis.yml

@@ -21,6 +21,7 @@ matrix:
         - bash etc/ci/check_no_panic.sh
         - bash etc/ci/lockfile_changed.sh
         - bash etc/ci/manifest_changed.sh
+         - (cd components/selectors && cargo test)


./mach cargo test -p selectors should work here

bholley · 2017-03-21T22:21:35Z

This approach seems fine. Thanks for fixing this emilio!

nox · 2017-03-25T10:33:26Z

components/selectors/parser.rs

@@ -1201,6 +1201,8 @@ pub mod tests {
        type PseudoElement = PseudoElement;
    }

+    type ShutUpTidy = String;


(Tidy complains when it sees &String as a function argument, even if in this case is unavoidable because it's generic code).

I see. You can probably do &<Self::Impl as SelectorImpl>::NamespacePrefix though.

bors-servo · 2017-04-05T09:38:25Z

☔ The latest upstream changes (presumably #16169) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2017-04-07T12:37:00Z

@bors-servo try

bors-servo · 2017-04-07T12:37:03Z

⌛ Trying commit 6551d04 with merge 6ca38d5...


        Auto merge of #16070 - emilio:selectors-bloom-hash-less, r=<try>

@bzbarsky

selectors: Get rid of hashing overhead using the precomputed hash atoms have. I realized of this when @bzbarsky mentioned the bloom filter in https://bugzilla.mozilla.org/show_bug.cgi?id=1348935#c7. Right now we hash (the hash) all the time, when we can do better. This requires a change in string-cache, which is at servo/string-cache#183.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16070)

bzbarsky · 2017-04-07T22:22:08Z

@emilio Is this good to go?

emilio · 2017-04-07T22:51:00Z

@bzbarsky it should, it's currently waiting for review, yeah. I wanted to wait for the try run to report before that, but bors decided not to publish test results.

Needs a squash, but happy to land this if somebody signs it off :)

emilio · 2017-04-07T22:51:04Z

@bors-servo try

bors-servo · 2017-04-07T22:51:09Z

⌛ Trying commit aae7984 with merge 1d3109b...


        Auto merge of #16070 - emilio:selectors-bloom-hash-less, r=<try>

@bzbarsky

selectors: Get rid of hashing overhead using the precomputed hash atoms have. I realized of this when @bzbarsky mentioned the bloom filter in https://bugzilla.mozilla.org/show_bug.cgi?id=1348935#c7. Right now we hash (the hash) all the time, when we can do better. This requires a change in string-cache, which is at servo/string-cache#183.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16070)

emilio · 2017-04-07T22:57:55Z

r? @bholley

Leaving it in Bobby's queue, but if someone wants to steal it, please do :)

bholley · 2017-04-07T22:58:33Z

@bors-servo delegate+

bors-servo · 2017-04-07T22:58:34Z

✌️ @emilio can now approve this pull request

bors-servo · 2017-04-07T23:21:06Z

💔 Test failed - linux-rel-wpt

emilio · 2017-04-07T23:58:13Z

@bors-servo r=bholley

That timeout looks unrelated.

bors-servo · 2017-04-07T23:58:14Z

📌 Commit aae7984 has been approved by bholley

bors-servo · 2017-04-07T23:58:16Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt...

emilio · 2017-04-07T23:59:16Z

@bors-servo r-

Whoops, the squash is missing though :)


        selectors: Unbust tests, and add a way to override the hash for the b…

…loom filter.


        style: Hash less stuff in the bloom filter, using the precomputed has…

…hes we have.

emilio · 2017-04-08T00:03:09Z

@bors-servo r=bholley

bors-servo · 2017-04-08T00:03:09Z

📌 Commit e29b84d has been approved by bholley

bors-servo · 2017-04-08T00:03:14Z

⌛ Testing commit e29b84d with merge a811776...


        Auto merge of #16070 - emilio:selectors-bloom-hash-less, r=bholley

@bzbarsky

selectors: Get rid of hashing overhead using the precomputed hash atoms have. I realized of this when @bzbarsky mentioned the bloom filter in https://bugzilla.mozilla.org/show_bug.cgi?id=1348935#c7. Right now we hash (the hash) all the time, when we can do better. This requires a change in string-cache, which is at servo/string-cache#183.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16070)

bors-servo · 2017-04-08T00:37:35Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: bholley
Pushing a811776 to master...

highfive assigned glennw Mar 21, 2017

highfive added the S-awaiting-review label Mar 21, 2017

mbrubeck reviewed Mar 21, 2017

View changes

nox reviewed Mar 25, 2017

View changes

nox added the S-blocked-on-external label Mar 26, 2017

bzbarsky mentioned this pull request Apr 4, 2017

Expose the precomputed hash using a trait so that I can use it from rust-selectors. servo/string-cache#183

Merged

highfive added the S-needs-rebase label Apr 5, 2017

emilio force-pushed the emilio:selectors-bloom-hash-less branch 2 times, most recently from 5880fe7 to 6551d04 Apr 7, 2017

emilio removed S-blocked-on-external S-needs-rebase labels Apr 7, 2017

highfive assigned bholley and unassigned glennw Apr 7, 2017

highfive added the S-tests-failed label Apr 7, 2017

highfive removed the S-awaiting-review label Apr 7, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 7, 2017

emilio added 2 commits Mar 21, 2017

selectors: Unbust tests, and add a way to override the hash for the b…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

65ebbb7

…loom filter.

style: Hash less stuff in the bloom filter, using the precomputed has…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

e29b84d

…hes we have.

emilio force-pushed the emilio:selectors-bloom-hash-less branch from aae7984 to e29b84d Apr 8, 2017

highfive added S-awaiting-review and removed S-awaiting-merge labels Apr 8, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 8, 2017

emilio mentioned this pull request Apr 8, 2017

bump required rayon version to 0.7 #16303

Merged

2 of 2 tasks complete

highfive removed the S-awaiting-merge label Apr 8, 2017

servo / servo

selectors: Get rid of hashing overhead using the precomputed hash atoms have. #16070

selectors: Get rid of hashing overhead using the precomputed hash atoms have. #16070

emilio commented Mar 21, 2017 •

edited by larsbergstrom

highfive commented Mar 21, 2017

highfive commented Mar 21, 2017

emilio commented Mar 21, 2017

emilio commented Mar 21, 2017

This comment has been minimized.

bholley commented Mar 21, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Apr 5, 2017

emilio commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bzbarsky commented Apr 7, 2017

emilio commented Apr 7, 2017

emilio commented Apr 7, 2017

bors-servo commented Apr 7, 2017

emilio commented Apr 7, 2017

bholley commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bors-servo commented Apr 7, 2017

emilio commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bors-servo commented Apr 7, 2017

emilio commented Apr 7, 2017

emilio commented Apr 8, 2017

bors-servo commented Apr 8, 2017

bors-servo commented Apr 8, 2017

bors-servo commented Apr 8, 2017

servo / servo

Join GitHub today

selectors: Get rid of hashing overhead using the precomputed hash atoms have. #16070

selectors: Get rid of hashing overhead using the precomputed hash atoms have. #16070

Conversation

emilio commented Mar 21, 2017 • edited by larsbergstrom

highfive commented Mar 21, 2017

highfive commented Mar 21, 2017

emilio commented Mar 21, 2017

emilio commented Mar 21, 2017

This comment has been minimized.

mbrubeck Mar 21, 2017

bholley commented Mar 21, 2017

This comment has been minimized.

nox Mar 25, 2017

This comment has been minimized.

emilio Mar 25, 2017

This comment has been minimized.

nox Mar 25, 2017

bors-servo commented Apr 5, 2017

emilio commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bzbarsky commented Apr 7, 2017

emilio commented Apr 7, 2017

emilio commented Apr 7, 2017

bors-servo commented Apr 7, 2017

emilio commented Apr 7, 2017

bholley commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bors-servo commented Apr 7, 2017

emilio commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bors-servo commented Apr 7, 2017

emilio commented Apr 7, 2017

emilio commented Apr 8, 2017

bors-servo commented Apr 8, 2017

bors-servo commented Apr 8, 2017

bors-servo commented Apr 8, 2017

emilio commented Mar 21, 2017 •

edited by larsbergstrom