Closed Bug 519949 Opened 10 years ago Closed 9 years ago

TM: remove LocalRootScopes

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gwagner)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 519950
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.
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
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.
Attached patch patch (obsolete) — Splinter Review
Passes all tests.
Attachment #452210 - Flags: review?(igor)
(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.
Depends on: 572991
Assignee: general → anygregor
Attached patch rebase (obsolete) — Splinter Review
Attachment #452210 - Attachment is obsolete: true
Attachment #452210 - Flags: review?(igor)
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_.
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.
Trying moving the TraceMonitor up to the top off JSThreadData.
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 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.
Attachment #452860 - Flags: review+
Attached patch fix (obsolete) — Splinter Review
Attachment #452860 - Attachment is obsolete: true
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;
(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 :)
Attached patch patchSplinter Review
It seems I was chasing some noise yesterday.
Attachment #453100 - Attachment is obsolete: true
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/fbf9ceffee06
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 572061
You need to log in before you can comment on or make changes to this bug.