519949 - TM: remove LocalRootScopes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

We scan the native stack for reference now, so as long the value is held on the stack somewhere (or in a register), there is no need for local root scopes. We use them in XML only. Its also a horrible API that is error prone. I hope nobody wants to argue in favor of keeping them for engine-external use. 

Igor, if you want to grab this go ahead. I think you are more qualified than me. Otherwise I will get to this in a couple days when the stack scanning patch is in.

Comment 1

[Igor Bukanov]

We cannot just remove the local roots since them could be used to root GC things placed native structures outside the stack. IIRC E4X relies on this.

Comment 2

[Brendan Eich [:brendan]]

Hey, these are based on JNI -- is that horrible too?

Srsly they have drawbacks in misleading developers into not protecting oldborns (no implicit newborn rooting can do that). But that's a problem in general in any exact GC.

As with JSAutoTempValueRooters, Igor is right that you can't assume that conservative stack scanning handles all cases. The stack may not be where the only would-be-strong reference (other than the newborn entry in the local root scope that strengthens the ref) lives.

/be

Comment 3

[Andreas Gal :gal]

Igor, can you please point out the places where e4x uses a lrs to root a heap object that has no stack reference? LRS are blocking my GC re-organization since they mess with the freelists. Patch to remove lrs in a sec. We have to use whatever e4x doesn't do right with them. I would appreciate if you could grab this bug since you know e4x a lot better than me.

Comment 4

[Andreas Gal :gal]

Attachment: patch

Passes all tests.

Comment 5

[Igor Bukanov]

(In reply to comment #3)
> Igor, can you please point out the places where e4x uses a lrs to root a heap
> object that has no stack reference?

Judging by the usage of malloc/free JSXMLArray used as a function local is the only possibility. The array stores namespaces and qnames yet the array itself is not rooted. I think we should just replace that with an auto class that would do all the necessary rooting ideally in a separated bug.

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: rebase

Comment 7

[Andreas Gal :gal]

Please run SS before pushing. I saw slowdowns with some of these patches (not sure why, was weird). LRS removal should really not make anything _slower_.

Comment 8

[Gregor Wagner [:gwagner]]

yeah looks like the removal of localRootStack out of JSThreadData has some negative alignment effect. It is pretty noisy with the standard configuration but if I increase the runs for SS I get a pretty consistent 4 ms (from 447 to 451ms) slowdown.

Comment 9

[Andreas Gal :gal]

Trying moving the TraceMonitor up to the top off JSThreadData.

Comment 10

[Andreas Gal :gal]

Its either TraceMonitor or the dtoa cache (it contains doubles, those might become misaligned). We should definitely figure out whats up here and then assert on proper alignment.

Comment 11

[Igor Bukanov]

Comment on attachment 452860 [details] [diff] [review]
rebase

>diff -r dde588a7831b js/src/jsapi.h
>+/* Obsolete rooting APIs. */
>+#define JS_EnterLocalRootScope(cx) (true)
>+#define JS_LeaveLocalRootScope(cx)
>+#define JS_LeaveLocalRootScopeWithResult(cx, rval)
>+#define JS_ForgetLocalRoot(cx, thing)

Use ((void) 0) as macro expansion here.

>diff -r dde588a7831b js/src/jscntxt.cpp
> JSThreadData::purgeGCFreeLists()
> {
>-    if (!localRootStack) {
>-        gcFreeLists.purge();
>-    } else {
>-        JS_ASSERT(gcFreeLists.isEmpty());
>-        localRootStack->gcFreeLists.purge();
>-    }
>+    gcFreeLists.purge();
> }

Nit: remove the method and use gcFreeLists.purge() at the call sites.

>diff -r dde588a7831b js/src/jscntxt.h
>@@ -1006,19 +983,16 @@ struct JSThreadData {
>-    /* Optional stack of heap-allocated scoped local GC roots. */
>-    JSLocalRootStack    *localRootStack;

Just replace the field with void *unused and file a new bug to find out the optimal alignment here. 

>diff -r dde588a7831b js/src/jsgc.cpp
> static inline JSGCFreeLists *
> GetGCFreeLists(JSContext *cx)
> {
>     JSThreadData *td = JS_THREAD_DATA(cx);
>-    if (!td->localRootStack)
>-        return &td->gcFreeLists;
>-    JS_ASSERT(td->gcFreeLists.isEmpty());
>-    return &td->localRootStack->gcFreeLists;
>+    return &td->gcFreeLists;

Nit: remove this function and use JS_THREAD_DATA(cx)->gcFreeLists at call sites.

>diff -r dde588a7831b js/src/jsxml.cpp
>--- a/js/src/jsxml.cpp	Mon Jun 21 15:27:48 2010 -0500
>+++ b/js/src/jsxml.cpp	Mon Jun 21 14:46:46 2010 -0700
>@@ -86,16 +86,32 @@ using namespace js;
>  *
>  * TODO
>  * - XXXbe patrol
>  * - Fuse objects and their JSXML* private data into single GC-things
>  * - fix function::foo vs. x.(foo == 42) collision using proper namespacing
>  * - JSCLASS_DOCUMENT_OBSERVER support -- live two-way binding to Gecko's DOM!
>  */
> 
>+static inline bool
>+js_EnterLocalRootScope(JSContext *cx)
>+{
>+    return true;
>+}
>+
>+static inline void
>+js_LeaveLocalRootScope(JSContext *cx)
>+{
>+}
>+
>+static inline void
>+js_LeaveLocalRootScopeWithResult(JSContext *cx, jsval rval)
>+{
>+}

Nit: file a new bug to eliminate these calls from jsxml.cpp.

Comment 12

[Gregor Wagner [:gwagner]]

Attachment: fix

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 453100 [details] [diff] [review]
fix

>@@ -1003,22 +980,22 @@ struct JSThreadData {
>     bool                waiveGCQuota;
> 
>     /*
>      * The GSN cache is per thread since even multi-cx-per-thread embeddings
>      * do not interleave js_GetSrcNote calls.
>      */
>     JSGSNCache          gsnCache;
> 
>+	/* Alignment. Will be removed */
>+	void 				*unused;

No hard tabs in files!

:-P

/be

>+
>     /* Property cache for faster call/get/set invocation. */
>     js::PropertyCache   propertyCache;
> 
>-    /* Optional stack of heap-allocated scoped local GC roots. */
>-    JSLocalRootStack    *localRootStack;

Comment 14

[Gregor Wagner [:gwagner]]

(In reply to comment #13)
> (From update of attachment 453100 [details] [diff] [review])
> >@@ -1003,22 +980,22 @@ struct JSThreadData {
> >     bool                waiveGCQuota;
> > 
> >     /*
> >      * The GSN cache is per thread since even multi-cx-per-thread embeddings
> >      * do not interleave js_GetSrcNote calls.
> >      */
> >     JSGSNCache          gsnCache;
> > 
> >+	/* Alignment. Will be removed */
> >+	void 				*unused;
> 
> No hard tabs in files!
> 
> :-P
> 
> /be
> 

uuhhh thx!
I blame it on my new laptop :)

Comment 15

[Gregor Wagner [:gwagner]]

Attachment: patch

It seems I was chasing some noise yesterday.

Comment 16

[Gregor Wagner [:gwagner]]

http://hg.mozilla.org/tracemonkey/rev/fbf9ceffee06

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/fbf9ceffee06