Last Comment Bug 730961 - GC: fix up indirect marking internals
: GC: fix up indirect marking internals
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on: 734250
Blocks: 721557
  Show dependency treegraph
 
Reported: 2012-02-27 13:17 PST by Terrence Cole [:terrence]
Modified: 2012-03-17 17:06 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0: don't pass objects through the stack (12.13 KB, patch)
2012-03-15 10:51 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-02-27 13:17:39 PST
Once all of the interfaces are fully indirect, we can carry the indirection all the way through to MarkInternal and avoid the extra stack store/load gymnastics we do currently.
Comment 1 Terrence Cole [:terrence] 2012-03-15 10:51:20 PDT
Created attachment 606288 [details] [diff] [review]
v0: don't pass objects through the stack

For our verifier, it would be nice if the void** we get through our trace function were an exact match for the JSObject** stored in our remembered set.  

Currently, this is not possible because in MarkInternal we store the void* to the stack, then dereference it there.  We need to do this because MarkInternal still takes a void*.  We did this so that we could update for indirection one block of functions at a time, without having to touch other stuff.  Now that this is done (except for 2 functions included in this patch), it is time to push the indirection all the way through to MarkInternal.

Try run is at:
https://tbpl.mozilla.org/?tree=Try&rev=a09d26b0ad1c
Comment 2 Bill McCloskey (:billm) 2012-03-15 11:15:17 PDT
Comment on attachment 606288 [details] [diff] [review]
v0: don't pass objects through the stack

Review of attachment 606288 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome.

::: js/src/jsgc.cpp
@@ +2058,5 @@
>          debugPrinter = elem->debugPrinter;
>          debugPrintArg = elem->debugPrintArg;
>          debugPrintIndex = elem->debugPrintIndex;
>  #endif
> +        MarkKind(this, &elem->thing, elem->kind);

Please assert here that elem->thing doesn't change. Right now elem->thing is just a copy, so the original location won't be updated properly.

@@ +2150,5 @@
>      const char *name = entry.value.name ? entry.value.name : "root";
> +    if (entry.value.type == JS_GC_ROOT_GCTHING_PTR) {
> +        void *tmp = *reinterpret_cast<void **>(entry.key);
> +        MarkGCThingRoot(trc, &tmp, name);
> +        JS_ASSERT(tmp == *reinterpret_cast<void **>(entry.key));

I think you should just be able to call:
  MarkGCThingRoot(trc, reinterpret_cast<void **>(entry.key), name);
No need for the assert.

@@ +2154,5 @@
> +        JS_ASSERT(tmp == *reinterpret_cast<void **>(entry.key));
> +    } else {
> +        Value tmp = *reinterpret_cast<Value *>(entry.key);
> +        MarkValueRoot(trc, &tmp, name);
> +        JS_ASSERT(tmp == *reinterpret_cast<Value *>(entry.key));

I don't think this code needed to change at all.

::: js/src/jsgcmark.cpp
@@ +92,2 @@
>  {
> +    JS_ASSERT(thingp);

Can you copy *thingp to a local variable and use it throughout this function?

@@ +257,4 @@
>  {
>      JS_ROOT_MARKING_ASSERT(trc);
>      JS_SET_TRACING_NAME(trc, name);
> +    if (!thingp || !*thingp)

I don't see any reason to allow thingp to be NULL. Let's just assert.
Comment 3 Terrence Cole [:terrence] 2012-03-15 13:24:53 PDT
Previous try looks like it contained the MacOS build bustage from this morning.

https://tbpl.mozilla.org/?tree=Try&rev=825630322f00
Comment 4 Terrence Cole [:terrence] 2012-03-16 10:24:21 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3cf00d5514d
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-17 17:06:46 PDT
https://hg.mozilla.org/mozilla-central/rev/c3cf00d5514d

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