Versioned dom #8227

asajeffrey · 2015-10-27T20:47:49Z

This PR adds versioning to the DOM. There are now node.get_version and node.get_descendent_version methods that return a counter that is bumped when the node is dirtied. This is used to implement cache invalidation for caching HTMLCollection state. Caching HTMCollections gets a 1000x speedup in the Dromaeo DOM query tests.

Addresses #6901, #3381 and #1916.

Replaces PR #6927.

asajeffrey · 2015-10-27T20:48:41Z

The PR in its current state needs squashing, which I'll do if it passes review.

eefriedman · 2015-10-27T21:28:04Z

More comments would be nice.

Review status: 0 of 6 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

^{components/script/dom/document.rs, line 1214 [r1] (raw file):}
Maybe tag_map?

^{components/script/dom/document.rs, line 1428 [r1] (raw file):}
Consider using HashMap::entry instead of get. (Also in other places.)

^{components/script/dom/document.rs, line 1433 [r1] (raw file):}
Whitespace.

^{components/script/dom/document.rs, line 1452 [r1] (raw file):}
Whitespace.

^{components/script/dom/htmlcollection.rs, line 30 [r1] (raw file):}
Consider using u64 for version? It's easy to overflow a u32 on a modern processor.

^{components/script/dom/htmlcollection.rs, line 171 [r1] (raw file):}
Whitespace.

^{components/script/dom/htmlcollection.rs, line 282 [r1] (raw file):}
Whitespace.

Comments from the review on Reviewable.io

asajeffrey · 2015-10-27T23:04:35Z

Thanks! I'll have a look at your review comments tomorrow. Also get test-tidy to pass, sigh.

asajeffrey · 2015-10-28T01:03:06Z

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/document.rs, line 1214 [r1] (raw file):}
The name was just for consistency with idmap. I'm happy to change all of them.

^{components/script/dom/document.rs, line 1428 [r1] (raw file):}
Good point. I'm still learning my way round the rust stdlib.

^{components/script/dom/htmlcollection.rs, line 30 [r1] (raw file):}
Tradeoff of space vs correctness? The u32 is only a problem if there's an overflow and a version comparison with exactly the right old version. The u64 uses an extra word per DOM node.

Comments from the review on Reviewable.io

eefriedman · 2015-10-28T01:10:34Z

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlcollection.rs, line 30 [r1] (raw file):}
I'm not convinced saving a word is worth compromising correctness... but if you really want it to wrap, at least use Wrapping<u32> so it doesn't panic in overflow-checked builds.

Comments from the review on Reviewable.io

asajeffrey · 2015-10-28T12:22:16Z

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlcollection.rs, line 30 [r1] (raw file):}
On reflection, I think the space saving probably isn't worth it, I'll change this to u64.

Comments from the review on Reviewable.io

asajeffrey · 2015-10-28T13:52:25Z

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/document.rs, line 1428 [r1] (raw file):}
On looking at the code again, I don't think I can use HashMap::entry, because of rooting.

Comments from the review on Reviewable.io

eefriedman · 2015-10-28T18:10:10Z

Because of rooting? Hmm... that sounds like we just need to update the whitelist in the lint plugin; std::collections::hash_map::Entry should always be fine to store on the stack. (https://github.com/servo/servo/blob/master/components/plugins/lints/unrooted_must_root.rs#L53 .)

asajeffrey · 2015-10-28T18:23:14Z

The problem isn't that Entry needs rooted, it's that the code would end up being something like:

self.foo_map.entry(key).or_insert(|| mk_foo().r() ).root()

that is, in the case of a cache miss, we build a rooted result, then unroot it, then root it again.

This is because the cache is being kept unrooted: should the cache be the rooted versions?

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

eefriedman · 2015-10-28T18:30:50Z

That doesn't seem like a big deal; rooting isn't an expensive operation.

If you really want to avoid it, you can do match foo_map.entry(key) { VacantEntry(e) => {}...

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

metajack · 2015-10-28T18:32:01Z

I believe that @asajeffrey has shown that rooting is expensive. It's certianly showing up in his profiles from what I understand.

asajeffrey · 2015-10-28T18:48:20Z

The problem with rooting is that it uses a Vec for roots, so the code includes the case where the vector has to grow, which means problems for code inlining. It's more of a code size and optimization problem than a time problem, since in the common case rooting is just a vector push into a vector which already has enough backing store.

I'll update to using HashMap::entry.

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

tschneidereit · 2015-10-30T12:33:16Z

@bors-servo: try

Manishearth · 2015-10-30T12:49:20Z

@bors-servo: try

bors-servo · 2015-10-30T12:49:24Z

⌛ Trying commit 89248e8 with merge 4554d8a...

Versioned dom This PR adds versioning to the DOM. There are now node.get_version and node.get_descendent_version methods that return a counter that is bumped when the node is dirtied. This is used to implement cache invalidation for caching HTMLCollection state. Caching HTMCollections gets a 1000x speedup in the Dromaeo DOM query tests. Addresses #6901, #3381 and #1916. Replaces PR #6927.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8227)

bors-servo · 2015-10-30T13:01:33Z

💔 Test failed - linux-rel

Manishearth · 2015-10-30T13:01:46Z

(Artificial failure)

bors-servo · 2015-10-31T13:28:05Z

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

asajeffrey · 2015-11-02T17:02:49Z

I responded to @eefriedman and rebased to builld with master. Anything else?

jdm · 2015-11-02T17:05:20Z

@eefriedman Are you doing a full review of this, or just drive-by? Please set the assignee field for the former, so it's clear who's responsible for the next step here.

eefriedman · 2015-11-06T20:07:17Z

I'll try to merge #8315, and then you can rebase.

asajeffrey · 2015-11-06T23:09:28Z

Thanks! I'll rebase... Grumble mutter dependent PRs.

There is now an inclusive_descendants_version field of each node, which increases each time the node, or any of its descendants, is dirtied. This can be used for cache invalidation, by caching a version number and comparting the current version number against the cached version number.

We cache the state of any live HTMLCollection, keeping track of a) the optional cached length of the collection, and b) an optional cursor into the collection (a node in the collection plus its index). The cache is invalidated based on the version number of the node. We use these caches for speeding up random access to the collection. When returning coll[i], we search from the cursor, if it exists, and otherwise search from the front of the collection. In particular, both a forward for-loop and a backward for-loop through the collection will now have each access take O(1) time rather than O(n) time. This gets 1000x speed-up on the relevant Dromaeo DOM query tests.

