1273276 - Rename HeapPtr to GCPtr

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 02_rename_HeapPtr_to_GCManagedPtr-v0.diff

I'm a bit less certain about this renaming. For one it's almost 400 lines: I really did not expect us to have 4x more HeapPtr than RelocatablePtr! It's also a longer and less convenient name. My initial intent was to make the name longer to discourage use since there are few places we use it and accidental usage could be disastrous. While the second of these is still true and (I believe merits the shorter GCPtr name), the first of these is definitely *not* true.

Advantages:
  * Unambiguously different from Heap<T>.
  * More meaningful name.

Disadvantages:
  * Lots of churn for dubious gain.
  * Longer name.

I don't think there's as clear a win here. I'm curious what people think.

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 8753059 [details] [diff] [review]
02_rename_HeapPtr_to_GCManagedPtr-v0.diff

Cancelling feedback as we have a new plan in bug 1273220 comment 1.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 02_rename_HeapPtr_to_GCPtr-v1.diff

As we discussed.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8753995 [details] [diff] [review]
02_rename_HeapPtr_to_GCPtr-v1.diff

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

This looks good.

One thing though, I think we should go with GCPtrValue rather than GCPtr<Value> for consistency with the rest of them.  I don't think this is so bad.

::: js/src/gc/Barrier.h
@@ +127,1 @@
>   * There are also special classes HeapValue and HeapId, which barrier js::Value

HeapValue and HeapId need updating in the line above.

@@ +140,5 @@
>   *  | WriteBarrieredBase     base class which provides common write operations
>   *  |  |  |  |  |
>   *  |  |  |  | PreBarriered  provides pre-barriers only
>   *  |  |  |  |
> + *  |  |  | GCPtr          provides pre- and post-barriers

Comment alignment is off.

@@ +145,4 @@
>   *  |  |  |
>   *  |  | RelocatablePtr      provides pre- and post-barriers and is relocatable
>   *  |  |
> + *  | HeapSlot               similar to GCPtr, but tailored to slots storage

Not sure what HeapSlot is now, GCSlot?

@@ +414,5 @@
>   * used in contexts where it may be implicitly moved or deleted, e.g. most
>   * containers.
>   *
>   * Not to be confused with JS::Heap<T>. This is a different class from the
>   * external interface and implements substantially different semantics.

I guess this comment can go now it's called something different.

::: js/src/jsiter.h
@@ +31,5 @@
>  class PropertyIteratorObject;
>  
>  struct NativeIterator
>  {
> +    GCPtrObject obj;             // Object being iterated.

Comment alignment is off.

::: js/src/jsscript.h
@@ +126,5 @@
>      uint32_t        parent;     // Index of parent block scope in notes, or UINT32_MAX.
>  };
>  
>  struct ConstArray {
> +    js::GCPtr<Value>* vector; // array of indexed constant values

Comment alignment is off.

@@ +132,5 @@
>  };
>  
>  struct ObjectArray {
> +    js::GCPtrObject* vector; // Array of indexed objects.
> +    uint32_t length; // Count of indexed objects.

Ditto above.

::: js/src/vm/Shape.h
@@ -585,5 @@
> -                                   to many-kids data structure */
> -        HeapPtrShape* listp;    /* dictionary list starting at shape_
> -                                   has a double-indirect back pointer,
> -                                   either to the next shape's parent if not
> -                                   last, else to obj->shape_ */

Can you fix comment alignements above, including |parent| which was already off?

Comment 4

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> Comment on attachment 8753995 [details] [diff] [review]
> 02_rename_HeapPtr_to_GCPtr-v1.diff
> 
> Review of attachment 8753995 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good.
> 
> One thing though, I think we should go with GCPtrValue rather than
> GCPtr<Value> for consistency with the rest of them.  I don't think this is
> so bad.

Yeah, it's better to be consistent. Fixed.

> ::: js/src/gc/Barrier.h
> @@ +127,1 @@
> >   * There are also special classes HeapValue and HeapId, which barrier js::Value
> 
> HeapValue and HeapId need updating in the line above.

Actually, that's lines totally wrong now that we're using the same wrapper for pointers and Value/jsid. I've just removed it.

> @@ +140,5 @@
> >   *  | WriteBarrieredBase     base class which provides common write operations
> >   *  |  |  |  |  |
> >   *  |  |  |  | PreBarriered  provides pre-barriers only
> >   *  |  |  |  |
> > + *  |  |  | GCPtr          provides pre- and post-barriers
> 
> Comment alignment is off.

D'oh! Thought I had fixed that one. Stuff kinda got confusing for awhile when I inverted this series.
 
> @@ +145,4 @@
> >   *  |  |  |
> >   *  |  | RelocatablePtr      provides pre- and post-barriers and is relocatable
> >   *  |  |
> > + *  | HeapSlot               similar to GCPtr, but tailored to slots storage
> 
> Not sure what HeapSlot is now, GCSlot?

How about just Slot? It's pretty generic, but I think it's the only place in spidermonkey that uses the term. I'll post a separate patch with that change. 

> @@ +414,5 @@
> >   * used in contexts where it may be implicitly moved or deleted, e.g. most
> >   * containers.
> >   *
> >   * Not to be confused with JS::Heap<T>. This is a different class from the
> >   * external interface and implements substantially different semantics.
> 
> I guess this comment can go now it's called something different.

\o/

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f95d305dc0f804dea500ea8910683668f8729c7e
Bug 1273276 - Rename HeapPtr to GCPtr; r=jonco

Comment 6

[Carsten Book [:Tomcat]]

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