The default bug view has changed. See this FAQ.

TM: remove weak roots (pigeon hole)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: gal, Assigned: gwagner)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
Bug 516832 added a conservative stack scanner. Time to start using it.
(Reporter)

Comment 1

7 years ago
Created attachment 451218 [details] [diff] [review]
patch
Assignee: general → gal
(Reporter)

Comment 2

7 years ago
Some gczeal fuzzing for this would be great.
(Reporter)

Updated

7 years ago
Depends on: 516832

Comment 3

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

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

Comment 5

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

Updated

7 years ago
Duplicate of this bug: 519947
(Reporter)

Comment 7

7 years ago
Created attachment 457217 [details] [diff] [review]
patch
Attachment #451218 - Attachment is obsolete: true
(Reporter)

Comment 8

7 years ago
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.
Attachment #457217 - Flags: review?(anygregor)
(Assignee)

Updated

7 years ago
Attachment #457217 - Flags: review?(anygregor) → review+

Comment 9

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

Updated

7 years ago
Depends on: 574313

Updated

7 years ago
Depends on: 580578
(Assignee)

Comment 10

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

Comment 11

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

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

Comment 13

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

rebase and address comments.
Attachment #457217 - Attachment is obsolete: true
(Assignee)

Comment 14

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

Comment 15

7 years ago
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;
-        }
Attachment #467074 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Assignee: gal → anygregor
(Assignee)

Updated

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

Updated

7 years ago
Blocks: 588522

Comment 16

7 years ago
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.
Attachment #467094 - Flags: review?(igor) → review+
(Assignee)

Updated

7 years ago
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 17

7 years ago
http://hg.mozilla.org/tracemonkey/rev/a009d47505f5

Comment 18

7 years ago
http://hg.mozilla.org/mozilla-central/rev/a009d47505f5
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.