Closed Bug 480187 (trace-quickstubs) Opened 11 years ago Closed 11 years ago

Make qsgen.py generate traceable natives

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 2 obsolete files)

Need traceable natives for quickstubs functions so that we can trace over them.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attached patch Patch, v1 (obsolete) — Splinter Review
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.
Attachment #367439 - Flags: review?(jorendorff)
Attached patch Patch, v1.1 (obsolete) — Splinter Review
Addressed some nits that jst brought up in review.
Attachment #367439 - Attachment is obsolete: true
Attachment #367936 - Flags: review?(jst)
Attachment #367439 - Flags: review?(jorendorff)
Alias: trace-quickstubs
The patch doesn't compile on 64bit linux:
dom_quickstubs.cpp:265 error: expected initializer before 'nsIDOMWindow_GetSelection_tn'
Presumably it doesn't work if you --disable-jit either.
Compiled ok on Mac. This is great, the speedup on my testcases is ~2x.
Attached patch Patch, v1.2Splinter Review
Ok, this version won't generate traceables in --disable-jit builds. Also gives us an easy on/off switch for benchmarking against bug 463238.
Attachment #367936 - Attachment is obsolete: true
Attachment #368096 - Flags: review?(jst)
Attachment #367936 - Flags: review?(jst)
Comment on attachment 368096 [details] [diff] [review]
Patch, v1.2

smaug confirms that this builds on x86_64 where jit is disabled.
Attachment #368096 - Flags: superreview+
Attachment #368096 - Flags: review?(jst)
Attachment #368096 - Flags: review?(jorendorff)
Attachment #368096 - Flags: review+
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.
Pushed to tracemonkey:
http://hg.mozilla.org/tracemonkey/rev/cd79ce3f743f
Whiteboard: fixed-in-tracemonkey
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.
http://hg.mozilla.org/mozilla-central/rev/cd79ce3f743f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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...
Yes, bug 480192 (which also blocks bug 453738).
Flags: blocking1.9.1?
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!
Attachment #368096 - Flags: approval1.9.1?
Comment on attachment 368096 [details] [diff] [review]
Patch, v1.2

Please land on branch ASAP
Attachment #368096 - Flags: approval1.9.1? → approval1.9.1+
Flags: wanted1.9.1+
Blocking to get this off the nomination queue, and since it already landed...
Flags: blocking1.9.1? → blocking1.9.1+
Johnny, is this something we'd really block on?  Comment #18 is a bit ambiguous on why we're +'ing this.
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.
Attachment #368096 - Flags: review?(jorendorff) → review+
You need to log in before you can comment on or make changes to this bug.