Last Comment Bug 737258 - GC: pass real addresses to the trace function
: GC: pass real addresses to the trace function
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla14
Assigned To: Terrence Cole [:terrence]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 673454 721557 743406
  Show dependency treegraph
Reported: 2012-03-19 16:50 PDT by Terrence Cole [:terrence]
Modified: 2012-04-09 10:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v0 (36.20 KB, patch)
2012-03-21 12:09 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v1: Much more reasonable. (5.30 KB, patch)
2012-04-05 11:22 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v2: A bit less ambitious. (5.76 KB, patch)
2012-04-05 15:28 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review

Description User image Terrence Cole [:terrence] 2012-03-19 16:50:26 PDT
Currently all Value and jsid's have to pass through the stack before getting passed to the tracer function.  This is because we need to send a raw void* as the target of the callback.  This cannot be a tagged pointer: every trace function doesn't want to have to care if its looking at a Value, id, or just an Object*.  Generally they only want to know the target that is being pointed at.

For our write buffer verifier, we do care about these details.  Passing all values and ids through the stack means that we do not get correct origin information (e.g. the pointer we store in the write buffer) and so cannot easily match against the pointers in the write buffer.

The easiest way to fix this is to make the pointers: void *target and void *location, which encode the target Cell and the location of the thing storing the pointer to the Cell.
Comment 1 User image Terrence Cole [:terrence] 2012-03-21 12:09:03 PDT
Created attachment 608048 [details] [diff] [review]

This turned out to be a bit longer than I had intended, but I think it's fairly sane overall.

The key change is that MarkInternal no longer updates the T* it gets passed, it just returns a new T*, if updated.  Since MarkKind is called by MarkValueInternal I made it have the same interface, so that we can do value updates correctly as well.  I did the same for MarkGCThingRoot, since everywhere we call it we need it to not move and this makes it easier to assert.

There is also one problem I uncovered that we're going to have down the road with the current interface to MarkValue.  Most callers of MarkValue want to just call MarkValue(&obj->foo) and don't care if the value changes.  For Values stored as hash table keys, the move has to be done by the HashTable.  Right now we would just use a temp Value on the stack, but this prevents us from having the location reported correctly to the remembered set verifier.  I think we need a new 2-argument version of MarkValue that does not update the key inline or one that takes the location separately.  I will probably end up folding this into the rekey bug, but I would appreciate your thoughts on the interface.
Comment 2 User image Bill McCloskey (:billm) 2012-04-04 17:44:58 PDT
Comment on attachment 608048 [details] [diff] [review]

I know we talked about changing the marking interface, but now that I see it, I don't really like it.

I do have an idea though. The location parameter is only going to be used in debug builds, since it's only needed by the verifier. So rather than passing it to the tracing callback via a parameter, we could add an extra field to JSTracer for the location. Most of the time it would be null, which would mean that the location from the parameter is correct. But if it's non-null, then the verifier should treat that as the real location.

I think this would be less confusing to people since it allows them to ignore the location unless they really need it.
Comment 3 User image Terrence Cole [:terrence] 2012-04-05 11:22:23 PDT
Created attachment 612621 [details] [diff] [review]
v1: Much more reasonable.

3 files changed, 18 insertions(+), 1 deletions(-)

I considered adding a RAII class for this, but there are only... well, you can see the diffstat.
Comment 4 User image Bill McCloskey (:billm) 2012-04-05 11:33:19 PDT
Comment on attachment 612621 [details] [diff] [review]
v1: Much more reasonable.

Review of attachment 612621 [details] [diff] [review]:

Thanks, this is very close.

::: js/src/jsapi.h
@@ +3239,5 @@
>      size_t              debugPrintIndex;
>      JSBool              eagerlyTraceWeakMaps;
> +#ifdef __cplusplus
> +    js::DebugOnly<void *> realLocation;
> +#endif

I think this will break anyone who uses a tracer from C code. We don't care too much about space here, so let's just make it a normal field.

::: js/src/jsgc.cpp
@@ +1051,5 @@
>      JS_snprintf(nameBuf, sizeof(nameBuf), pattern, thing);
>      JS_SET_TRACING_NAME(trc, nameBuf);
>  #endif
>      void *tmp = thing;
> +    trc->realLocation = (void *)w;

Let's add an inline function somewhere to set this, but only #ifdef DEBUG.

@@ +1056,2 @@
>      MarkKind(trc, &tmp, traceKind);
> +    trc->realLocation = NULL;

Can you have MarkInternal null out realLocation at the end, like it does for debugPrinter and debugPrintArg? Then you can get rid of the other null assignments.

@@ +1104,5 @@
> +            void *tmp = root->thing;
> +            trc->realLocation = &root->thing;
> +            MarkKind(trc, &tmp, root->kind);
> +            trc->realLocation = NULL;
> +            JS_ASSERT(tmp == root->thing);

I don't think we'll be using this code with the verifier, so there's no need to set the real location here AFAIK.
Comment 5 User image Terrence Cole [:terrence] 2012-04-05 15:28:27 PDT
Created attachment 612709 [details] [diff] [review]
v2: A bit less ambitious.

Uglier, but I suppose it matches the rest of JSTracer functionality better this way.
Comment 6 User image Terrence Cole [:terrence] 2012-04-06 13:32:51 PDT
Comment 7 User image Matt Brubeck (:mbrubeck) 2012-04-09 10:10:37 PDT

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