503155 - TM: js_DropScope uses ATOMIC_DECREMENT even during GC (which is single threaded)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

ATOMIC_DECREMENT turns out to be _extremely_ expensive due to the lock prefix. This is a major cost during GC. The attached patch tells js_DropScope whether the gc lock is currently being held, in which case a simple non-atomic decrement is sufficient.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 387508 [details] [diff] [review]
patch

No, even simpler: assert CX_THREAD_IS_RUNNING_GC(cx) and always use the unlocked decrement.

/be

Comment 3

[Andreas Gal :gal]

No, GetMutableScope calls DropScope from outside the GC.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> No, GetMutableScope calls DropScope from outside the GC.

Oops, but that is a different call. It can never destroy a scope, in particular. Instead of adding flag param overhead, specialize just that function.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: suggested simplification to Andreas's patch

Don't want to r+ myself, this is different-looking by enough code, but provably equivalent (check out the comments and assertion).

/be

Comment 6

[Brendan Eich [:brendan]]

Want to get this in soon, scope-related patches stacking up all over.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix to #ifdef JS_THREADSAFE that assertion

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 387683 [details] [diff] [review]
fix to #ifdef JS_THREADSAFE that assertion

This test flunks the assertion in js_GetMutableScope:

// a -> b -> c
function B(){}
var c = B.prototype = {x: 1};
function A(){}
var b = A.prototype = new B;
var a = new A;

// Kill c.
b.__proto__ = null;
c = B.prototype = null;
gc();

a.y = 2;

Comment 9

[Brendan Eich [:brendan]]

Thanks, will fix.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: inline a bit more of js_DropScope into js_GetMutableScope

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 387692 [details] [diff] [review]
inline a bit more of js_DropScope into js_GetMutableScope

Looks good.

Comment 12

[Brendan Eich [:brendan]]

I cloned the

    JS_ASSERT(scope->nrefs > 0);

from js_DropScope into the tail of js_GetMutableScope too.

http://hg.mozilla.org/tracemonkey/rev/51ed66617fb5

/be

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/51ed66617fb5