GC: manual post barriers for Map/Set Object

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
We cannot store HeapValues in a hashtable.
(Assignee)

Comment 1

5 years ago
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.
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #622927 - Flags: review?(wmccloskey)
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?
Attachment #622927 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

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

Comment 4

5 years ago
Created attachment 622971 [details] [diff] [review]
v1

Your way is indeed much cleaner.
Attachment #622927 - Attachment is obsolete: true
Attachment #622971 - Flags: review?(wmccloskey)
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.
Attachment #622971 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

5 years ago
*sigh*  Looks like I forgot to qref.  Your version is slightly nicer, though, so I'm going to steal it.
(Assignee)

Comment 7

5 years ago
Created attachment 622983 [details] [diff] [review]
v2: With 100% more qrefing that v1.
Attachment #622971 - Attachment is obsolete: true
Attachment #622983 - Flags: review?(wmccloskey)
Comment on attachment 622983 [details] [diff] [review]
v2: With 100% more qrefing that v1.

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

Very nice, thanks!
Attachment #622983 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 9

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