1273220 - Rename RelocatablePtr to HeapPtr

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 00_rename_RelocatablePtr_to_GCPtr-v0.diff

I've been putting this off way too long. Turns out it was a total molehill: there are only about a hundred instances and it took very little time to change everything over manually.

There are a couple caveats in this patch. I went to change our typedefs and found myself changing RelocatableValue to GCValue, which strikes me as just very, very wrong. I figured I would try out just removing the typedef for Value; when I realized how few instances there are, I figured I would just ditch the typedefs and see how it looks. In my opinion, GCPtr<T> is short enough that the sans-typedefs version clarifies more than it hinders. There is really only one case where I think it looks terrible: buried inside a Map<Hasher<GCPtr<T>>>. That said, I don't think the handful of these cases are worth the cognitive burden of having two names for GCPtr<T>.

I've flagged the entire GC team for review here. Consider this an RFC with intent to land unless anyone has substantial reservations.

Comment 1

[Terrence Cole [:terrence]]

Okay new plan!

HeapPtr -> GCPtr (use templates for Value/Id and typedefs elsewhere)
RelocatablePtr -> HeapPtr (similar semantics to Heap<T>)

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8752970 [details] [diff] [review]
00_rename_RelocatablePtr_to_GCPtr-v0.diff

Cancelling review as we have a new plan now.

Comment 3

[Terrence Cole [:terrence]]

Attachment: 04_rename_RelocatablePtr_to_HeapPtr-v1.diff

As we discussed. This now goes after the HeapPtr->GCPtr change, obviously.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8754020 [details] [diff] [review]
04_rename_RelocatablePtr_to_HeapPtr-v1.diff

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

::: js/src/asmjs/WasmModule.h
@@ -484,5 @@
>          {}
>      };
>      typedef Vector<FuncPtrTable, 0, SystemAllocPolicy> FuncPtrTableVector;
>      typedef Vector<CacheableChars, 0, SystemAllocPolicy> FuncLabelVector;
> -    typedef RelocatablePtrArrayBufferObjectMaybeShared BufferPtr;

This existed? Wow.

The templatized name feels so much more manageable for this.

::: js/src/gc/Barrier.h
@@ +142,5 @@
>   *  |  |  |  | PreBarriered  provides pre-barriers only
>   *  |  |  |  |
>   *  |  |  | GCPtr          provides pre- and post-barriers
>   *  |  |  |
> + *  |  | HeapPtr             provides pre- and post-barriers and is relocatable

It would mess up the formatting, but "...is relocatable and deletable" would be nice.

@@ -910,5 @@
> -typedef RelocatablePtr<JSLinearString*> RelocatablePtrLinearString;
> -typedef RelocatablePtr<JSString*> RelocatablePtrString;
> -typedef RelocatablePtr<JSAtom*> RelocatablePtrAtom;
> -typedef RelocatablePtr<ArrayBufferObjectMaybeShared*> RelocatablePtrArrayBufferObjectMaybeShared;
> -

I am not sad to see these go.

::: js/src/jsweakmap.h
@@ +377,5 @@
>  {
>    public:
>      ObjectValueMap(JSContext* cx, JSObject* obj)
> +      : WeakMap<HeapPtr<JSObject*>, HeapPtr<Value>,
> +                MovableCellHasher<HeapPtr<JSObject*>>>(cx, obj)

You've unlocked the three >>> achievement. Can you do four?

::: js/src/vm/DebuggerMemory.cpp
@@ +233,5 @@
>          result->setDenseElement(i, ObjectValue(*obj));
>  
>          // Pop the front queue entry, and delete it immediately, so that the GC
> +        // sees the AllocationsLogEntry's HeapPtr barriers run atomically with
> +        // the change to the graph (the queeue link).

while you're here, "queeue".

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f18e46bce0a40e6b73435efb633c50a38fdb18e8
Bug 1273220 - Rename RelocatablePtr to HeapPtr; r=sfink

Comment 6

[Pulsebot]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f18e46bce0a4
https://hg.mozilla.org/mozilla-central/rev/a7d6b2c16a3f