Bug 480187 (trace-quickstubs)

Make qsgen.py generate traceable natives

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

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

Tracking

({fixed1.9.1})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 2 obsolete attachments)

Need traceable natives for quickstubs functions so that we can trace over them.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Posted 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)
Posted 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'

Comment 5

10 years ago
Presumably it doesn't work if you --disable-jit either.
Compiled ok on Mac. This is great, the speedup on my testcases is ~2x.
Posted 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.

Comment 12

10 years ago
http://hg.mozilla.org/mozilla-central/rev/cd79ce3f743f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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).

Updated

10 years ago
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 17

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

Please land on branch ASAP
Attachment #368096 - Flags: approval1.9.1? → approval1.9.1+

Updated

10 years ago
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.