Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #8753059 - Flags: feedback?(sphink)
Attachment #8753059 - Flags: feedback?(jcoppeard)
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.
Attachment #8753059 - Flags: feedback?(sphink)
Attachment #8753059 - Flags: feedback?(jcoppeard)
As we discussed.
Attachment #8753059 - Attachment is obsolete: true
Attachment #8753995 - Flags: review?(jcoppeard)
Summary: Rename HeapPtr to GCManagedPtr → Rename HeapPtr to GCPtr
Blocks: 1273220
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?
Attachment #8753995 - Flags: review?(jcoppeard) → review+
(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/
https://hg.mozilla.org/mozilla-central/rev/f95d305dc0f8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.