Optimize selector matching for some common cases #19781
Conversation
|
Heads up! This PR modifies the following files:
|
highfive
added
the
S-awaiting-review
|
|
|
It shows 3% improvement for dromaeo_css on Linux and macOS. It doesn't show a certain result for Windows platform on its overall score, but it still shows significant improvement on subtest dojo.html and prototype.html. Mysteriously, subtest ext.html, mootools.html, and yui.html shows significant regression on win64, but those tests don't use qS or qSA at all... PGO is mysterious... In addition, this change also improves performance on bug 1422522. On my machine, the time is pulled from ~2.6s down to ~2.2s, probably mostly benefits from the inlining of the common cases. |
|
From that try run, it seems that we would almost close the gap for the regression of bug 1414789 with this patch. |
|
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29affb31a7bcb1d78bfab1d686aa0644e8f51b1e (in case I did something wrong) |
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
Looks good, as unfortunate as it is. r=me when you want.
| F: FnMut(&E, ElementSelectorFlags), | ||
| { | ||
| let matches_hover_and_active_quirk = | ||
| matches_hover_and_active_quirk(&selector_iter, context, rightmost); |
There was a problem hiding this comment.
Looks like this could be moved below.
There was a problem hiding this comment.
No, because it wants the original selector_iter. Alternatively we can clone the iter at the beginning and pass it into the function later, but I don't think it's worth.
| let matches_hover_and_active_quirk = | ||
| matches_hover_and_active_quirk(&selector_iter, context, rightmost); | ||
|
|
||
| // Handle some common cases first. |
There was a problem hiding this comment.
It's a bit unfortunate that we need to do this indeed.
Mind leaving a comment like:
// TODO(emilio): It'd be nice to just get rid of this, if we make Talos happy / the generic case fast enough.I've seen similar wins today just from enabling stylo-chrome and such (https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=aea8893308877a9139af703db9d5abcc822dddd5&newProject=try&newRevision=e4667f74741b200ae5cc687ce7fb37697b7f394f&framework=1&showOnlyImportant=1). I'm not sure how you feel about landing this vs. waiting. Both look fine to me.
There was a problem hiding this comment.
The big match needs dispatch anyway, and that may be less friendly to all the predicator and jump cache, so I think unrolling some common cases into a sequential if-chain is a reasonable optimization before rust compiler can do pto.
There was a problem hiding this comment.
And, it seems to me that commit is just from #19774 plus an #[inline(never)] on the helper function. I don't see how that's related to stylo-chrome.
|
@bors-servo r=emilio |
|
📌 Commit d0fd922 has been approved by |
highfive
added
S-awaiting-merge
Optimize selector matching for some common cases This is the "better way" I mentioned in #19774, which seems to actually improve the score of dromaeo_css on talos. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19781) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
highfive
removed
the
S-awaiting-merge
5 participants
This is the "better way" I mentioned in #19774, which seems to actually improve the score of dromaeo_css on talos.
This change is