480187 - (trace-quickstubs) Make qsgen.py generate traceable natives

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Need traceable natives for quickstubs functions so that we can trace over them.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Here's my first shot. There are a few problems still but overall I think this works really well.

With this patch we now automatically generate both FastNative and TraceableNative functions for all methods in dom_quickstubs.qconf with three arguments or less. Attributes don't get traced yet.

The makefile logic is really ugly, we need to include private js/nanojit headers and have defines that get pulled from js/src/mozilla-config.h for it all to work :( Need to figure out how the engine is going to expose these macros without all that, I think.

In a simple contrived testcase with document.getElementById, node.isEqualNode, and window.dump I saw a 3x speedup and verified that we're staying on trace!

Dromaeo doesn't really budge with this patch, I'm betting because of upvar and the fact that getters/setters aren't traced. At least I hope so.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Simple testcase

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

Addressed some nits that jst brought up in review.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

The patch doesn't compile on 64bit linux:
dom_quickstubs.cpp:265 error: expected initializer before 'nsIDOMWindow_GetSelection_tn'

Comment 5

[Benjamin Smedberg]

Presumably it doesn't work if you --disable-jit either.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Compiled ok on Mac. This is great, the speedup on my testcases is ~2x.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.2

Ok, this version won't generate traceables in --disable-jit builds. Also gives us an easy on/off switch for benchmarking against bug 463238.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 368096 [details] [diff] [review]
Patch, v1.2

smaug confirms that this builds on x86_64 where jit is disabled.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 368096 [details] [diff] [review]
Patch, v1.2

Looks good to me. I'd say lets get this in on the trunk to get testing on it. It'd be great if Jason could review this at some point too though, but I'd be fine with landing this on the trunk and waiting until Jason gets back for him to have a look at this.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed to tracemonkey:
http://hg.mozilla.org/tracemonkey/rev/cd79ce3f743f

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, so, windows headers include a file that redefines "THIS" which messes up jsbuiltins macros. Fix was to include the jsbuiltins.h file after the others.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cd79ce3f743f

Comment 13

[Boris Zbarsky [:bzbarsky]]

Is there a bug on making getters and setters work too, by the way?  Once that happens, we should be able to stay on trace through a number of DOM-touching scripts...

Comment 14

[Jason Orendorff [:jorendorff]]

Yes, bug 480192 (which also blocks bug 453738).

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 368096 [details] [diff] [review]
Patch, v1.2

This has baked on trunk for a while now and speeds up some simpler scripts. It won't make dromaeo significantly faster until we trace getters/setters, but it's better than what we have now!

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d014bb2acef7

Comment 17

[Benjamin Smedberg]

Comment on attachment 368096 [details] [diff] [review]
Patch, v1.2

Please land on branch ASAP

Comment 18

[Johnny Stenback (:jst)]

Blocking to get this off the nomination queue, and since it already landed...

Comment 19

[Damon Sicore (:damons)]

Johnny, is this something we'd really block on?  Comment #18 is a bit ambiguous on why we're +'ing this.

Comment 20

[Johnny Stenback (:jst)]

To be honest, I don't know. But I'd rather have this marked a blocker and figuring that out if this gets backed out and we need to decide what to do than to have it get backed out and forgotten since it's not a blocker.