Last Comment Bug 745326 - GC: manual post barriers for Map/Set Object
: GC: manual post barriers for Map/Set Object
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Terrence Cole [:terrence]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 744880
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 13:55 PDT by Terrence Cole [:terrence]
Modified: 2012-05-11 11:41 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0: no manual post barriers were used in the making of this patch. (7.43 KB, patch)
2012-05-10 14:40 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v1 (7.61 KB, patch)
2012-05-10 16:49 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v2: With 100% more qrefing that v1. (7.31 KB, patch)
2012-05-10 17:29 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-04-13 13:55:40 PDT
We cannot store HeapValues in a hashtable.
Comment 1 Terrence Cole [:terrence] 2012-05-10 14:40:46 PDT
Created attachment 622927 [details] [diff] [review]
v0: no manual post barriers were used in the making of this patch.

This patch makes Map and Set use the new RelocatableValue.  Since we don't need to do manual postbarriering anymore, this was fairly straightforward.

The only difficulty is how hard it is to construct temporary HashableValues.  I've gotten around this by adding a private constructor (with aggressive assertions) and friending it to Map/Set.  This cuts ~5 lines out of both Map and Set's marking and, I believe, makes them both much clearer.
Comment 2 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-10 14:54:48 PDT
Comment on attachment 622927 [details] [diff] [review]
v0: no manual post barriers were used in the making of this patch.

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

::: js/src/builtin/MapObject.cpp
@@ +188,5 @@
>  {
>      MapObject *mapobj = static_cast<MapObject *>(obj);
>      if (ValueMap *map = mapobj->getData()) {
> +        for (ValueMap::Enum iter(*map); !iter.empty(); iter.popFront()) {
> +            gc::MarkValue(trc, &((RelocatableValue &)iter.front().value), "value");

Why is the cast needed here?

@@ +194,5 @@
> +            Value key = iter.front().key.value.get();
> +            JS_SET_TRACING_LOCATION(trc, (void *)&iter.front().key);
> +            gc::MarkValueUnbarriered(trc, &key, "key");
> +            if (key != iter.front().key.value.get())
> +                iter.rekeyFront(HashableValue(key));

I think this would be cleaner if you add a mark method to HashableValue. Then you wouldn't need to expose the constructor. Also, the code could be shared between Set and Map.

::: js/src/builtin/MapObject.h
@@ +77,5 @@
>      };
>  
>      HashableValue() : value(UndefinedValue()) {}
>  
> +    operator const RelocatableValue &() const { return value; }

Where is this used?
Comment 3 Terrence Cole [:terrence] 2012-05-10 16:02:48 PDT
(In reply to Bill McCloskey (:billm) from comment #2)
> ::: js/src/builtin/MapObject.cpp
> @@ +188,5 @@
> >  {
> >      MapObject *mapobj = static_cast<MapObject *>(obj);
> >      if (ValueMap *map = mapobj->getData()) {
> > +        for (ValueMap::Enum iter(*map); !iter.empty(); iter.popFront()) {
> > +            gc::MarkValue(trc, &((RelocatableValue &)iter.front().value), "value");
> 
> Why is the cast needed here?

I think that is a holdover from the in-place key marking, subsequently copied to the value marker, then faithfully updated with the type change to RelocatableValue.  It always astonishes me that I can rework a patch five or more times without catching something so basic and obvious.
 
> @@ +194,5 @@
> > +            Value key = iter.front().key.value.get();
> > +            JS_SET_TRACING_LOCATION(trc, (void *)&iter.front().key);
> > +            gc::MarkValueUnbarriered(trc, &key, "key");
> > +            if (key != iter.front().key.value.get())
> > +                iter.rekeyFront(HashableValue(key));
> 
> I think this would be cleaner if you add a mark method to HashableValue.
> Then you wouldn't need to expose the constructor. Also, the code could be
> shared between Set and Map.

Good idea!

> ::: js/src/builtin/MapObject.h
> @@ +77,5 @@
> >      };
> >  
> >      HashableValue() : value(UndefinedValue()) {}
> >  
> > +    operator const RelocatableValue &() const { return value; }
> 
> Where is this used?

Looks like it's not.  I'll rip it out.
Comment 4 Terrence Cole [:terrence] 2012-05-10 16:49:28 PDT
Created attachment 622971 [details] [diff] [review]
v1

Your way is indeed much cleaner.
Comment 5 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-10 16:56:54 PDT
Comment on attachment 622971 [details] [diff] [review]
v1

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

::: js/src/builtin/MapObject.cpp
@@ +158,5 @@
> +{
> +    Value tmp = value.get();
> +    JS_SET_TRACING_LOCATION(trc, (void *)&value);
> +    gc::MarkValueUnbarriered(trc, &tmp, "key");
> +    return HashableValue(tmp);

My thinking was that this could instead read:

HashableValue hv(*this);
JS_SET_TRACING_LOCATION(trc, (void *)this);
gc::MarkValueUnbarriered(trc, &hv.value, "key");
return hv;

Then you wouldn't need the constructor for HashableValue.
Comment 6 Terrence Cole [:terrence] 2012-05-10 17:26:18 PDT
*sigh*  Looks like I forgot to qref.  Your version is slightly nicer, though, so I'm going to steal it.
Comment 7 Terrence Cole [:terrence] 2012-05-10 17:29:30 PDT
Created attachment 622983 [details] [diff] [review]
v2: With 100% more qrefing that v1.
Comment 8 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-10 17:31:20 PDT
Comment on attachment 622983 [details] [diff] [review]
v2: With 100% more qrefing that v1.

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

Very nice, thanks!
Comment 9 Terrence Cole [:terrence] 2012-05-10 17:39:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd6ed757687
Comment 10 Matt Brubeck (:mbrubeck) 2012-05-11 11:41:38 PDT
https://hg.mozilla.org/mozilla-central/rev/0cd6ed757687

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