Last Comment Bug 705280 - document.head is slower than other methods to access the head element
: document.head is slower than other methods to access the head element
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://jsperf.com/document-head
Depends on: 706131
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-25 07:01 PST by Anthony Ricaud (:rik)
Modified: 2012-02-01 13:56 PST (History)
9 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.48 KB, patch)
2011-11-25 07:42 PST, Mounir Lamouri (:mounir)
mrbkap: review+
mounir: checkin+
Details | Diff | Review
Custom quickstub (3.35 KB, patch)
2011-11-28 11:26 PST, Boris Zbarsky [:bz]
mrbkap: review+
mounir: checkin+
Details | Diff | Review
Quickstub nsIDOMHTMLDocument.head (960 bytes, patch)
2011-11-29 02:32 PST, Mounir Lamouri (:mounir)
mrbkap: review+
mounir: checkin+
Details | Diff | Review

Description Anthony Ricaud (:rik) 2011-11-25 07:01:40 PST
In the attached testcase, document.head is the slowest method to access the head element.

On my machine, with a 2011-11-24 nightly, I get :
- 720,647 ops/sec for document.head
- 4,306,393 ops/sec for document.getElementsByTagName('head')[0];
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-11-25 07:06:42 PST
Well, we don't quickstub it, so that's not that surprising.
Comment 2 Mounir Lamouri (:mounir) 2011-11-25 07:11:52 PST
I have a patch compiling with document.head quickstubbed.
Comment 3 Mounir Lamouri (:mounir) 2011-11-25 07:12:23 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> I have a patch compiling with document.head quickstubbed.

Actually, nsIDOMHTMLDocument.*
Comment 4 Mounir Lamouri (:mounir) 2011-11-25 07:42:46 PST
Created attachment 576932 [details] [diff] [review]
Patch v1
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-11-25 08:02:14 PST
Comment on attachment 576932 [details] [diff] [review]
Patch v1

Looks good to me.
Comment 6 Ed Morley [:emorley] 2011-11-26 00:29:01 PST
https://hg.mozilla.org/mozilla-central/rev/2040980c0792
Comment 7 Marco Bonardo [::mak] 2011-11-26 02:05:37 PST
According to tree-management and graphs-new this caused a Dromaeo(V8) Regression of 4%.
http://graphs-new.mozilla.org/graph.html#tests=[[76,63,22],[76,63,21],[76,63,17]]&sel=1322137898241.9265,1322301874142&displayrange=7&datatype=running
Comment 8 Marco Bonardo [::mak] 2011-11-26 02:22:40 PST
Original
NOISE: |0;crypto.html;333.22;333.22;333.22;333.22;333.22
NOISE: |1;deltablue.html;909.20;909.20;909.20;909.20;909.20
NOISE: |2;earley-boyer.html;73.16;73.16;73.16;73.16;73.16
NOISE: |3;raytrace.html;33.45;33.45;33.45;33.45;33.45
NOISE: |4;richards.html;1421.40;1421.40;1421.40;1421.40;1421.40

Regressed
NOISE: |0;crypto.html;333.65;333.65;333.65;333.65;333.65
NOISE: |1;deltablue.html;829.00;829.00;829.00;829.00;829.00
NOISE: |2;earley-boyer.html;73.68;73.68;73.68;73.68;73.68
NOISE: |3;raytrace.html;32.93;32.93;32.93;32.93;32.93
NOISE: |4;richards.html;1418.00;1418.00;1418.00;1418.00;1418.00
Comment 9 Boris Zbarsky [:bz] 2011-11-26 12:17:31 PST
So mostly deltablue?

