Last Comment Bug 519949 - TM: remove LocalRootScopes
: TM: remove LocalRootScopes
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Gregor Wagner [:gwagner]
:
:
Mentors:
: 572061 (view as bug list)
Depends on: 519947 572991
Blocks: 519950
  Show dependency treegraph
 
Reported: 2009-10-01 04:39 PDT by Andreas Gal :gal
Modified: 2011-08-12 00:28 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (28.08 KB, patch)
2010-06-18 01:42 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
rebase (28.41 KB, patch)
2010-06-21 14:49 PDT, Gregor Wagner [:gwagner]
igor: review+
Details | Diff | Splinter Review
fix (33.33 KB, patch)
2010-06-22 10:25 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
patch (33.23 KB, patch)
2010-06-22 23:06 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review

Description Andreas Gal :gal 2009-10-01 04:39:19 PDT
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 2009-10-01 10:49:34 PDT
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] 2009-10-01 11:10:46 PDT
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 2010-06-18 01:33:28 PDT
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 2010-06-18 01:42:50 PDT
Created attachment 452210 [details] [diff] [review]
patch

Passes all tests.
Comment 5 Igor Bukanov 2010-06-18 02:04:59 PDT
(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] 2010-06-21 14:49:57 PDT
Created attachment 452860 [details] [diff] [review]
rebase
Comment 7 Andreas Gal :gal 2010-06-21 15:16:33 PDT
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] 2010-06-21 19:04:52 PDT
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 2010-06-22 00:11:52 PDT
Trying moving the TraceMonitor up to the top off JSThreadData.
Comment 10 Andreas Gal :gal 2010-06-22 01:22:42 PDT
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 2010-06-22 01:44:46 PDT
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] 2010-06-22 10:25:47 PDT
Created attachment 453100 [details] [diff] [review]
fix
Comment 13 Brendan Eich [:brendan] 2010-06-22 13:21:45 PDT
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] 2010-06-22 13:33:19 PDT
(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] 2010-06-22 23:06:46 PDT
Created attachment 453297 [details] [diff] [review]
patch

It seems I was chasing some noise yesterday.
Comment 16 Gregor Wagner [:gwagner] 2010-06-23 09:39:25 PDT
http://hg.mozilla.org/tracemonkey/rev/fbf9ceffee06
Comment 18 Luke Wagner [:luke] 2011-08-12 00:28:51 PDT
*** Bug 572061 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.