Closed Bug 1185749 Opened 5 years ago Closed 5 years ago

Add a DynamicTraceable HashMap subclass that can be used with Rooted

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8636269 - Flags: review?(jcoppeard)
Blocks: 1185752
Blocks: 1185755
Updated as per https://bugzilla.mozilla.org/show_bug.cgi?id=1185755#c1.
Attachment #8636269 - Attachment is obsolete: true
Attachment #8636269 - Flags: review?(jcoppeard)
Attachment #8636328 - Flags: review?(jcoppeard)
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?
Attachment #8636328 - Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/mozilla-central/rev/d7b666080438
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.