572057 - TM: remove weak roots (pigeon hole)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

Bug 516832 added a conservative stack scanner. Time to start using it.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Some gczeal fuzzing for this would be great.

Comment 3

[Igor Bukanov]

As I wrote in bug 516832 comment 220 lets wait at least couple of weeks after the patch hits mozilla central and the corresponding nighties and we know for sure that there are no hard to fix leaks and request model violations (the patch strengthen the asserts in that area).

Comment 4

[Jesse Ruderman]

Is gczeal enough for testing this well, or do we need bug 569451?

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> Is gczeal enough for testing this well, or do we need bug 569451?

gczeal should be enough, but what is really desirable is testing in a browser. I suppose gczeal does not make that fast.

Comment 7

[Andreas Gal :gal]

Attachment: patch

Comment 8

[Andreas Gal :gal]

Comment on attachment 457217 [details] [diff] [review]
patch

2ms speedup on SS, measured with 100 runs and a couple times. Its small but its there.

Comment 9

[Igor Bukanov]

Lets wait with this until at least bugs, exposed by the initial patch for the bug 574313, are fixed. The weak roots may mitigate missing request calls so until we have sound enforcement of that we should not remove them IMO.

Comment 10

[Gregor Wagner [:gwagner]]

We shouldn't forget about this bug! Igor do you think it's time now?

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> We shouldn't forget about this bug! Igor do you think it's time now?

Yes, lets do it now. I have much more confidence in FF codebase following the request model.

Comment 12

[Igor Bukanov]

Comment on attachment 457217 [details] [diff] [review]
patch

Few nits:

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
> JS_PUBLIC_API(void)
> JS_ClearNewbornRoots(JSContext *cx)
> {
>-    JS_CLEAR_WEAK_ROOTS(&cx->weakRoots);
> }

Lets make JS_ClearNewbornRoots a no-op macro in jsapi.h.

>@@ -4647,17 +4644,17 @@ JS_CompileUCFunctionForPrincipals(JSCont
> 
>       out:
>-        cx->weakRoots.finalizableNewborns[FINALIZE_FUNCTION] = fun;
>+        ;
>     }
> 
>   out2:
>     LAST_FRAME_CHECKS(cx, fun);
>     return fun;
> }


Remove out and rename out2 into out here.

> 
> JS_PUBLIC_API(JSFunction *)
>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp
>--- a/js/src/jsarray.cpp
>+++ b/js/src/jsarray.cpp
>@@ -3183,19 +3183,16 @@ js_NewArrayObject(JSContext *cx, jsuint 
>     {
>         AutoValueRooter tvr(cx, obj);
>         if (!InitArrayObject(cx, obj, length, vector, holey))
>             obj = NULL;
>     }
>-
>-    /* Set/clear newborn root, in case we lost it.  */
>-    cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = obj;
>     return obj;

Remove an extra block scope  here together with its scope so the code becomes just
return InitArrayObject(...) ? obj : NULL;


>@@ -3302,18 +3299,16 @@ js_NewArrayObjectWithCapacity(JSContext 
>     AutoValueRooter tvr(cx, obj);
>     if (!obj->ensureDenseArrayElements(cx, capacity, JS_FALSE))
>         obj = NULL;
> 
>-    /* Set/clear newborn root, in case we lost it.  */
>-    cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = obj;
>     if (!obj)
>         return NULL;

Remove that obj = NULL and just return NULL.

Comment 13

[Gregor Wagner [:gwagner]]

Attachment: patch

rebase and address comments.

Comment 14

[Gregor Wagner [:gwagner]]

> 
> 
> >@@ -3302,18 +3299,16 @@ js_NewArrayObjectWithCapacity(JSContext 
> >     AutoValueRooter tvr(cx, obj);
> >     if (!obj->ensureDenseArrayElements(cx, capacity, JS_FALSE))
> >         obj = NULL;
> > 
> >-    /* Set/clear newborn root, in case we lost it.  */
> >-    cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = obj;
> >     if (!obj)
> >         return NULL;
> 
> Remove that obj = NULL and just return NULL.

This function is gone anyway.

I get now assertions in the jsapi-tests. Igor feel free to steal the bug if you know what's going on.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x000000010015eb89 in JS_Assert (s=0x100221b98 "(addr & GC_CHUNK_MASK) < GC_MARK_BITMAP_ARRAY_OFFSET", file=0x1002219c8 "../jsgc.cpp", ln=417) at ../jsutil.cpp:80
80	    *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  0x000000010015eb89 in JS_Assert (s=0x100221b98 "(addr & GC_CHUNK_MASK) < GC_MARK_BITMAP_ARRAY_OFFSET", file=0x1002219c8 "../jsgc.cpp", ln=417) at ../jsutil.cpp:80
#1  0x0000000100088e33 in CheckValidGCThingPtr (thing=0x7fff5fbfda90) at ../jsgc.cpp:417
#2  0x0000000100088eff in JSGCArena::fromGCThing (thing=0x7fff5fbfda90) at ../jsgc.cpp:438
#3  0x000000010007ecf3 in js_GetGCThingRuntime (thing=0x7fff5fbfda90) at ../jsgc.cpp:901
#4  0x000000010013c254 in js_GetStringBytes (cx=0x0, str=0x7fff5fbfda90) at ../jsstr.cpp:4144
#5  0x0000000100010c60 in JS_GetStringBytes (str=0x7fff5fbfda90) at ../jsapi.cpp:5092

Comment 15

[Gregor Wagner [:gwagner]]

Attachment: patch

yeah the pitfalls of saving some lines :)

-        if (str->isAtomized()) {
-            cx->weakRoots.lastAtom = *atomp = STRING_TO_ATOM(str);
+        if (str->isAtomized())
             return true;
-        }

Comment 16

[Igor Bukanov]

Comment on attachment 467094 [details] [diff] [review]
patch

>diff -r 61e1128fb57b js/src/jsinterp.cpp

> BEGIN_CASE(JSOP_ENDINIT)
> {
>     /* Re-set the newborn root to the top of this object tree. */
>     JS_ASSERT(regs.sp - fp->base() >= 1);
>     const Value &lref = regs.sp[-1];
>     JS_ASSERT(lref.isObject());
>-    cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = &lref.toObject();
> }


Nit: replace the comment with FIXME pointing to the bug 588522 and remove lref local using regs.sp[-1] in the assertion directly.

Comment 17

[Gregor Wagner [:gwagner]]

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

Comment 18

[Robert Sayre]

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