477319 - Some objects left locked in tracer code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Found while running parallel raytrace with JIT'd workers, we're not always unlocking objects when we should. Patch attached with a bit of debug-only stuff that I had to fix for logging. The real fix is in jsinterp.h and jstracer.cpp.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 360979 [details] [diff] [review]
Patch, v1

>diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h
>--- a/js/src/jsinterp.h
>+++ b/js/src/jsinterp.h
>@@ -332,17 +332,20 @@ js_FillPropertyCache(JSContext *cx, JSOb
>                 OBJ_IS_NATIVE(tmp_)) {                                        \
>                 JS_UNLOCK_OBJ(cx, pobj);                                      \
>                 pobj = tmp_;                                                  \
>                 JS_LOCK_OBJ(cx, pobj);                                        \
>             }                                                                 \
>             if (PCVCAP_SHAPE(entry->vcap) == OBJ_SHAPE(pobj)) {               \
>                 PCMETER(cache_->pchits++);                                    \
>                 PCMETER(!PCVCAP_TAG(entry->vcap) || cache_->protopchits++);   \
>-                pobj = OBJ_SCOPE(pobj)->object;                               \
>+                tmp_ = OBJ_SCOPE(pobj)->object;                               \
>+                JS_UNLOCK_OBJ(cx, pobj);                                      \
>+                pobj = tmp_;                                                  \
>+                JS_LOCK_OBJ(cx, pobj);                                        \

This is unnecessary, please revert. The title-lock lives in the scope, which if pobj != tmp_ is shared between the two objects (so no need to dance). If pobj == tmp_ then definitely no need to juggle locks. And juggling opens a race by unlocking prematurely.

>+++ b/js/src/jsscope.cpp
>@@ -171,31 +171,31 @@ js_NewScope(JSContext *cx, jsrefcount nr
> #endif
>     JS_RUNTIME_METER(cx->runtime, liveScopes);
>     JS_RUNTIME_METER(cx->runtime, totalScopes);
>     return scope;
> }
> 
> #ifdef DEBUG_SCOPE_COUNT
> extern void
>-js_unlog_scope(JSScope *scope);
>+js_unlog_title(JSTitle *title);
> #endif
> 
> #if defined DEBUG || defined JS_DUMP_PROPTREE_STATS
> # include "jsprf.h"
> # define LIVE_SCOPE_METER(cx,expr) JS_LOCK_RUNTIME_VOID(cx->runtime,expr)
> #else
> # define LIVE_SCOPE_METER(cx,expr) /* nothing */
> #endif
> 
> void
> js_DestroyScope(JSContext *cx, JSScope *scope)
> {
> #ifdef DEBUG_SCOPE_COUNT
>-    js_unlog_scope(scope);
>+    js_unlog_title(&scope->title);
> #endif

No need for this now that you are unlogging title not scope -- hide it all in jslock.cpp, don't export js_unlog_title, just do the deed in js_FinishTitle:
> 
> #ifdef JS_THREADSAFE
>     js_FinishTitle(cx, &scope->title);
> #endif
 . . .
>+++ b/js/src/jstracer.cpp
>@@ -5557,16 +5557,20 @@ TraceRecorder::test_property_cache(JSObj
>             pcval = PCVAL_NULL;
>             ABORT_TRACE("failed to find property");
>         }
> 
>         OBJ_DROP_PROPERTY(cx, obj2, prop);
>         if (!entry)
>             ABORT_TRACE("failed to fill property cache");
>     }
>+    else {

Cuddle } else { to follow prevailing style (after K&R).

>+        // Null atom means that obj2 is locked and must now be unlocked.
>+        JS_UNLOCK_OBJ(cx, obj2);
>+    }

The "then" clause for whose "if" this is the "else" is big. Suggest inverting to test "if (!atom)" and making this then "then" and the bigger junk (the property cache miss case, which we hope is rare) the "else" (PGO can figure out which way is better based on profile data).

>diff --git a/js/src/jsutil.h b/js/src/jsutil.h
>--- a/js/src/jsutil.h
>+++ b/js/src/jsutil.h
>@@ -156,13 +156,15 @@ struct JSCallsite {
>     JSCallsite  *parent;
>     JSCallsite  *siblings;
>     JSCallsite  *kids;
>     void        *handy;
> };
> 
> extern JSCallsite *JS_Backtrace(int skip);
> 
>+extern void JS_DumpBacktrace(JSCallsite *trace);

These should both be JS_FRIEND_API (prototypes and definitions in jsutil.cpp of course) -- please wrap the prototypes to match prevailing style while you are at it.

/be

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.1

Ok, with those changes.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 360984 [details] [diff] [review]
Patch v1.1

Great, thanks!

/be

Comment 4

[Brendan Eich [:brendan]]

This should block.

/be

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 70080b9e0be0 to tracemonkey.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/70080b9e0be0

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is the patch already checked into the 1.9.1 branch?

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to comment #7)
> Is the patch already checked into the 1.9.1 branch?

no. is there any reason you changed the target milestone?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It is blocking 1.9.1, must land there before we ship.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> It is blocking 1.9.1, must land there before we ship.

True. This is indicated by the blocking1.9.1+ flag and has nothing to do with the Target Milestone. The latter one gives us the information about the most recent version this patch has been landed for. And this is mozilla1.9.2a1 right now. See the in-length discussion in md.planning. I talked with sayer on IRC and he agreed the change I've made here.

With the current status we accidentally identify that this bug has been already fixed on 1.9.1 while it isn't. IMO that's a bad praxis and makes it hard to search for 1.9.1 branch fixes. Please use the fixed1.9.1 keyword to indicate that it has been fixed on the branch.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #10)
> With the current status we accidentally identify that this bug has been already
> fixed on 1.9.1 while it isn't.

Then that's just bad search. I don't care one way or another but we've always used the flags to indicate fixed-on-branch.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ecb1c8ef066a