Last Comment Bug 637985 - Reimplement watchpoints using a JSObject bit
: Reimplement watchpoints using a JSObject bit
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 610034 (view as bug list)
Depends on: 677652
Blocks: 605596 604781
  Show dependency treegraph
 
Reported: 2011-03-01 19:30 PST by Jason Orendorff [:jorendorff]
Modified: 2011-10-18 12:26 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 - passes tests (yikes) (71.53 KB, patch)
2011-04-21 14:05 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (77.89 KB, patch)
2011-04-25 16:21 PDT, Jason Orendorff [:jorendorff]
dvander: review+
Details | Diff | Splinter Review
v3 (81.08 KB, patch)
2011-06-28 14:33 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-03-01 19:30:26 PST
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] 2011-03-01 19:31:18 PST
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] 2011-03-01 19:50:15 PST
Love it. Also, unwatch can revert to best-shared-shape status, not own-shape.

/be
Comment 3 Andreas Gal :gal 2011-03-01 19:55:18 PST
This will be good for the GC too which right now gets incredibly slow if there are any watchpoints installed at all.
Comment 4 Jim Blandy :jimb 2011-03-02 01:54:27 PST
*** Bug 610034 has been marked as a duplicate of this bug. ***
Comment 5 Nicholas Nethercote [:njn] 2011-03-02 22:05:10 PST
I don't know anything about watchpoints but that list in comment 0 sounds great :)
Comment 6 Jason Orendorff [:jorendorff] 2011-04-21 14:05:15 PDT
Created attachment 527646 [details] [diff] [review]
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] 2011-04-25 16:21:19 PDT
Created attachment 528210 [details] [diff] [review]
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] 2011-04-25 17:23:17 PDT
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 2011-06-25 02:49:49 PDT
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] 2011-06-28 14:29:04 PDT
(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] 2011-06-28 14:33:00 PDT
Created attachment 542604 [details] [diff] [review]
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 2011-06-29 21:20:50 PDT
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] 2011-07-01 17:35:10 PDT
> > +    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] 2011-07-26 11:15:09 PDT
(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] 2011-07-27 15:48:06 PDT
hg.mozilla.org/integration/mozilla-inbound/rev/7c43296e7545
Comment 17 Giorgio Maone [:mao] 2011-08-11 16:00:20 PDT
Is there any chance this or a related landing caused bug 677652 ?
Comment 18 Giorgio Maone [:mao] 2011-08-12 12:32:21 PDT
Please document the performance impact of this change and suggest modern/standard alternatives to Object.watch() on MDN, thanks.

Note You need to log in before you can comment on or make changes to this bug.