Need traceable natives for quickstubs functions so that we can trace over them.
10 years ago
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
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.
Addressed some nits that jst brought up in review.
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.
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 on attachment 368096 [details] [diff] [review] Patch, v1.2 smaug confirms that this builds on x86_64 where jit is disabled.
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
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.
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...
10 years ago
Depends on: 484352
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+
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.