516827 - Don't build a stack frame to execute empty scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Andreas Gal :gal]

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

[Brian Crowder]

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.

Comment 2

[Andreas Gal :gal]

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

[Brian Crowder]

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

[Brian Crowder]

Also, my patch in bug 481419 shouldn't actually be emitting any new opcodes.  Does that come from dvander's patch in bug 459301?

Comment 5

[Brendan Eich [:brendan]]

Attachment: patch in progress

Comment 6

[Brendan Eich [:brendan]]

Shared empty script is just the start. Still to do: don't execute or call it!

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix

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

Comment 8

[Igor Bukanov]

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.

Comment 9

[Brendan Eich [:brendan]]

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

Comment 10

[Brendan Eich [:brendan]]

Attachment: with const-ipation

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

Comment 11

[Igor Bukanov]

(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?

Comment 12

[Brendan Eich [:brendan]]

(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

Comment 13

[Andreas Gal :gal]

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.

Comment 14

[Brendan Eich [:brendan]]

But it is a shame to lose the memory savings...

/be

Comment 15

[Brian Crowder]

Would be nice if we could just clone the script when inserting traps...

Comment 16

[Brendan Eich [:brendan]]

(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

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(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.)

Comment 18

[Brendan Eich [:brendan]]

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

[Igor Bukanov]

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.

Comment 20

[Brendan Eich [:brendan]]

I'm going to try to rescue the original patch as crowder and igor suggest.

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: proposed fix

I'll work on tests over the weekend.

/be

Comment 22

[Brendan Eich [:brendan]]

Interdiffs against previous "proposed fix" (attachment 406343 [details] [diff] [review]).

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: proposed fix

If the debugger is not hooking into newScript and destroyScript hooks, it will hit an error case (novel) in JS_SetTrap. Too bad!

/be

Comment 24

[Brendan Eich [:brendan]]

TBH, only newScriptHook'ing is detected, but that seems fair.

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: fix XDR decode path to make mutable empty scripts when necessary

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

Comment 26

[Brendan Eich [:brendan]]

Attachment: cope with JSOP_TRACE as prolog or main op (it can be either), fix eval to cache mutable empty scripts

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

Comment 27

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/60ec3940a434

/be

Comment 28

[Brendan Eich [:brendan]]

(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

Comment 29

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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?

Comment 31

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/60ec3940a434

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This patch has been caused a crash on start-up. See bug 525481.

Comment 34

[Robert Sayre]

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.

Comment 35

[Brendan Eich [:brendan]]

No problem, it would take some big perf win to motivate a late addition to 3.6.

/be