745326 - GC: manual post barriers for Map/Set Object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

We cannot store HeapValues in a hashtable.

Comment 1

[Terrence Cole [:terrence]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

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

Attachment: v1

Your way is indeed much cleaner.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

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

Attachment: v2: With 100% more qrefing that v1.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd6ed757687

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/0cd6ed757687