GC: fix up indirect marking internals

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

5 years ago
Depends on: 734250
(Assignee)

Comment 1

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

Comment 3

5 years ago
Previous try looks like it contained the MacOS build bustage from this morning.

https://tbpl.mozilla.org/?tree=Try&rev=825630322f00
(Assignee)

Comment 4

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3cf00d5514d
https://hg.mozilla.org/mozilla-central/rev/c3cf00d5514d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.