637985 - Reimplement watchpoints using a JSObject bit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Now that we have JITs and shape guards in all the fast paths, we can reimplement breakpoints in the dumbest, most straightforward possible way without hurting fast-path perf.

jimb proposed this. Here's the plan: when we set the first watchpoint on an object, set a bit on the object, give it a unique shape, and make all property assignments on that object uncacheable, thus forcing them into the slow path (js_SetPropertyHelper). In the slow path, test for the JSObject bit, and if it's set, look for watchpoints.

Benefits:

  - Gets rid of the js_watch_set magic setter op and WrapWatchedSetter.

  - Gets rid of js_UpdateWatchpointsForShape. Setting or clearing
    watchpoints won't touch js::Shapes at all, only the JSObject's
    "watched" bit and shape.

  - Watchpoints won't have to know anything about how to perform
    a property set.

  - Stuff that looks at Shape::setterOp won't have to know anything
    about watchpoints.

  - Watchpoints won't have to cache the watched setter.

  - The watchpoint implementation won't have to know so much about
    methods. (It'll still need to trip the method read barrier,
    I think.)

  - Bug 604781 and bug 605596 will go away. Probably others too.

  - Watchpoints on accessor properties won't be exposed to
    __lookupSetter__ and Object.getOwnPropertyDescriptor, which
    always seemed inappropriate for a debugger feature, at least
    to me.

Comment 1

[Jason Orendorff [:jorendorff]]

We will likely have to test for the JSObject bit in the trace recorder: once up front, to make sure we do not record at all if the global object is watched; and also on each SET opcode, to make sure the target object isn't watched. These are record-time checks, not run-time guards.

Comment 2

[Brendan Eich [:brendan]]

Love it. Also, unwatch can revert to best-shared-shape status, not own-shape.

/be

Comment 3

[Andreas Gal :gal]

This will be good for the GC too which right now gets incredibly slow if there are any watchpoints installed at all.

Comment 5

[Nicholas Nethercote [inactive]]

I don't know anything about watchpoints but that list in comment 0 sounds great :)

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1 - passes tests (yikes)

This passes all tests. That just goes to show how few correctness tests we have on watch. I know of two bugs it introduces.

  - avoid caching assignments on watched objects in ICs
  - avoid tracing assignments on watched objects in tracer (comment 1)

Big simplicity win though, as expected.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2

r?dvander for the JIT pieces. In particular, I need to know if the changes to jstracer and methodjit are sufficient. All I'm trying to accomplish is to avoid missing a watchpoint. I do it by inhibiting caching and optimizations.

r?jimb for everything else.

Comment 8

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

Comment on attachment 528210 [details] [diff] [review]
v2

Review of attachment 528210 [details] [diff] [review]:

r=me on JIT parts. Awesome to see the watchpoint hacks go away!

::: js/src/methodjit/PolyIC.cpp
@@ +482,5 @@
             return disable("dense array");
         if (!obj->isNative())
             return disable("non-native");
+        if (obj->watched())
+            return disable("watchpoint");

Thinking out loud, not really in relation to this patch... it kind of sucks that when trying to cache object operations that there's a bunch of mysterious checks you have to make that aren't really documented, unless you have a complete understanding of the different states an object and property can be in.

And for some operations the set of things you need to check is different. Like here, the watch check isn't needed for GetProp, which makes sense... but I guess I'm hoping that someday all of the caching logic will be magically condensed to one place, or as few as possible.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 528210 [details] [diff] [review]
v2

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

Some comments:

::: js/src/jsdbgapi.cpp
@@ +605,5 @@
>  }
>  
>  /************************************************************************/
>  
> +#include "jswatchpoints.h"

This should be #included at the top of the file, with the other headers, not at the spot where the old code was deleted.

::: js/src/jstracer.cpp
@@ +13993,5 @@
>   * property with a slot; if the property turns out to be anything else, abort
>   * tracing (rather than emit a call to a native getter or GetAnyProperty).
>   */
>  JS_REQUIRES_STACK AbortableRecordingStatus
>  TraceRecorder::prop(JSObject* obj, LIns* obj_ins, uint32 *slotp, LIns** v_insp, Value *outp)

