109183 - JS_GetClass when called indirectly via the GC should not ClaimScope

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brendan Eich [:brendan]]

dbaron found this while jprof'ing new compose window:

                 69 JS_GetClass
                  5 js_FinalizeObject
                  3 js_Interpret
                  2 JS_CloneFunctionObject
                  2 JS_TypeOfValue
                  1 LookupArgOrVar
                  1 JS_GetPrivate
  2241   2       83 js_GetSlotThreadSafe
                 79 ClaimScope
                  2 js_LiveContext

He profiled by opening a bunch of compose windows (~40), so js_LiveContext was
hotter than usual.  Still, the GC_AWARE_GET_SLOT stuff in jsapi.c that I did
based on jband's report of similar unwarranted ClaimScope activity should be
used by JS_GetClass (and js_FinalizeObject should use it, too).

/be

Comment 1

[Brendan Eich [:brendan]]

Patch coming right up.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

js_FinalizeObject can simply use LOCKED_OBJ_GET_CLASS, as the GC stops all
other JS activity that might race badly to update obj->slots while
js_FinalizeObject is trying to load obj->slots[JSSLOT_CLASS].

/be

Comment 3

[John Bandhauer]

Comment on attachment 57139 [details] [diff] [review]
proposed fix

sr=jband

>+#define GC_AWARE_GET_SLOT(cx,obj,slot)  LOCKED_OBJ_GET_SLOT(obj, slot)
could just be?...

#define GC_AWARE_GET_SLOT     LOCKED_OBJ_GET_SLOT

You might update the big comment a bit since this is not only probably a 'mark' 
anymore.

Comment 4

[Brendan Eich [:brendan]]

Attachment: revised comment per jband's review

You can't #define BAR FOO and call BAR(x,y,z) when FOO was #defined to take
only two params, e.g.

Comment beefed up a bit.

/be

Comment 5

[John Bandhauer]

> You can't #define BAR FOO and call BAR(x,y,z) when FOO was #defined to take
> only two params, e.g.

d'oh. I misread. You *can* get rid of a space before "slot" to make it look like 
the lines above. (like it matters :)

Comment 6

[Brendan Eich [:brendan]]

jband: done, thanks (I must have been reading an old-style diff in my haste, I
always try to line things up, given context).

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 57180 [details] [diff] [review]
revised comment per jband's review

r=shaver.

Comment 8

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 9

[Phil Schwartau]

Marking Verified -