Closed Bug 614333 Opened 9 years ago Closed 9 years ago

Remove the empty script optimization

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
The empty script optimization doesn't play well with my ongoing work (for bug 514568) to move binding information out of functions and into scripts.  In particular, fun->u.i.names is not a pointer that you can burn into an empty script constant, which means either you remove the empty script constant or you create it at runtime -- but then you have to deal with another failure point, you need to do something to make it play well with compartments, and from there it all turns into a mess.  Since it's materially impeding progress on bug 514568 (and further note the optimization has been disabled for some time in JM in any case), we should just remove it.
Attachment #492734 - Flags: review?(brendan)
Blocks: 514568
Blocks: 614493
Jeff, can you record the stats you gathered on script populations here for posterity. Thanks,

/be
Comment on attachment 492734 [details] [diff] [review]
Patch

http://infomonkey.cdleary.com/questions/81/what-iswas-the-empty-script-optimization-and-why-dodid-we-think-it-iswas-necessary

Let's get rid of the empty script singleton. It is painful not only for the work in bug 514568 but already, in time spent extending it, avoiding writing to it, and otherwise special casing it.

But absent measurement showing it doesn't matter for code that has empty scripts (gmail, the v8 tests), let's keep JSScript::isEmpty (the #if 0 bits, without the pointer test against the address of the singleton).

/be
Attachment #492734 - Flags: review?(brendan) → review-
(In reply to comment #2)

Do we really want to do that? The more complicated part of empty scripts, IMO, is the checks that it sprinkles about in jsinterp, jstracer, and InvokeHelpers. Keeping the dispatch optimization means introducing more disparity between the interpreter and method JIT calling sequences.

The dispatch overhead was measured back when we were interpreting, i.e. back then we actually had dispatch overhead. But the data in the bug is about optimizing space anyway, I don't see anything else mentioned.

I think we should just get rid of it entirely, and file a follow-up bug on getting more data, and then if warranted, re-introducing the optimization in a simpler way (and not pattern matching every time we want to test).
(In reply to comment #3)
> Do we really want to do that? The more complicated part of empty scripts, IMO,
> is the checks that it sprinkles about in jsinterp, jstracer, and InvokeHelpers.

Small anecdote of the death by 1000 wasted seconds from just this morning: I was grepping for js_CreateThis* to see how we are creating 'this', and had to consider 3 additional cases (out of 9 total) just for the empty script.
Attached patch PatchSplinter Review
(This is atop the earlier patch, but I can move it underneath without much effort -- just don't want to do that until we agree this system is a reasonable idea/way to do it.)

I added some stuff to log empty/non-empty script counts, then browsed around with it through popular JS-heavy sites of the sort in the answer to this question:

http://infomonkey.cdleary.com/questions/100/what-are-some-good-js-heavy-sites-to-use-for-generating-real-world-usage-statistics

Using those sites over an extended browsing session, this was what the heap looked like at the point where the most scripts ever were live:

high water script count: 1548 empty, 41293 not (total 124568)

The meaning is that, at that point, 1548 living scripts were empty, 41293 were not, and over all time 124568 scripts had been created.  So we're talking about 0.3% of all scripts as empty, for that run.  An earlier run I did with the current set of tabs I'm regularly browsing with gave these numbers:

high water script count: 1066 empty, 124220 not (total 137743)

There are more empty scripts in that set of pages, but that set is more specific to me and less generally representative.

In any case, it looks like we're seeing on the order of one script in every hundred being an empty script.
Attachment #495638 - Flags: review?(brendan)
Comment on attachment 495638 [details] [diff] [review]
Patch

>@@ -1044,7 +1056,7 @@ JSScript::NewScriptFromCG(JSContext *cx,
>     const char *filename;
>     JSFunction *fun;
> #ifdef DEBUG
>-    jsrefcount newLive, newTotal, oldHigh;
>+    jsrefcount newEmptyLive, newLive, newTotal, oldHigh;
> #endif
> 
>     /* The counts of indexed things must be checked during code generation. */
>@@ -1165,13 +1177,22 @@ JSScript::NewScriptFromCG(JSContext *cx,
>     /* Tell the debugger about this compiled script. */
>     js_CallNewScriptHook(cx, script, fun);

Blank line here, and move the jsrefcount declarations down here, since they don't need to be in scope earlier and this is the tail of the function (only one return below).

>@@ -1207,6 +1228,10 @@ js_CallDestroyScriptHook(JSContext *cx, 
> static void
> DestroyScript(JSContext *cx, JSScript *script, JSThreadData *data)
> {
>+#ifdef DEBUG
>+    bool isEmpty = IsEmptyScript(script);
>+#endif
>+
>     js_CallDestroyScriptHook(cx, script);
>     JS_ClearScriptTraps(cx, script);
> 
>@@ -1276,7 +1301,12 @@ DestroyScript(JSContext *cx, JSScript *s
> 
>     cx->free(script);
> 
>-    JS_RUNTIME_UNMETER(cx->runtime, liveScripts);
>+#ifdef DEBUG
>+    if (isEmpty)
>+        JS_RUNTIME_UNMETER(cx->runtime, liveEmptyScripts);
>+    else
>+        JS_RUNTIME_UNMETER(cx->runtime, liveScripts);
>+#endif
> }
> 
> void

Similarly, can you move up the instrumentation, since this function is infallible, and do it all early while script still lives?

We can get rid of all this empty script jazz, though, if there's no ding on v8 tests or other benchmarks. It sounds like there won't be now that we're JITting well.

Thanks for the measurements! That is really what comment 2 wanted.

/be
Attachment #495638 - Flags: review?(brendan) → review+
(In reply to comment #6)
> Blank line here, and move the jsrefcount declarations down here, since they
> don't need to be in scope earlier and this is the tail of the function (only
> one return below).

Would love to do it, would have done it, but the bad: label at the end precludes that.  :-(  But scoping them as minimally as possible with an extra block does the same trick -- should have thought to do that myself.

> Similarly, can you move up the instrumentation, since this function is
> infallible, and do it all early while script still lives?

I considered doing that, decided the delta was slightly more than I wanted (atop my other patches, recall -- the standalone patch delta's much smaller) just to satisfy my own little itch -- glad to do it if it's more than just me.

http://hg.mozilla.org/tracemonkey/rev/5fe72e174745

More to do here still -- time to rebase mq through that change...
Attached patch Take twoSplinter Review
So, now that we have the requested stats and their meaning, let's see about finishing this up.  Take two?
Attachment #492734 - Attachment is obsolete: true
Attachment #496032 - Flags: review?(brendan)
Comment on attachment 496032 [details] [diff] [review]
Take two

>@@ -13196,28 +13193,6 @@ TraceRecorder::guardArguments(JSObject *
> JS_REQUIRES_STACK RecordingStatus
> TraceRecorder::interpretedFunctionCall(Value& fval, JSFunction* fun, uintN argc, bool constructing)
> {
>-    /*
>-     * The function's identity (JSFunction and therefore JSScript) is guarded,
>-     * so we can optimize for the empty script singleton right away. No need to
>-     * worry about crossing globals or relocating argv, even, in this case!
>-     *
>-     * Note that the interpreter shortcuts empty-script call and construct too,
>-     * and does not call any TR::record_*CallComplete hook.
>-     */
>-    if (fun->u.i.script->isEmpty()) {
>-        LIns* rval_ins;
>-        if (constructing) {
>-            LIns* args[] = { get(&fval), w.nameImmpNonGC(&js_ObjectClass), cx_ins };
>-            LIns* tv_ins = w.call(&js_CreateThisFromTrace_ci, args);
>-            guard(false, w.eqp0(tv_ins), OOM_EXIT);
>-            rval_ins = tv_ins;
>-        } else {
>-            rval_ins = w.immiUndefined();
>-        }
>-        stack(-2 - argc, rval_ins);
>-        return RECORD_CONTINUE;
>-    }
>-

Keep this, it is a valid use of isEmpty to trace-JIT-optimize.

>@@ -422,8 +410,7 @@ stubs::UncachedNewHelper(VMFrame &f, uin
>     Value *vp = f.regs.sp - (argc + 2);
> 
>     /* Try to do a fast inline call before the general Invoke path. */
>-    if (IsFunctionObject(*vp, &ucr->fun) && ucr->fun->isInterpreted() && 
>-        !ucr->fun->script()->isEmpty())
>+    if (IsFunctionObject(*vp, &ucr->fun) && ucr->fun->isInterpreted())
>     {

Pull brace up now that condition fits on one line.

r=me with these fixed.

/be
Attachment #496032 - Flags: review?(brendan) → review+
If you keep that, you also have to keep the interpreter case.
(In reply to comment #10)
> If you keep that, you also have to keep the interpreter case.

Yes, should have noted that.

/be
Cc'ing gal, who is arguing that empty event handlers from session store make for tons of empty function event handlers.

This means keeping isEmpty() tests in the other fat path that can afford the overhead (besides the interpreter): the ExternalInvoke path from JS_CallFunctionValue.

/be
(In reply to comment #12)
> This means keeping isEmpty() tests in the other fat path that can afford the
> overhead (besides the interpreter): the ExternalInvoke path from
> JS_CallFunctionValue.

Current in Invoke, in the removed lines of the last patch.

Need some data here. Andreas?

/be
I am looking for the bug, but basically we observed that a lot of our internal APIs require sets of event handlers, but often we only really care about 1 event, and the rest are empty handlers. Before the optimization some huge fraction of script executions were empty scripts, and building the stack was very costly.
So, looking at this, I'm not sure I'm convinced it's a good idea.  If it were isolated to the tracer only, that would be one thing -- with just that I was all for the change.  But then when I ran into the obvious failures from not changing the interpreter, I began to wonder.  Given the current *CallComplete post-hook, special-casing empty here means more complexity in multiple locations, not just one isolated spot in tracing code.

Do we have evidence that the performance of calling empty scripts on trace matters on the web?  I'm wondering if this added complexity pays its way in practice, especially for the constructing-empty case where I would think object construction dominates analysis.
Agree with the above. Keeping in mind that the amount of overhead from inlining an empty script is super small, and that these days our stack frames are really lightweight.
This is not about inlining. This is about JSAPI calls with empty scripts. You cannot ignore that case. I am as much against complexity as the next guy, but this is not a trivial case you can ignore. Measure before you cut.
Okay, if we're talking about the fatter API calls, that seems reasonable.
There are no isEmpty() checks in the fat path right now.  Are you suggesting adding them anew to ExternalInvoke?
(in ExternalInvoke directly, that is, rather than indirectly via Invoke)
http://hg.mozilla.org/mozilla-central/rev/5fe72e174745
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We're no longer measuring. In the absence of measurements of real-world apps and pages and Firefox session restore, etc., we shouldn't be ripping out stuff just to simplify code slightly. That is for after Fx4. This says to leave the isEmpty tests, take out the singleton, file a followup bug or bugs.

/be
oops, what's not fixed here?
I added some stats-logging stuff, but the main body of work to be done here isn't landed.  I'm running tests on that now, however, and if they pass I'll land today.
http://hg.mozilla.org/tracemonkey/rev/be1532afeb63

I think I responded to all the comment here, and the little bit of clarification I got on IRC.  If you see something not actually desired, please speak up.  :-)
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/be1532afeb63
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 595391
You need to log in before you can comment on or make changes to this bug.