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]
:
: Jason Orendorff [:jorendorff]
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 User image Andreas Gal :gal 2010-06-14 21:56:41 PDT
Bug 516832 added a conservative stack scanner. Time to start using it.
Comment 1 User image Andreas Gal :gal 2010-06-14 21:57:11 PDT
Created attachment 451218 [details] [diff] [review]
patch
Comment 2 User image Andreas Gal :gal 2010-06-14 21:58:33 PDT
Some gczeal fuzzing for this would be great.
Comment 3 User image 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 User image Jesse Ruderman 2010-06-15 08:15:44 PDT
Is gczeal enough for testing this well, or do we need bug 569451?
Comment 5 User image 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 User image Andreas Gal :gal 2010-07-13 19:08:45 PDT
*** Bug 519947 has been marked as a duplicate of this bug. ***
Comment 7 User image Andreas Gal :gal 2010-07-13 19:09:19 PDT
Created attachment 457217 [details] [diff] [review]
patch
Comment 8 User image 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 User image 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 User image 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 User image 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 User image 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 User image Gregor Wagner [:gwagner] 2010-08-18 11:40:17 PDT
Created attachment 467074 [details] [diff] [review]
patch

rebase and address comments.
Comment 14 User image 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 User image 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 User image 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 User image 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.