Note: There are a few cases of duplicates in user autocompletion which are being worked on.

GC: fix up indirect marking internals

RESOLVED FIXED in mozilla14



JavaScript Engine
6 years ago
5 years ago


(Reporter: terrence, Assigned: terrence)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)



6 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.


5 years ago
Depends on: 734250

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:
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]:


::: 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 = ? : "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_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+

Comment 3

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

Comment 4

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