Note: There are a few cases of duplicates in user autocompletion which are being worked on.

TM: remove LocalRootScopes

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: gal, Assigned: gwagner)

Tracking

(Blocks: 1 bug)

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Blocks: 519950

Comment 1

8 years ago
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
(Reporter)

Comment 3

7 years ago
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.
(Reporter)

Comment 4

7 years ago
Created attachment 452210 [details] [diff] [review]
patch

Passes all tests.
(Reporter)

Updated

7 years ago
Attachment #452210 - Flags: review?(igor)

Comment 5

7 years ago
(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.

Updated

7 years ago
Depends on: 572991
(Assignee)

Updated

7 years ago
Assignee: general → anygregor
(Assignee)

Comment 6

7 years ago
Created attachment 452860 [details] [diff] [review]
rebase
Attachment #452210 - Attachment is obsolete: true
Attachment #452210 - Flags: review?(igor)
(Reporter)

Comment 7

7 years ago
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_.
(Assignee)

Comment 8

7 years ago
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.
(Reporter)

Comment 9

7 years ago
Trying moving the TraceMonitor up to the top off JSThreadData.
(Reporter)

Comment 10

7 years ago
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

7 years ago
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+
(Assignee)

Comment 12

7 years ago
Created attachment 453100 [details] [diff] [review]
fix
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;
(Assignee)

Comment 14

7 years ago
(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 :)
(Assignee)

Comment 15

7 years ago
Created attachment 453297 [details] [diff] [review]
patch

It seems I was chasing some noise yesterday.
Attachment #453100 - Attachment is obsolete: true
(Assignee)

Comment 16

7 years ago
http://hg.mozilla.org/tracemonkey/rev/fbf9ceffee06
(Assignee)

Updated

7 years ago
Whiteboard: fixed-in-tracemonkey

Comment 17

7 years ago
http://hg.mozilla.org/mozilla-central/rev/fbf9ceffee06
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

6 years ago
Duplicate of this bug: 572061
You need to log in before you can comment on or make changes to this bug.