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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Anthony Ricaud (:rik)]

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 (Exited; not receiving bugmail, old account, do not use)]

Well, we don't quickstub it, so that's not that surprising.

Comment 2

[Mounir Lamouri (:mounir)]

I have a patch compiling with document.head quickstubbed.

Comment 3

[Mounir Lamouri (:mounir)]

(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)]

Attachment: Patch v1

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 576932 [details] [diff] [review]
Patch v1

Looks good to me.

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2040980c0792

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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] (Away Apr 25 - May 5)]

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 [:bzbarsky]]

So mostly deltablue?

Mounir, can you reproduce that locally?

Comment 10

[Mounir Lamouri (:mounir)]

(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 [:bzbarsky]]

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 [:bzbarsky]]

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)]

(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)]

I pushed a backout to mozilla-inbound.

Comment 15

[Ed Morley [:emorley]]

Reopening since now backed out.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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] (Away Apr 25 - May 5)]

The backout restored the values to the previous level.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: 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)]

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)]

Attachment: Quickstub nsIDOMHTMLDocument.head

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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)]

(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.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/8f31d4132055
https://hg.mozilla.org/mozilla-central/rev/9ff2a655cf0d