780647 - remove debug scopes' dependency on having shapes/slots for unaliased variables

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

As an optimization, bug 767013 is going to remove the shapes/slots in bindings/scopes associated with unaliased variables.  Right now, the DebugScopeProxy logic uses the shapes to do lookup and the slots to store unaliased variables after function's stack frame is popped.  The first is easy to fix (use BindingIter).  The second requires creating some additional storage for unaliased slots.  This patch uses a dense array that hangs off the DebugScopeObject.  Ultimately, not too tricky.

Comment 1

[Luke Wagner [:luke]]

Attachment: avoid touching unaliased slots

By the way, this patch stacks atop bug 775323, but only insofar as it assumes Binding::name is non-NULL.

Comment 2

[Luke Wagner [:luke]]

Attachment: don't use the shape of CallObject

Comment 3

[Luke Wagner [:luke]]

Attachment: avoid touching unaliased slots

Ahem, read p->value *before* removing p from the hash table...

Comment 4

[Luke Wagner [:luke]]

(green on try)

Comment 5

[Jim Blandy :jimb]

Comment on attachment 649307 [details] [diff] [review]
don't use the shape of CallObject

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

Fantastic! It's nice to see Bindings used there, at least partially because the terminology (ARGUMENT/VARIABLE/CONSTANT) matches frames better than mashing shape smallids.

Great comments, and I appreciate removing an early return, too.

Comment 6

[Luke Wagner [:luke]]

For reviewing the second patch: start with the fancy new comment on handleUnaliasedAccess.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 649313 [details] [diff] [review]
avoid touching unaliased slots

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

Wow, very fun!

When we've got a function invocation, none of whose arguments or locals are aliased, we still create a pretend CallObject for that --- but we don't use it for much. handleUnaliasedAccess uses its type and its script, but never touches its slots. Is there another role for the pretend CallObject? If not, it would be nice to have a comment in GetDebugScopeForMissing explaining that this object we're creating doesn't actually do much.

::: js/src/vm/ScopeObject.cpp
@@ +1129,2 @@
>                      if (action == GET)
> +                        *vp = UndefinedValue();

In this case, I think we may want to throw an error, explaining why the variable is unavailable. Is that practical?

Technically, what should happen when this situation arises is that the Debugger evaluation function, Debugger.Frame.prototype.evaluate or whatever, should itself throw an error, rather than returning a { throw: ... } completion code, thus making a clean distinction between genuine debuggee errors and the debugger catching an optimization in action. But that is out of scope for this patch.

@@ +1423,5 @@
>          return NULL;
>  
>      JS_ASSERT(!enclosing->isScope());
>      SetProxyExtra(obj, ENCLOSING_EXTRA, ObjectValue(*enclosing));
> +    SetProxyExtra(obj, SNAPSHOT_EXTRA, NullValue());

So, it's the case that proxy extra slots are simply always traced by the GC?

@@ +1444,5 @@
> +JSObject *
> +DebugScopeObject::maybeSnapshot() const
> +{
> +    JS_ASSERT(!scope().asCall().isForEval());
> +    return GetProxyExtra(const_cast<DebugScopeObject*>(this), SNAPSHOT_EXTRA).toObjectOrNull();

Is it not practical to make GetProxyExtra and GetReservedSlot take a const RawObject, instead of these const_casts?

::: js/src/vm/ScopeObject.h
@@ +496,5 @@
>  /* Provides debugger access to a scope. */
>  class DebugScopeObject : public JSObject
>  {
>      static const unsigned ENCLOSING_EXTRA = 0;
> +    static const unsigned SNAPSHOT_EXTRA = 1;

It seems to me each of these should have a brief comment:

/* The enclosing scope object. DebugScopeObjects are proxies, not ScopeObjects, so they don't have a SCOPE_CHAIN_SLOT. */

/* Dense array holding debugger's copy of unaliased variables, for
   frames that have returned. */

Comment 8

[Luke Wagner [:luke]]

(In reply to Jim Blandy :jimb from comment #7)
> handleUnaliasedAccess uses its type and its script, but never
> touches its slots. Is there another role for the pretend CallObject?

One more corner case: if the debugger code dynamically adds names then those are dynamically added/found on the faked-up CallObject.  I will add a comment to this effect in GetDebugScopeForMissing.

> ::: js/src/vm/ScopeObject.cpp
> @@ +1129,2 @@
> >                      if (action == GET)
> > +                        *vp = UndefinedValue();
> 
> In this case, I think we may want to throw an error, explaining why the
> variable is unavailable. Is that practical?

I was a bit scared to do that since we haven't done it yet (we'd just return 'undefined') and I think this will happen through the normal course of debugging (enumerating all a scope's values for the watch view).  There is also the yet-to-be-filed bug to use the information in bug 753145 to give more eager missing-variable errors, so it seems like making this throw would logically belong with those changes.  On the subject: I was thinking some API users would be just fine with 'undefined' and might not appreciate the exception which would suggest a dynamic pref on the Debugger object... but that is all for another bug.

> So, it's the case that proxy extra slots are simply always traced by the GC?

correct

> Is it not practical to make GetProxyExtra and GetReservedSlot take a const
> RawObject, instead of these const_casts?

Originally they were.  This const_cast business seems to have come in with the RawObject typedef.  The problem is that writing "const RawObject" means "JSObject *const", not "const JSObject *".  Presumably the fix is to have a ConstRawObject typedef.

Can we just do that sfink?

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe1f44c8749
https://hg.mozilla.org/integration/mozilla-inbound/rev/70a7988bfd21

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cbe1f44c8749
https://hg.mozilla.org/mozilla-central/rev/70a7988bfd21

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Luke Wagner [:luke] from comment #8)
> Originally they were.  This const_cast business seems to have come in with
> the RawObject typedef.  The problem is that writing "const RawObject" means
> "JSObject *const", not "const JSObject *".  Presumably the fix is to have a
> ConstRawObject typedef.
> 
> Can we just do that sfink?

That's what I had originally, and people (bhackett? terrence? billm? can't remember who all was involved) voted for eliminating the use of const in the engine instead. That felt too unrelated to do in the same patch, so I sprinkled in a couple of const_casts instead. We should probably have a larger conversation about that.