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 upAvoid re-validating the same attribute name multiple times #8668
Conversation
Interning a DOMString as an atom can be expensive, and that work is unnecessary if the attribute being added already exists and already has an interned name. Instead of eagerly interning, first do a linear search over the existing attributes to see if we can skip interning. This is an optimization that Gecko already employs, as described in #6906. I tested Dromaeo's dom subsuite and a super naive micro benchmark of my own writing. I haven't looked at Dromaeo's code, but given the units it reports, I'm assuming that higher is better. For my super naive micro benchmark, smaller is better. Here are the results: +---------+-------------------------+----------------------+----------------+ | | Servo w/out this commit | Servo w/ this commit | Gecko | +---------+-------------------------+----------------------+----------------+ | Dromaeo | 2684.37 runs/s | 2771.50 runs/s | 2068.78 runs/s | +---------+-------------------------+----------------------+----------------+ | Micro | 4985 ms | 4606 ms | 1616 ms | +---------+-------------------------+----------------------+----------------+ So this seems to yield a slight performance increase, but nothing to write home about. This could potentially hurt performance when there are many attributes or if the attribute names are very long; intuitively, neither seem super common to me. Here is the source of my super naive micro benchmark: ```html <!DOCTYPE html> <body> <button onclick="run()">Test</button> <h1 id="result"></h1> <script> function run() { var h1 = document.getElementById("result"); var start = Date.now(); var i = 10000000; while (i--) { h1.setAttribute("foobar", "baz"); } h1.textContent = Date.now() - start; } </script> </body> ```
If the name given to setAttribute names an attribute that already exists, then it must have already been validated as a valid attribute name. In this case, we can skip validation, which shows up fairly heavily on profiles.
|
I agree that since the performance numbers aren't great, this may not be worth merging. |
|
|
|
Given the discussion linked in #6906 (comment), it seems postponing the validity check of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
fitzgen commentedNov 24, 2015
If the name given to setAttribute names an attribute that already exists, then
it must have already been validated as a valid attribute name. In this case, we
can skip validation, which shows up fairly heavily on profiles.
Dromaeo DOM attributes is largely unchanged: From 2721.35 runs/s to 2727.40 runs/s.
setAttributein a loop (micro benchmark from #8645) gets a little bump: From 4540 ms to 4400 ms.Depends on #8645.
Not 100% clear this is a valid optimization, see #6906 (comment).
Given that, and the relatively lackluster speedups, this may not be worth merging.