105571 - gc marking forces heavyweight lock creation

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[John Bandhauer]

fun_mark calls JS_GetPrivate which calls js_GetSlotThreadSafe. Even though we 
are safely in singlethreaded gc we treat this like any other access. So, if we 
are marking a function created on a JSContext other than the one on which gc 
occurs then we force the creation of a heavyweight lock for the function object 
(and are stuck with it for the duration).

I'm thinking that fun_mark can avoid all the checks and pretty much go straight 
to the slot. We know we are serialized thorugh the current thread (though not 
actually locked). And we know this is a native object with a private slot. 
Right?

Comment 1

[Brendan Eich [:brendan]]

Right, but other JSClass.mark impls would need to go straight to the slot, via
jsobj.h include, and violate the API boundary.

How about we make the public API's slot-getters (JS_GetPrivate, JS_GetPrototype,
JS_GetParent), #ifdef JS_THREADSAFE, go straight to the slot if (rt->gcRunning
&& rt->gcThread == cx->thread)?

/be

Comment 2

[John Bandhauer]

and OBJ_IS_NATIVE or course.

Comment 3

[Brendan Eich [:brendan]]

Attachment: retracted, jband pointed out that OBJ_IS_NATIVE should be tested first

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 5

[John Bandhauer]

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

sr=jband
Looks right to me.
You're skipping the OBJ_CHECK_SLOT. Are we going to just trust that
on one misuses this macro in the future?

I'm doing a Quantify run now. I'll post results.

Comment 6

[John Bandhauer]

I ran it. Nothing bad happened. I opened the browser, went to one page, closed 
the browser.

With and without this patch there are about 60 calls to fun_mark. 

Without the patch there are a total of 26k calls to js_GetSlotThreadSafe.
With the patch there are a total of only 255 calls to js_GetSlotThreadSafe.

Total PR_Lock (and PR_Unlock) call count goes down from 126K to 111K.

Very cool.

Comment 7

[Brendan Eich [:brendan]]

Macro is local to jsapi.c -- if you're hacking there, you are trusted.  Ok, who
will r= the patch?

/be

Comment 8

[Johnny Stenback (:jst)]

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

r=jst, looks right to me.

Comment 9

[Brendan Eich [:brendan]]

Fixed, thanks!

/be

Comment 10

[Phil Schwartau]

Marking Verified -