eefriedman · 2015-11-09T19:06:51Z

@bors-servo r+

bors-servo · 2015-11-09T19:06:52Z

📌 Commit 237ddc3 has been approved by eefriedman

bors-servo · 2015-11-09T19:06:54Z

⌛ Testing commit 237ddc3 with merge 9dbd5dd...

Versioned dom This PR adds versioning to the DOM. There are now node.get_version and node.get_descendent_version methods that return a counter that is bumped when the node is dirtied. This is used to implement cache invalidation for caching HTMLCollection state. Caching HTMCollections gets a 1000x speedup in the Dromaeo DOM query tests. Addresses #6901, #3381 and #1916. Replaces PR #6927.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8227)

bors-servo · 2015-11-09T19:21:09Z

💔 Test failed - gonk

eefriedman · 2015-11-09T19:36:18Z

@bors-servo retry #8416

bors-servo · 2015-11-09T19:41:24Z

⌛ Testing commit 237ddc3 with merge f39faaf...

Versioned dom This PR adds versioning to the DOM. There are now node.get_version and node.get_descendent_version methods that return a counter that is bumped when the node is dirtied. This is used to implement cache invalidation for caching HTMLCollection state. Caching HTMCollections gets a 1000x speedup in the Dromaeo DOM query tests. Addresses #6901, #3381 and #1916. Replaces PR #6927.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8227)

bors-servo · 2015-11-09T20:23:04Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

Fixes servo#6942.

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 27, 2015

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 30, 2015

highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 31, 2015

asajeffrey force-pushed the versioned_dom branch from 89248e8 to 854675e Compare October 31, 2015 16:32

highfive removed the S-tests-failed The changes caused existing tests to fail. label Oct 31, 2015

jdm removed the S-needs-rebase There are merge conflict errors. label Nov 2, 2015

eefriedman self-assigned this Nov 2, 2015

Alan Jeffrey added 3 commits November 6, 2015 17:23

Made QualName traceable by JS.

4aa6a76

asajeffrey force-pushed the versioned_dom branch from 3e4cc15 to 237ddc3 Compare November 6, 2015 23:28

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 6, 2015

highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 9, 2015

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 9, 2015

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 9, 2015

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 9, 2015

bors-servo merged commit 237ddc3 into servo:master Nov 9, 2015

bors-servo mentioned this pull request Nov 9, 2015

Implement attribute restyle hints #8381

Merged

bholley referenced this pull request in bholley/servo Nov 10, 2015

Implement attribute restyle hints.

f6a87dc

Fixes servo#6942.

eefriedman mentioned this pull request Nov 24, 2015

setAttribute() seems way too slow #6906

Open

This was referenced Nov 29, 2015

HTMLCollection::create should cache results #6901

Closed

HTMLCollection::Item() is very slow on large pages. #3381

Closed

Have live collections being updated by signals, not by traversing node tree #1916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Versioned dom #8227

Versioned dom #8227

asajeffrey commented Oct 27, 2015

asajeffrey commented Oct 27, 2015

eefriedman commented Oct 27, 2015

asajeffrey commented Oct 27, 2015

asajeffrey commented Oct 28, 2015

eefriedman commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

eefriedman commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

eefriedman commented Oct 28, 2015

metajack commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

tschneidereit commented Oct 30, 2015

Manishearth commented Oct 30, 2015

bors-servo commented Oct 30, 2015

bors-servo commented Oct 30, 2015

Manishearth commented Oct 30, 2015

bors-servo commented Oct 31, 2015

asajeffrey commented Nov 2, 2015

jdm commented Nov 2, 2015

eefriedman commented Nov 6, 2015

asajeffrey commented Nov 6, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

Versioned dom #8227

Versioned dom #8227

Conversation

asajeffrey commented Oct 27, 2015

asajeffrey commented Oct 27, 2015

eefriedman commented Oct 27, 2015

asajeffrey commented Oct 27, 2015

asajeffrey commented Oct 28, 2015

eefriedman commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

eefriedman commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

eefriedman commented Oct 28, 2015

metajack commented Oct 28, 2015

asajeffrey commented Oct 28, 2015

tschneidereit commented Oct 30, 2015

Manishearth commented Oct 30, 2015

bors-servo commented Oct 30, 2015

bors-servo commented Oct 30, 2015

Manishearth commented Oct 30, 2015

bors-servo commented Oct 31, 2015

asajeffrey commented Nov 2, 2015

jdm commented Nov 2, 2015

eefriedman commented Nov 6, 2015

asajeffrey commented Nov 6, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015