Closed
Bug 516827
Opened 15 years ago
Closed 15 years ago
Don't build a stack frame to execute empty scripts
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: gal, Assigned: brendan)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 6 obsolete files)
The top script executed in a gmail session by frequency is an empty observer in places (1 byte, STOP). Another top one is 3 bytes (FALSE POPV STOP). We should catch such cases in the JS API layer and bypass execution and just directly return the result.
Comment 1•15 years ago
|
||
I'm not sure the empty observer is necessarily the top script, it was one of many 2-byte scripts which did add up to be the most frequent calls to js_Interpret.
Reporter | ||
Comment 2•15 years ago
|
||
A 2-byte script is always empty, by definition, since 1 byte is TRACE, and the other is STOP. Both are mandatory (it would be 1 byte without your TRACE patch).
Comment 3•15 years ago
|
||
Yeah, I'm just saying it's not necessarily that particular empty function which is our top call, it's just one of many 2-byters. :)
Comment 4•15 years ago
|
||
Also, my patch in bug 481419 shouldn't actually be emitting any new opcodes. Does that come from dvander's patch in bug 459301?
Assignee | ||
Comment 5•15 years ago
|
||
Assignee: general → brendan
Status: NEW → ASSIGNED
Assignee | ||
Updated•15 years ago
|
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Assignee | ||
Comment 6•15 years ago
|
||
Shared empty script is just the start. Still to do: don't execute or call it!
/be
Assignee | ||
Comment 7•15 years ago
|
||
Startup with 9 tabs, opened a 10th, started to load gmail with breakpoint in js_Interpret:
(gdb) p cx.runtime.liveEmptyScripts
$1 = 131
(gdb) p cx.runtime.totalEmptyScripts
$2 = 282
(gdb) p cx.runtime.totalScripts
$3 = 8960
(gdb) p cx.runtime.liveScripts
$4 = 8396
Disabled the bp and continued, loading gmail. Enabled and checked again:
(gdb) p cx.runtime.liveEmptyScripts
$5 = 175
(gdb) p cx.runtime.totalEmptyScripts
$6 = 356
(gdb) p cx.runtime.totalScripts
$7 = 19070
(gdb) p cx.runtime.liveScripts
$8 = 18361
This patch helps a little. Hoping to generalize in a separte bug to non-empty scripts a la ccache(1)!
/be
Attachment #405581 -
Attachment is obsolete: true
Attachment #406343 -
Flags: review?(igor)
Comment 8•15 years ago
|
||
Comment on attachment 406343 [details] [diff] [review]
proposed fix
>
>+static jsbytecode emptyScriptCode[] = {JSOP_STOP, SRC_NULL};
That should be static const jsbytecode so the constant can exists in read-only memory.
>+
>+/* static */ JSScript JSScript::emptyScript = {
>+ emptyScriptCode, 1, JSVERSION_DEFAULT, 0, 0, 0, 0, 0, 1, 0, 0,
>+ emptyScriptCode, {0, NULL}, NULL, 0, 0, 0, NULL
>+};
Again, make it like const JSScript with const_cast<jsbytecode *>(emptyScriptCode). r+ with that fixed.
Attachment #406343 -
Flags: review?(igor) → review+
Assignee | ||
Comment 9•15 years ago
|
||
I got through patching this, it was good (found some no-longer-needed stores of JSOP_STOP and SRC_NULL in js_InitFunctionClass), but then I thought of debuggers. Don't they need to plant JSOP_TRAP on top of JSOP_STOP to "stop in function", even for empty functions?
/be
Flags: wanted1.9.2?
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 10•15 years ago
|
||
It's worse than that: a debugger trap (breakpoint) in any empty function will trap in all empty functions. Will the debugger check for the desired function identity?
Note that with emptyScript sharing we do not call the debugger's newScriptHook or destroyScriptHook, so this may be broken at that level, or even "working" (no breakpoint for you!).
Probably we should try this and see how it flies, and fix any issue in a followup bug. But what would the fix entail?
One possibility is an emptyTrapScript singleton, with switching to and fro.
/be
Attachment #406343 -
Attachment is obsolete: true
Attachment #406509 -
Flags: review?(igor)
Comment 11•15 years ago
|
||
(In reply to comment #10)
> Created an attachment (id=406509) [details]
> with const-ipation
>
> It's worse than that: a debugger trap (breakpoint) in any empty function will
> trap in all empty functions.
I have missed that completely. And moreover, the map from script to line number / file name would not work at all. What is the place that most frequently executes such empty scripts?
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=406509) [details] [details]
> > with const-ipation
> >
> > It's worse than that: a debugger trap (breakpoint) in any empty function will
> > trap in all empty functions.
>
> I have missed that completely. And moreover, the map from script to line number
> / file name would not work at all. What is the place that most frequently
> executes such empty scripts?
A bunch of places in gmail, apparently. I didn't study these use-cases.
We could just bypass executing very short (1- or 2-byte) scripts. That would do the trick. I'll look into it.
/be
Reporter | ||
Comment 13•15 years ago
|
||
Thats what we originally came up with, too. Just peek into the script at a high level before allocating the stack and then bypass its execution.
Assignee | ||
Comment 14•15 years ago
|
||
But it is a shame to lose the memory savings...
/be
Comment 15•15 years ago
|
||
Would be nice if we could just clone the script when inserting traps...
Assignee | ||
Comment 16•15 years ago
|
||
(In reply to comment #15)
> Would be nice if we could just clone the script when inserting traps...
We have no way to recover the filename and lineno, though.
/be
Comment 17•15 years ago
|
||
(In reply to comment #12)
> We could just bypass executing very short (1- or 2-byte) scripts. That would do
> the trick. I'll look into it.
In that case, though, breakpoints set there _still_ won't get fired, so it doesn't seem like there's any gain.
We could check that the script's code was exactly "STOP" or "TRACE STOP" to skip those cases without tripping up users who expect breakpoints in empty functions to fire. (Or they could just change the function, I guess, but that's more invasive and a special case.)
Assignee | ||
Comment 18•15 years ago
|
||
Yeah, my patch inspects bytecode in JSScript::isEmpty(). I'm thinking about caching with a bit in JSScript to avoid reinspecting all the time. Setting a trap would clear the bit.
/be
Comment 19•15 years ago
|
||
What about disabling the empty script optimization with the debugger installed? it would not cover all the cases, but at least the most common one would not cause surprises.
Assignee | ||
Comment 20•15 years ago
|
||
I'm going to try to rescue the original patch as crowder and igor suggest.
/be
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 21•15 years ago
|
||
I'll work on tests over the weekend.
/be
Attachment #406509 -
Attachment is obsolete: true
Attachment #406829 -
Flags: review?(igor)
Attachment #406509 -
Flags: review?(igor)
Assignee | ||
Comment 22•15 years ago
|
||
Interdiffs against previous "proposed fix" (attachment 406343 [details] [diff] [review]).
/be
Assignee | ||
Comment 23•15 years ago
|
||
If the debugger is not hooking into newScript and destroyScript hooks, it will hit an error case (novel) in JS_SetTrap. Too bad!
/be
Attachment #406829 -
Attachment is obsolete: true
Attachment #406835 -
Flags: review?(igor)
Attachment #406829 -
Flags: review?(igor)
Assignee | ||
Comment 24•15 years ago
|
||
TBH, only newScriptHook'ing is detected, but that seems fair.
/be
Updated•15 years ago
|
Attachment #406835 -
Flags: review?(igor) → review+
Assignee | ||
Comment 25•15 years ago
|
||
JS_XDRScript callers then call JS_NewScriptObject, so they need to set u.object. This is unfortunate but it affects only top-level XUL and XPC fastload scripts, which should not be empty (although may be vacuously full of JSOP_NOPs for top level function definitions, preceded by JSOP_DEFFUNs in the prolog).
XDR'ed function scripts can be empty, which is the important win.
Without this fix we'd bus-error writing to fields in JSScript::emptyScriptConst.
Also fixed two dumb bugs:
* one in js_Execute: storing through result without null-testing it first (it's optional);
* the other JS_CompileUCScriptForPrincipals not setting TCF_NEED_MUTABLE_SCRIPT.
The jsapi-tests/testXDR.cpp additional case for this bug covers both.
Enough change for a re-review request.
/be
Attachment #406835 -
Attachment is obsolete: true
Attachment #406883 -
Flags: review?(igor)
Assignee | ||
Comment 26•15 years ago
|
||
I had forgotten about JSOP_TRACE being a prolog op for top-level scripts but a main op for functions. I'm not sure this is required but I bet current JIT code needs it to be this way, so I'm going to allow either.
Separately the eval cache wants mutable empty scripts in order to cache them, so rather than hack it up to use the immutable empty singleton, I just have it pass TCF_NEED_MUTABLE_SCRIPT to JSCompiler::compileScript.
/be
Attachment #406883 -
Attachment is obsolete: true
Attachment #406885 -
Flags: review?(igor)
Attachment #406883 -
Flags: review?(igor)
Updated•15 years ago
|
Attachment #406885 -
Flags: review?(igor) → review+
Assignee | ||
Comment 27•15 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 28•15 years ago
|
||
(In reply to comment #26)
> I had forgotten about JSOP_TRACE being a prolog op for top-level scripts but a
> main op for functions. I'm not sure this is required but I bet current JIT code
> needs it to be this way, so I'm going to allow either.
dvander, can you remind me why JSOP_TRACE is prolog for top-level scripts and main for functions?
/be
It was for bug 481419. We can probably eliminate JSOP_TRACE for top-level scripts since the opcode hint is only used for recursion right now, and top-level scripts can't be recursive.
Comment 30•15 years ago
|
||
That bug doesn't explain the difference, that I can tell. Is it OK to just make it be a main op for top-level scripts as well?
bug 523678
It's okay but it can also just be removed entirely. The difference is probably bogus, the JSOP_TRACE can be in the prolog as long as recursion knows to look there for blacklisting.
Comment 32•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 33•15 years ago
|
||
This patch has been caused a crash on start-up. See bug 525481.
Target Milestone: --- → mozilla1.9.3a1
Comment 34•15 years ago
|
||
the last regression for this was just fixed last week, so I don't think we should take this on the branch. speak up if you disagree, of course.
Flags: wanted1.9.2? → wanted1.9.2-
Assignee | ||
Comment 35•15 years ago
|
||
No problem, it would take some big perf win to motivate a late addition to 3.6.
/be
You need to log in
before you can comment on or make changes to this bug.
Description
•