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
Replaces rust-wapcaplet with a new interning module in Rust, and supports interning on CSS selector matching #1135
Conversation
…ash_match using new interning library written in Rust
I only briefly looked at this, but let's hold off on merging until the new rendering lands. |
Also only a brief look, but here are a few comments:
|
I also had the same reaction to |
Ditto for the hash table itself.
I want to hear more about this too. I think it could be done safely with the right atomic intrinsics, but that will be tricky, and also suggests further that this should become a generic Relatedly I'm not sure about the design of having each script task invoke Also |
Thanks everyone for the comments.
The interner is using a global variable with unsafe blocks. There are some design issues that I need to decide before we could implement string interning. To implement interning properly, hubbub and css parser need to share the interning table, but with the current design, this is not easy. For that, I would like to have your opinions on how to do this properly.
To guarantee the probability of hash collision, I implemented a universal hash for the interning, and customized it for u8 string hash. From my toy experiment, that boosted the hash performance by about 3 times on average. |
In my opinion, a new hash table implementation should be a separate patch from string interning. |
@sanxiyn, I think it’s fine to have the hashing algorithm, hash table and string interning in the same patch, especially if the former is only used by the latter. I’d just rather have them in different source files. However, is it really useful to implement a new hashing algorithm rather than using @ryanhc, I think we can have useful string interning without integrating with hubbub. We can intern stuff when creating the DOM nodes, hubbub doesn’t need to know about it. In the current patch, the interner is in a single |
@@ -210,7 +210,7 @@ impl Document { | |||
} | |||
|
|||
pub fn GetElementsByTagName(&self, tag: &DOMString) -> @mut HTMLCollection { | |||
self.createHTMLCollection(|elem| eq_slice(elem.tag_name, null_str_as_empty(tag))) | |||
self.createHTMLCollection(|elem| elem.tag_name.eq(&intern_string(null_str_as_empty(tag)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably should not intern tag
. elem.tag_name
can be converted to string slice instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that createHTMLCollection
loops over the entire tree, I think interning probably makes a lot of sense, but not every time the callback is called.
I think this will need the concurrent hash map, which I have written but is blocked on a Rust upgrade. |
Expand the test for document.embeds/plugins; r=Manishearth
Replaces current rust-wapcaplet with a new interning module written in Rust.
Supports interning on tag, id, class, attr_exists, attr_equal, attr_dash_match.