933557 - [jsdbg2] Setting a Debugger.prototype.onEnterFrame hook imposes substantial overhead

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

As shown in the chart in bug 822000, in that benchmark, the overhead of a single onEnterFrame hook is substantial. This makes it less than ideal for things like tracers, which we were hoping it could be used for.

Shu-yu Guo has done some measurements: commenting out the entire cache miss branch in Debugger::getScriptFrame, causing the handler function to be passed 'undefined', cuts the time the benchmark spends on the onEnterFrame hook from ~500ms to ~330ms (34%).

Omitting only the 'iter.copyData()' call drops it by 15%-20%.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Lazily construct ScriptFrameIter in Debugger.Frame methods. (r=?)

Results of https://github.com/jimblandy/benchmark-debugger with patch below.

--ion,    debugger, onEnterFrame
warmup
measure
[Stats total: 173.08414697265633s, mean: 0.3461682939453127s, stddev: 5%]

--no-ion, debugger, onEnterFrame
warmup
measure
[Stats total: 174.5817080078126s, mean: 0.3491634160156252s, stddev: 3%]

--ion,    debugger, no hooks
warmup
measure
[Stats total: 75.49647534179682s, mean: 0.15099295068359364s, stddev: 4%]

--no-ion, debugger, no hooks
warmup
measure
[Stats total: 75.13240478515618s, mean: 0.15026480957031235s, stddev: 3%]

--ion
warmup
measure
[Stats total: 20.671670166015605s, mean: 0.041343340332031214s, stddev: 98%]

--no-ion
warmup
measure
[Stats total: 41.65933740234373s, mean: 0.08331867480468746s, stddev: 5%]

My numbers without patch are similar to what's in bug 914429.

Comment 2

[Jim Blandy :jimb]

So, this brings the overhead down from ~12x to ~8x. Looking forward to doing the review.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 825789 [details] [diff] [review]
Lazily construct ScriptFrameIter in Debugger.Frame methods. (r=?)

Review of attachment 825789 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great.

The one change I would ask for is this: rather than exporting the knowledge that no raw-form AbstractFramePtr has its bottom two bits clear, instead, give AbstractFramePtr a third "void *" variant, with its own isBlah and asBlah methods. Then the tag can be entirely private to AbstractFramePtr, and the raw values will be used solely for hashing and storing as private values.

Aren't we going to add an Ion frame pointer variant to AbstractFramePtr soon? Would it be worth actually having a private enum for the tag values? We'll certainly want that if we have four variants...

::: js/src/vm/Debugger.cpp
@@ +990,5 @@
>      RootedObject hook(cx, getHook(OnEnterFrame));
>      JS_ASSERT(hook);
>      JS_ASSERT(hook->isCallable());
>  
> +    //ScriptFrameIter iter(cx);

This should just come out altogether.

@@ +3945,5 @@
>  static bool
>  DebuggerFrame_getCallee(JSContext *cx, unsigned argc, Value *vp)
>  {
> +    THIS_FRAME(cx, argc, vp, "get callee", args, thisobj, frame);
> +    RootedValue calleev(cx, (frame.isFunctionFrame() && !frame.isEvalFrame()) ? frame.calleev() : NullValue());

SpiderMonkey style limits lines to 100 columns.

::: js/src/vm/Stack.h
@@ +102,5 @@
>  
>      AbstractFramePtr(StackFrame *fp)
>          : ptr_(fp ? uintptr_t(fp) | 0x1 : 0)
>      {
> +        JS_ASSERT((uintptr_t(fp) & TagMask) == 0);

Suggestion, only if you like it: I like to do these post-tagging assertions by calling the untagging function itself: JS_ASSERT(asStackFrame() == fp), as that seems like a very direct way to say, "The pointer survives the round trip."

@@ +128,5 @@
>          return ptr_ & 0x1;
>      }
>      StackFrame *asStackFrame() const {
>          JS_ASSERT(isStackFrame());
> +        StackFrame *res = (StackFrame *)(ptr_ & ~TagMask);

I don't know if this is still true, but it used to be the case that *subtracting* off lowtags, instead of *masking* them off, would generate better code, because the compiler could fold in the subtraction with subsequent additions (say, adding the offset of a structure member), but wasn't smart enough to tell that the masking had the same effect as a subtraction because the relevant bits are known. (Kent Dybvig mentioned this in a paper about Chez Scheme.)

Comment 4

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #3)
> The one change I would ask for is this: rather than exporting the knowledge
> that no raw-form AbstractFramePtr has its bottom two bits clear, instead,
> give AbstractFramePtr a third "void *" variant, with its own isBlah and
> asBlah methods. Then the tag can be entirely private to AbstractFramePtr,
> and the raw values will be used solely for hashing and storing as private
> values.

To clarify, I meant that Debugger should actually use this third variant to store its ScriptFrameIter pointer. Then, there's no need for anything outside AbstractFramePtr to do tests of its low bits.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Lazily construct ScriptFrameIter in Debugger.Frame methods. (r=?)

Abstracting away pointer bit tagging. Asking for a quick re-r? since C++ is
stupid and had to shuffle things around.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 826222 [details] [diff] [review]
Lazily construct ScriptFrameIter in Debugger.Frame methods. (r=?)

Review of attachment 826222 [details] [diff] [review]:
-----------------------------------------------------------------

There's another issue I noticed this read-through: unless I'm missing something, walking a stack from newest to oldest has become quadratic, since Debugger::getScriptFrame only constructs lazy D.Fs. If DebuggerFrame_getOlder could create non-lazy D.Fs, then that wouldn't affect our benchmark, and it would avoid the quadratic behavior. Perhaps the ScriptFrameIter overload of getScriptFrame could be smarter?

::: js/src/vm/Stack.h
@@ +143,5 @@
> +        return frame;
> +    }
> +
> +    bool isScriptFrameIterData() const {
> +        return (ptr_ & TagMask) == Tag_ScriptFrameIterData;

This will return true on a null AbstractFramePtr; is that a problem?

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #6)
> Comment on attachment 826222 [details] [diff] [review]
> Lazily construct ScriptFrameIter in Debugger.Frame methods. (r=?)
> 
> Review of attachment 826222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's another issue I noticed this read-through: unless I'm missing
> something, walking a stack from newest to oldest has become quadratic, since
> Debugger::getScriptFrame only constructs lazy D.Fs. If
> DebuggerFrame_getOlder could create non-lazy D.Fs, then that wouldn't affect
> our benchmark, and it would avoid the quadratic behavior. Perhaps the
> ScriptFrameIter overload of getScriptFrame could be smarter?
> 

Ooh, great catch there.

> ::: js/src/vm/Stack.h
> @@ +143,5 @@
> > +        return frame;
> > +    }
> > +
> > +    bool isScriptFrameIterData() const {
> > +        return (ptr_ & TagMask) == Tag_ScriptFrameIterData;
> 
> This will return true on a null AbstractFramePtr; is that a problem?

Good eye, this should check !!ptr_.

Comment 8

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dec60f5c326c

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dec60f5c326c