Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider storing each DOM node's list of children as a vec in the node #25206

Open
nox opened this issue Dec 7, 2019 · 9 comments
Open

Consider storing each DOM node's list of children as a vec in the node #25206

nox opened this issue Dec 7, 2019 · 9 comments

Comments

@nox
Copy link
Member

@nox nox commented Dec 7, 2019

When I implemented NodeList years ago, I wrote a complicated caching system so that iterating from start to finish on the list of children wouldn't start again from the node's first child.

On the other side of the pond, in Firefox, we just store a vec of nodes and access that in O(1). Furthermore, that vec is used in unrelated places too, even in layout.

If we did this in Servo, this could simplify parallel box construction a lot, and given Firefox does it, I'm not sure it's a huge performance hit.

Cc @jdm @SimonSapin

@nox
Copy link
Member Author

@nox nox commented Dec 7, 2019

(Note that I suggest we do that in addition of the first_child, last_child pointers etc, not as a replacement.)

@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Dec 13, 2019

I think if it's good enough for Firefox, it's good enough for us.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Dec 13, 2019

In that case will we still need the first_child last_child, children_count fields?

At a first glance it does seem to me that we can just switch over. It's not clear to me why we need NodeList to have both a Vec and a magical node-based form.

@nox
Copy link
Member Author

@nox nox commented Dec 13, 2019

Which fields are you talking about? Node's?

NodeListType::Simple is for static collections of nodes that aren't specific to anything, NodeListType::Children is for live collections of children, I was not your colleague yet and I decided to go fancy.

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Dec 13, 2019

nextSibling etc are exposed to the DOM. If we only had children: Vec<Node> they would take O(n) time by linear-searching in the parent’s children.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Dec 13, 2019

@nox yeah i guess i mean that we're using NodeList to store children but we use NodeList::Children instead of just a vec. We should just use a vec directly, and have NodeList::Children read from that vec.

nextSibling etc are exposed to the DOM. If we only had children: Vec<Node> they would take O(n) time by linear-searching in the parent’s children.

Wasn't talking about nextSibling :) Some of these are O(1)

@nox
Copy link
Member Author

@nox nox commented Dec 13, 2019

@nox yeah i guess i mean that we're using NodeList to store children but we use NodeList::Children instead of just a vec. We should just use a vec directly, and have NodeList::Children read from that vec.

Yeah, 2015 nox didn't want to allocate a vec when he could write some cute code instead. I optimised for the case where you just traverse .childNodes with a for loop, and not have to patch up the vec every time the children change.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Dec 13, 2019

We could still use that iterator for ChildNodes, actually, if it's a live iterator.

@nox
Copy link
Member Author

@nox nox commented Dec 13, 2019

If we always maintained that vector of children for each node, style::parallel::traverse_nodes could just use rayon's par_iter method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.