Is my thinking correct that the reason things like prop and incProp never run afoul of watchpoints is that they always try to fill a property cache entry, but js::PropertyCache::fill will refuse to create a property cache entry for an object that has a watchpoint set on it?

::: js/src/jswatchpoints.cpp
@@ +1,1 @@
> +#include "jswatchpoints.h"

Need top-of-file boilerplate here.

@@ +103,5 @@
> +    if (obj->isNative()) {
> +        if (const Shape *shape = obj->nativeLookup(id)) {
> +            uint32 slot = shape->slot;
> +            if (obj->containsSlot(slot)) {
> +                old = obj->nativeGetSlot(slot);

Couldn't this be in an 'else' clause of the 'if (shape->isMethod())'? As far as I can see, the assignment is dead if shape->isMethod() is true.

@@ +128,5 @@
> +    return handler(cx, obj, id, Jsvalify(old), Jsvalify(vp), closure);
> +}
> +
> +bool
> +WatchpointMap::trace(JSTracer *trc, JSCompartment *compartment)

The 'compartment' parameter is unused. Is this right?

@@ +133,5 @@
> +{
> +    bool mutated = false;
> +    JSRuntime *rt = trc->context->runtime;
> +    for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c)
> +        mutated |= (*c)->watchpointMap.trace(trc);

Took me a while to understand why this '|=' was safe (bool values are reliably 0 or 1 when converted to int), but perhaps that's just my C paranoia and not a real legibility issue.

@@ +143,5 @@
> +{
> +    bool marked = false;
> +    for (Map::Range r = map.all(); !r.empty(); r.popFront()) {
> +        Map::Entry &e = r.front();
> +        if (e.key.object->isMarked() || e.value.held) {

Since we're going to retain the entry if the key is not marked but the entry is held, it seems to me we'd better mark the key, or else we'll have entries with dead keys.

@@ +172,5 @@
> +WatchpointMap::sweep()
> +{
> +    for (Map::Enum r(map); !r.empty(); r.popFront()) {
> +        Map::Entry &e = r.front();
> +        if (!e.key.object->isMarked() && !e.value.held)

This makes me think the above comment is correct: we should never be retaining a table entry pointing to an unmarked key.

::: js/src/jswatchpoints.h
@@ +49,5 @@
> +
> +struct WatchKey {
> +    WatchKey() {}
> +    WatchKey(JSObject *obj, jsid id) : object(obj), id(id) {}
> +    WatchKey(const WatchKey &key) : object(key.object), id(key.id) {}

How is this different from the default copy constructor?

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to comment #9)
> > +#include "jswatchpoints.h"
> 
> This should be #included at the top of the file, with the other headers, not
> at the spot where the old code was deleted.

Fixed. I renamed these files from jswatchpoints.{h,cpp} to the singular jswatchpoint.{h,cpp}.

> Is my thinking correct that the reason things like prop and incProp never
> run afoul of watchpoints is that they always try to fill a property cache
> entry, but js::PropertyCache::fill will refuse to create a property cache
> entry for an object that has a watchpoint set on it?

Yes.

> ::: js/src/jswatchpoints.cpp
> Need top-of-file boilerplate here.

Fixed.

> > +                old = obj->nativeGetSlot(slot);
> 
> Couldn't this be in an 'else' clause of the 'if (shape->isMethod())'? As far
> as I can see, the assignment is dead if shape->isMethod() is true.

Ah, yes, good point. Fixed.

> > +WatchpointMap::trace(JSTracer *trc, JSCompartment *compartment)
> 
> The 'compartment' parameter is unused. Is this right?

No. Fixed. I also renamed the mark methods to use the word "mark" instead of "trace".

> > +    bool mutated = false;
[...]
> > +        mutated |= (*c)->watchpointMap.trace(trc);
> 
> Took me a while to understand why this '|=' was safe (bool values are
> reliably 0 or 1 when converted to int), but perhaps that's just my C
> paranoia and not a real legibility issue.

My understanding of this is simply that the | operator is well-defined on boolean operands: (true | false) ===> true and so forth.

> @@ +143,5 @@
> > +{
> > +    bool marked = false;
> > +    for (Map::Range r = map.all(); !r.empty(); r.popFront()) {
> > +        Map::Entry &e = r.front();
> > +        if (e.key.object->isMarked() || e.value.held) {
> 
> Since we're going to retain the entry if the key is not marked but the entry
> is held, it seems to me we'd better mark the key, or else we'll have entries
> with dead keys.

Yes, you're right. I don't think it can happen, since e.value.held is only true while an AutoEntryHolder exists on the stack, which means conservative GC is in effect, and the caller certainly has a pointer to the object (it is in the middle of doing a property set on it).

Still, better safe. I added code to mark the key.

> > +    WatchKey(const WatchKey &key) : object(key.object), id(key.id) {}
> 
> How is this different from the default copy constructor?

It's not. Deleted.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: v3

Unbitrotted and with fixes for Jim's comments.

Also I changed JSCompartment::watchpointMap to be a pointer. This is a bit of a pain, but it means that jswatchpoint.h does not need to be in INSTALLED_HEADERS even though jscompartment.h is quite scandalously #included all over the tree.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 542604 [details] [diff] [review]
v3

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

Just two chaser comments --- fix them if you like.

::: js/src/jswatchpoint.cpp
@@ +194,5 @@
> +{
> +    bool marked = false;
> +    for (Map::Range r = map.all(); !r.empty(); r.popFront()) {
> +        Map::Entry &e = r.front();
> +        bool objectIsLive = e.key.object->isMarked();

Just to confirm: this doesn't need to call IsAboutToBeFinalized because the watchpoint map is per-compartment, and thus e.key.object will always be an intra-compartment reference. Right?

@@ +233,5 @@
> +WatchpointMap::sweep()
> +{
> +    for (Map::Enum r(map); !r.empty(); r.popFront()) {
> +        Map::Entry &e = r.front();
> +        if (!e.key.object->isMarked() && !e.value.held)

I think this should become:
  if (!e.key.object->isMarked()) {
    JS_ASSERT(!e.value.held);
    r.removeFront();
  }

to make it clearer that it is never correct to retain an entry whose key is not marked.

Comment 13

[Jason Orendorff [:jorendorff]]

> > +    bool marked = false;
> > +    for (Map::Range r = map.all(); !r.empty(); r.popFront()) {
> > +        Map::Entry &e = r.front();
> > +        bool objectIsLive = e.key.object->isMarked();
> 
> Just to confirm: this doesn't need to call IsAboutToBeFinalized because the
> watchpoint map is per-compartment, and thus e.key.object will always be an
> intra-compartment reference. Right?

Hmm. I'm not sure. I'll look at this closer before landing it.
 
> @@ +233,5 @@
> > +WatchpointMap::sweep()
> > +{
> > +    for (Map::Enum r(map); !r.empty(); r.popFront()) {
> > +        Map::Entry &e = r.front();
> > +        if (!e.key.object->isMarked() && !e.value.held)
> 
> I think this should become:
>   if (!e.key.object->isMarked()) {
>     JS_ASSERT(!e.value.held);
>     r.removeFront();
>   }
> to make it clearer that it is never correct to retain an entry whose key is
> not marked.

Agreed. I have made the change locally.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to comment #12)
> Just to confirm: this doesn't need to call IsAboutToBeFinalized because the
> watchpoint map is per-compartment, and thus e.key.object will always be an
> intra-compartment reference. Right?

Right.

Anyway I'll file the follow-up bug to rip all this out and use a C++ WeakMap.

Comment 15

[Jason Orendorff [:jorendorff]]

hg.mozilla.org/integration/mozilla-inbound/rev/7c43296e7545

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7c43296e7545

Comment 17

[Giorgio Maone [:ma1]]

Is there any chance this or a related landing caused bug 677652 ?

Comment 18

[Giorgio Maone [:ma1]]

Please document the performance impact of this change and suggest modern/standard alternatives to Object.watch() on MDN, thanks.