Closed Bug 820390 Opened 8 years ago Closed 8 years ago

Implement AutoHashMapRooter and AutoObjectObjectHashMap

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: till, Unassigned)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file, 1 obsolete file)

For bug 784293, I need a rootable <JSObject*, JSOBject*> hashmap. This is the bug for implementing it.
And this is the patch implementing it. I hope it's not too ridiculously wrong.
Attachment #690876 - Flags: review?(terrence)
Comment on attachment 690876 [details] [diff] [review]
Implement AutoHashMapRooter and AutoObjectObjectHashMap. r=

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

This looks just about perfect, modulo a couple nits.

::: js/src/gc/RootMarking.cpp
@@ +532,5 @@
>        }
>  
> +      case OBJOBJHASHMAP: {
> +        AutoObjectObjectHashMap::HashMapImpl &map = static_cast<AutoObjectObjectHashMap *>(this)->map;
> +        for (AutoObjectObjectHashMap::Range r = map.all(); !r.empty(); r.popFront()) {

Make this an Enum instead of a Range so that we can handle key movement later without needing to update this type.

@@ +533,5 @@
>  
> +      case OBJOBJHASHMAP: {
> +        AutoObjectObjectHashMap::HashMapImpl &map = static_cast<AutoObjectObjectHashMap *>(this)->map;
> +        for (AutoObjectObjectHashMap::Range r = map.all(); !r.empty(); r.popFront()) {
> +            MarkObjectRoot(trc, (JSObject **) &r.front().key, "AutoObjectObjectHashMap key");

Store r.front().key before marking it and JS_ASSERT that the value is unchanged after marking. This will very quickly remind us to revisit this spot when we turn on moving GC.

@@ +534,5 @@
> +      case OBJOBJHASHMAP: {
> +        AutoObjectObjectHashMap::HashMapImpl &map = static_cast<AutoObjectObjectHashMap *>(this)->map;
> +        for (AutoObjectObjectHashMap::Range r = map.all(); !r.empty(); r.popFront()) {
> +            MarkObjectRoot(trc, (JSObject **) &r.front().key, "AutoObjectObjectHashMap key");
> +            MarkObjectRoot(trc, (JSObject **) &r.front().value, "AutoObjectObjectHashMap value");

This explicit cast should not be necessary after you update the types.

::: js/src/jsapi.h
@@ +1041,5 @@
>          JS_ASSERT(this == *stackTop);
>          *stackTop = down;
>      }
>  
> +    /* Implemented in gc/RootMarking.cpp. */

\o/  Nice catch!

@@ +1351,5 @@
> +    typedef typename HashMapImpl::AddPtr AddPtr;
> +
> +    bool init(uint32_t len = 16) {
> +        return map.init(len);
> +        //FIXME: make GC safe

You don't need to MakeRangeGCSafe for heap storage structures, so it's safe to remove this note.

::: js/src/jscntxt.h
@@ +2143,5 @@
>  
>      JS_DECL_USE_GUARD_OBJECT_NOTIFIER
>  };
>  
> +class AutoObjectObjectHashMap : public AutoHashMapRooter<JSObject *, JSObject *>

These GC thing pointers are stored on the Heap, so they need to be wrapped in one of the types in the HeapPtr<T> class hierarchy. The KeyType should be EncapsulatedPtrObject and the ValueType should RelocatablePtrObject. I can explain why on IRC if you are interested.

@@ +2147,5 @@
> +class AutoObjectObjectHashMap : public AutoHashMapRooter<JSObject *, JSObject *>
> +{
> +  public:
> +    explicit AutoObjectObjectHashMap(JSContext *cx
> +                              JS_GUARD_OBJECT_NOTIFIER_PARAM)

Make JS_GUARD... line up with JSContext.

@@ +2148,5 @@
> +{
> +  public:
> +    explicit AutoObjectObjectHashMap(JSContext *cx
> +                              JS_GUARD_OBJECT_NOTIFIER_PARAM)
> +        : AutoHashMapRooter<JSObject *, JSObject *>(cx, OBJOBJHASHMAP)

2 space indent before the :
Attachment #690876 - Flags: review?(terrence) → review+
Attachment #690876 - Attachment is obsolete: true
Comment on attachment 691995 [details] [diff] [review]
Implement AutoHashMapRooter and AutoObjectObjectHashMap. r=

As discussed on IRC, I implemented your feedback and changed the Auto*Vectors to use raw* types. I think that a quick interdiff review should be sufficient.
Attachment #691995 - Flags: review?(terrence)
Comment on attachment 691995 [details] [diff] [review]
Implement AutoHashMapRooter and AutoObjectObjectHashMap. r=

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

Looks good!
Attachment #691995 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/b80437be0e70
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.