Mounir, can you reproduce that locally?
Comment 10 Mounir Lamouri (:mounir) 2011-11-27 06:30:11 PST
(In reply to Boris Zbarsky (:bz) from comment #9)
> So mostly deltablue?
> 
> Mounir, can you reproduce that locally?

So I spent some time yesterday trying to reproduce this and I wasn't able to do it locally with a release build [1], the version with the patch is faster (or the same) the previous version. Though, I didn't use talos locally to try that.

So I tried to only quickstub .head [2] and quickstub all properties individually [3] and the results where quite variable: some results are faster, some slower and some pretty much the same (compared to a version with this patch). I wonder if such variation is expected from talos on try but it's hard to analyze anything in those conditions :-/

[1] Using dromaeo v8 tests from our website.
[2] https://tbpl.mozilla.org/?tree=Try&rev=002f14c699f2
[3] https://tbpl.mozilla.org/?tree=Try&rev=5c6e2f3a2e6c
Comment 11 Boris Zbarsky [:bz] 2011-11-27 07:01:45 PST
The thing that confuses me, by the way, is that fundamentally the patche in this should not affect Dromaeo V8 at all.
Comment 12 Boris Zbarsky [:bz] 2011-11-27 07:14:10 PST
By the way... the patch helps, but it would help more if you used a custom quickstub combined with a non-addreffing version of GetHead.  Followup bug on that?
Comment 13 Mounir Lamouri (:mounir) 2011-11-27 07:18:03 PST
(In reply to Boris Zbarsky (:bz) from comment #11)
> The thing that confuses me, by the way, is that fundamentally the patche in
> this should not affect Dromaeo V8 at all.

I definitely agree: for what I understand, it should not affect the test. 

I see Bobby pushed a lot of patches just after mine and the talos result after mine is the day after which means no test where explicitly run for this push. Is it possible that the talos run was actually using bobby's changes and did report wrong a wrong changesets list?
Maybe it would be worth backouting my patch just to check what's happening.
Comment 14 Mounir Lamouri (:mounir) 2011-11-27 08:00:04 PST
I pushed a backout to mozilla-inbound.
Comment 15 Ed Morley [:emorley] 2011-11-27 08:50:29 PST
Reopening since now backed out.
Comment 16 Boris Zbarsky [:bz] 2011-11-27 10:57:30 PST
For what it's worth, if this backout doesn't send the numbers back to normal, xpconnect gunk (which is what I assume bholley pushed) would be a good next candidate to try.
Comment 17 Marco Bonardo [::mak] 2011-11-28 02:05:28 PST
The backout restored the values to the previous level.
Comment 18 Boris Zbarsky [:bz] 2011-11-28 11:26:35 PST
Created attachment 577317 [details] [diff] [review]
Custom quickstub

For what it's worth, in addition to the manifest change it would be good to do this.
Comment 19 Mounir Lamouri (:mounir) 2011-11-29 02:31:45 PST
I talked with mrbkap yesterday and according to him, one think that might happen is that it takes more time to initialize the list of nsIDOMHTMLDocument quickstubbed properties because the list is bigger. Why it only affects dromaeo v8, he doesn't know.

Anyhow, it should be fixed by the new dom bindings very likely so I'm going to put a patch quickstubbing document.head and open a follow-up, hoping it will be closed by the new dom bindings soon ;)
Comment 20 Mounir Lamouri (:mounir) 2011-11-29 02:32:23 PST
Created attachment 577533 [details] [diff] [review]
Quickstub nsIDOMHTMLDocument.head
Comment 21 Marco Bonardo [::mak] 2011-11-29 03:17:55 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #19)
> Anyhow, it should be fixed by the new dom bindings very likely so I'm going
> to put a patch quickstubbing document.head and open a follow-up, hoping it
> will be closed by the new dom bindings soon ;)

Just to clarify, does this mean an eventual regression on next push should be ignored?
Comment 22 Mounir Lamouri (:mounir) 2011-11-29 03:19:15 PST
(In reply to Marco Bonardo [:mak] from comment #21)
> Just to clarify, does this mean an eventual regression on next push should
> be ignored?

No, that means the next push shouldn't have any regression.

Note You need to log in before you can comment on or make changes to this bug.