Last Comment Bug 572057 - TM: remove weak roots (pigeon hole)
: TM: remove weak roots (pigeon hole)
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Gregor Wagner [:gwagner]
:
:
Mentors:
: 519947 (view as bug list)
Depends on: 516832 574313 580578
Blocks: 588522
  Show dependency treegraph
 
Reported: 2010-06-14 21:56 PDT by Andreas Gal :gal
Modified: 2010-08-23 14:50 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (19.60 KB, patch)
2010-06-14 21:57 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (17.92 KB, patch)
2010-07-13 19:09 PDT, Andreas Gal :gal
anygregor: review+
Details | Diff | Splinter Review
patch (18.48 KB, patch)
2010-08-18 11:40 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
patch (18.37 KB, patch)
2010-08-18 12:19 PDT, Gregor Wagner [:gwagner]
igor: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2010-06-14 21:56:41 PDT
Bug 516832 added a conservative stack scanner. Time to start using it.
Comment 1 Andreas Gal :gal 2010-06-14 21:57:11 PDT
Created attachment 451218 [details] [diff] [review]
patch
Comment 2 Andreas Gal :gal 2010-06-14 21:58:33 PDT
Some gczeal fuzzing for this would be great.
Comment 3 Igor Bukanov 2010-06-15 00:52:18 PDT
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 2010-06-15 08:15:44 PDT
Is gczeal enough for testing this well, or do we need bug 569451?
Comment 5 Igor Bukanov 2010-06-15 16:11:37 PDT
(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 6 Andreas Gal :gal 2010-07-13 19:08:45 PDT
*** Bug 519947 has been marked as a duplicate of this bug. ***
Comment 7 Andreas Gal :gal 2010-07-13 19:09:19 PDT
Created attachment 457217 [details] [diff] [review]
patch
Comment 8 Andreas Gal :gal 2010-07-13 19:09:57 PDT
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 2010-07-14 03:09:14 PDT
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] 2010-08-17 15:38:53 PDT
We shouldn't forget about this bug! Igor do you think it's time now?
Comment 11 Igor Bukanov 2010-08-18 01:46:10 PDT
(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 2010-08-18 02:01:25 PDT
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] 2010-08-18 11:40:17 PDT
Created attachment 467074 [details] [diff] [review]
patch

rebase and address comments.
Comment 14 Gregor Wagner [:gwagner] 2010-08-18 11:43:57 PDT
> 
> 
> >@@ -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] 2010-08-18 12:19:46 PDT
Created attachment 467094 [details] [diff] [review]
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 2010-08-18 12:34:03 PDT
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] 2010-08-18 12:49:54 PDT
http://hg.mozilla.org/tracemonkey/rev/a009d47505f5

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