1185749 - Add a DynamicTraceable HashMap subclass that can be used with Rooted

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: add_traceable_hashmap_support-v0.diff

This was a bit less straightforward than I was hoping, but I'm happy enough with the result to proceed. The one wart is that Rooted with only a cx wants to call T() to construct, which totally don't work since you have to pass the Allocator. This means that you have to do root(cx, Move(MyHashMap(cx))). On the other hand, this is not substantially more onerous than making the constructor explicit, which I've gone ahead and done. On balance, particularly considering |auto|, I think this is a great tradeoff.

I've got some nice patches simplifying CloneMemory and FieldInfoHash following this in my queue, so I think I've even got all the major kinks worked out.

Comment 1

[Terrence Cole [:terrence]]

Attachment: add_traceable_hashmap_support-v1.diff

Updated as per https://bugzilla.mozilla.org/show_bug.cgi?id=1185755#c1.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8636328 [details] [diff] [review]
add_traceable_hashmap_support-v1.diff

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

This is really nice.

::: js/public/RootingAPI.h
@@ +677,1 @@
>      }

What you want here is a universal reference so you don't need to two separate construtors to take lvalue and rvalue references:


    template <typename S>
    Rooted(js::ContextFriendFields* cx, S&& initial
           MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
      : ptr(mozilla::Forward<S>(initial))
    {
        MOZ_GUARD_OBJECT_NOTIFIER_INIT;
        registerWithRootLists(cx);
    }

::: js/src/jsapi-tests/testGCExactRooting.cpp
@@ +163,5 @@
> +using MyHashMap = js::TraceableHashMap<js::Shape*, JSObject*>;
> +
> +BEGIN_TEST(testGCRootedHashMap)
> +{
> +    JS::Rooted<MyHashMap> map(cx, Move(MyHashMap(cx)));

I don't get why you need Move() here, and it works fine if I take it out.

@@ +169,5 @@
> +    CHECK(map.initialized());
> +
> +    for (size_t i = 0; i < 10; ++i) {
> +        RootedObject obj(cx, JS_NewObject(cx, nullptr));
> +        CHECK(map.putNew(obj->as<NativeObject>().lastProperty(), obj));

Isn't lastProperty() going to be the same shape every time here?

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b666080438

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7b666080438