Closed Bug 1273220 Opened 4 years ago Closed 4 years ago

Rename RelocatablePtr to HeapPtr

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8752970 - Flags: review?(sphink)
Attachment #8752970 - Flags: review?(jcoppeard)
Okay new plan!

HeapPtr -> GCPtr (use templates for Value/Id and typedefs elsewhere)
RelocatablePtr -> HeapPtr (similar semantics to Heap<T>)
Comment on attachment 8752970 [details] [diff] [review]
00_rename_RelocatablePtr_to_GCPtr-v0.diff

Cancelling review as we have a new plan now.
Attachment #8752970 - Flags: review?(sphink)
Attachment #8752970 - Flags: review?(jcoppeard)
Depends on: 1273276
Blocks: 1225577
Summary: Rename RelocatablePtr to GCPtr → Rename RelocatablePtr to HeapPtr
As we discussed. This now goes after the HeapPtr->GCPtr change, obviously.
Attachment #8752970 - Attachment is obsolete: true
Attachment #8754020 - Flags: review?(sphink)
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".
Attachment #8754020 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/f18e46bce0a4
https://hg.mozilla.org/mozilla-central/rev/a7d6b2c16a3f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.