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 upCheck for an extant attribute before interning in setAttribute #8645
Conversation
|
I'm afraid this is wrong. <!DOCTYPE html>
<div>...</div>
<script>
var e = document.body;
e.setAttributeNS(null, "FOO", "bar");
console.log(e.getAttributeNS(null, "FOO"));
console.log(e.getAttributeNS(null, "foo"));
e.setAttribute("FOO", "quux");
console.log(e.getAttributeNS(null, "FOO"));
console.log(e.getAttributeNS(null, "foo"));
window.close()
</script>currently correctly logs and with your change logs |
|
I'm also not convinced I want to slow down all the |
|
The |
Yeah, I was just about to comment when I realized it was already there. Adding the namespace check and reverting the I didn't see any wpt test failures, are there other tests that would have caught this? |
|
The relevant tests are in tests/wpt/web-platform-tests/dom/nodes/attributes.html ; there might not be a test for the particular bug you introduced. |
fitzgen
force-pushed the
fitzgen:check-if-attribute-exists-before-interning
branch
from
2bcfb19
to
606d9bf
|
Fixed it up, added the test case from #8645 (comment) to Benchmark numbers are about the same. See the commit message. r? @Ms2ger |
|
|
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> ```
fitzgen
force-pushed the
fitzgen:check-if-attribute-exists-before-interning
branch
from
606d9bf
to
4a42093
Rebased and fixed. |
|
@Ms2ger review ping |
|
@Ms2ger review ping Also going to ping @asajeffrey since this is related to strings |
|
I need to try to prove this correct; feel free to help with that :) |
|
Also, I'm about to commit a patch which makes Atom -> DOMString -> Atom essentially free. This may impact the performance results. |
|
|
|
Closed for the same reason as #8668. |
fitzgen commentedNov 22, 2015
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:
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: