The default bug view has changed. See this FAQ.

document.head is slower than other methods to access the head element

RESOLVED FIXED in mozilla11

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rik, Assigned: mounir)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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];
Well, we don't quickstub it, so that's not that surprising.
(Assignee)

Comment 2

5 years ago
I have a patch compiling with document.head quickstubbed.
Assignee: nobody → mounir
(Assignee)

Comment 3

5 years ago
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> I have a patch compiling with document.head quickstubbed.

Actually, nsIDOMHTMLDocument.*
(Assignee)

Comment 4

5 years ago
Created attachment 576932 [details] [diff] [review]
Patch v1
Attachment #576932 - Flags: review?(mrbkap)
Comment on attachment 576932 [details] [diff] [review]
Patch v1

Looks good to me.
Attachment #576932 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Flags: in-testsuite-
Target Milestone: --- → mozilla11
(Assignee)

Updated

5 years ago
Attachment #576932 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/2040980c0792
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
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
So mostly deltablue?

Mounir, can you reproduce that locally?
(Assignee)

Comment 10

5 years ago
(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
The thing that confuses me, by the way, is that fundamentally the patche in this should not affect Dromaeo V8 at all.
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?
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
I pushed a backout to mozilla-inbound.
Reopening since now backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla11 → ---
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.
The backout restored the values to the previous level.
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.
(Assignee)

Comment 19

5 years ago
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 ;)
(Assignee)

Comment 20

5 years ago
Created attachment 577533 [details] [diff] [review]
Quickstub nsIDOMHTMLDocument.head
Attachment #576932 - Attachment is obsolete: true
Attachment #577533 - Flags: review?(mrbkap)
(Assignee)

Updated

5 years ago
Attachment #577317 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #577533 - Flags: review?(mrbkap) → review+
(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?
(Assignee)

Comment 22

5 years ago
(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.

Updated

5 years ago
Attachment #577317 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

5 years ago
Attachment #577317 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #577533 - Flags: checkin+
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla11
(Assignee)

Updated

5 years ago
Depends on: 706131
https://hg.mozilla.org/mozilla-central/rev/8f31d4132055
https://hg.mozilla.org/mozilla-central/rev/9ff2a655cf0d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.