Closed Bug 516827 Opened 13 years ago Closed 13 years ago

Don't build a stack frame to execute empty scripts

Categories

(Core :: JavaScript Engine, defect, P3)

defect

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.
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.
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).
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.  :)
Also, my patch in bug 481419 shouldn't actually be emitting any new opcodes.  Does that come from dvander's patch in bug 459301?
Attached patch patch in progress (obsolete) — Splinter Review
Assignee: general → brendan
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Shared empty script is just the start. Still to do: don't execute or call it!

/be
Attached patch proposed fix (obsolete) — Splinter Review
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 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+
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
Attached patch with const-ipation (obsolete) — Splinter Review
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)
(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?
(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
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.
But it is a shame to lose the memory savings...

/be
Would be nice if we could just clone the script when inserting traps...
(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
(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.)
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
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.
I'm going to try to rescue the original patch as crowder and igor suggest.

/be
Whiteboard: fixed-in-tracemonkey
Attached patch proposed fix (obsolete) — Splinter Review
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)
Interdiffs against previous "proposed fix" (attachment 406343 [details] [diff] [review]).

/be
Attached patch proposed fix (obsolete) — Splinter Review
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)
TBH, only newScriptHook'ing is detected, but that seems fair.

/be
Attachment #406835 - Flags: review?(igor) → review+
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)
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)
Attachment #406885 - Flags: review?(igor) → review+
http://hg.mozilla.org/tracemonkey/rev/60ec3940a434

/be
Whiteboard: fixed-in-tracemonkey
(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.
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.
http://hg.mozilla.org/mozilla-central/rev/60ec3940a434
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This patch has been caused a crash on start-up. See bug 525481.
Target Milestone: --- → mozilla1.9.3a1
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-
No problem, it would take some big perf win to motivate a late addition to 3.6.

/be
Depends on: 572232
You need to log in before you can comment on or make changes to this bug.