Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upstyle: Avoid quadratic time serialization of a declaration block. #17315
Conversation
At least when the longhands aren't custom properties. We should also look into not serializing the style attribute eagerly when it's not needed... But a lot of code currently rely on attribute values being dereferenciables to &str, so that's harder to fix. We should really look into all those vectors around too, but that's probably less urgent.
highfive
commented
Jun 14, 2017
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jun 14, 2017
|
|
|
This should fix most of the perf cliffs seen in #17308. r? @SimonSapin (or anyone else) |
|
cc @glennw |
|
This shaves a good amount of time in the following benchmark (~1s to ~600ms): <!doctype html>
<script>
var div = document.createElement('div');
div.style.border = "10px solid red";
div.style.position = "absolute";
div.style.top = "10px";
div.style.left = "100px";
div.style.width = "100px";
div.style.height = "100px";
var start = new Date();
for (var i = 0; i < 100000; ++i)
div.style.cssText;
document.write(new Date() - start);
</script>But we should aim to be at the ~300/~400ms boundary. |
|
Latest commit puts us at 430ms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89803b4a80a7093c29443cdbaa96a9b09f1af4df |
|
Gah, bad timing pushing to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e59616696ec527441273f8e9afa92990d56fe59 |
…tion block. Concretely we avoid allocating and scanning a temporary vector of longhands not yet serialized for each shorthand. This doesn't save a lot of time in Linux, but I bet it's somewhat important on OSX.
|
This is all green :) |
|
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3. components/style/properties/properties.mako.rs, line 1342 at r3 (raw file):
Can this whole method be replaced with Comments from Reviewable |
By looking at the shorthands, not the other way around, given a longhand uses to not appear in multiple longhands. This makes the testcase go to 430ms on my machine. This could be even faster having a static LonghandIdSet per shorthand, I guess, but this is not done in this PR.
Not literally, but yes if you match on |
|
Looks good! @bors-servo r+ Reviewed 1 of 1 files at r4. Comments from Reviewable |
|
|
style: Avoid quadratic time serialization of a declaration block. At least when the longhands aren't custom properties. We should also look into not serializing the style attribute eagerly when it's not needed... But a lot of code currently rely on attribute values being dereferenciables to &str, so that's harder to fix. We should really look into all those vectors around too, but that's probably less urgent. <!-- 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/17315) <!-- Reviewable:end -->
|
|
emilio commentedJun 14, 2017
•
edited by larsbergstrom
At least when the longhands aren't custom properties.
We should also look into not serializing the style attribute eagerly when it's
not needed... But a lot of code currently rely on attribute values being
dereferenciables to &str, so that's harder to fix.
We should really look into all those vectors around too, but that's probably
less urgent.
This change is