1128110 - Common up the redundant bits of our multiple pointer-like GC classes

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

In order to make a pointer-like class in C++, you need a bunch of overloads. Thus, our pointer-like classes have a long list of similar, but not always identical methods. It would be nice to know which of these divergences are actually necessary for correct operation and, better, to have them de-redundantified.

Comment 1

[Terrence Cole [:terrence]]

Attachment: 1_equality-v0.diff

Comment 2

[Terrence Cole [:terrence]]

Attachment: 2_const_ref-v0.diff

Surprisingly, the pointer-like reference operations can all be const.

Comment 3

[Terrence Cole [:terrence]]

Attachment: 3_assign-v0.diff

And we can get away with returning wrapped from all assignment.

Comment 4

[Terrence Cole [:terrence]]

Attachment: 4_nonpointer-v0.diff

The reason is that we use nonpointer methods for mutable access, except for Handle, where such are not used.

Comment 5

[Terrence Cole [:terrence]]

Attachment: 5_nonassignable-v0.diff

And the operators we use to make (Fake)(Mutable)Handle unassignable.

With this you can pretty much annotate the top of a pointer class with the operations it is allowed to perform and get the expected behavior. Or at least our current behavior.

Comment 6

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c501c4019726

Comment 7

[Jon Coppeard (:jonco)]

This is a really nice tidyup!

Comment 8

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

Comment on attachment 8559460 [details] [diff] [review]
1_equality-v0.diff

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

::: js/public/RootingAPI.h
@@ +151,5 @@
> +        return static_cast<const Base<T> *>(self);
> +    }
> +  public:
> +    bool operator==(const T &other) const { return extractEquality(this)->get() == other; }
> +    bool operator!=(const T &other) const { return extractEquality(this)->get() != other; }

The name "extractEquality" seems a little odd. extractForEquality? castForEquality? How about "asBase"? (Though this is the first patch in the stack I've looked at; maybe it'll need to be asConstBase. Or can that be done with const/non-const overrides?)

@@ +403,5 @@
>   * specialization, define a HandleBase<T> specialization containing them.
>   */
>  template <typename T>
> +class MOZ_NONHEAP_CLASS Handle : public js::HandleBase<T>,
> +                                 public js::PointerEqualityOpsMixin<Handle, T>

I herd you like templates...

Comment 9

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

Comment on attachment 8559461 [details] [diff] [review]
2_const_ref-v0.diff

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

::: js/public/RootingAPI.h
@@ +167,5 @@
> +    operator const T &() const { return extractConstRef(this)->get(); }
> +    const T &operator->() const { return extractConstRef(this)->get(); }
> +};
> +
> +#undef EXTRACT_FACTORY

I still like "as" better than "extract". But do you need it at all? Is the macro and helper struct really better than something like

struct PointerConstRefOpsMixin {
    typedef const Base<T> *BasePtr;
    operator const T &() const { return static_cast<BasePtr>(this)->get(); }
    const T &operator->() const { return static_cast<BasePtr>(this)->get(); }
};

I'm going to continue making inane comments that I will possibly later retract, and marking things as r+ anyway, as I move through this stack. :-)

Comment 10

[Terrence Cole [:terrence]]

Ohh, looks like the try run indicates that multiple-inheritance requires more space in the instances in visual studio?!? Maybe we should just go all the way to macros for all of these?

Comment 11

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

Comment on attachment 8559463 [details] [diff] [review]
3_assign-v0.diff

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

::: js/public/RootingAPI.h
@@ +183,5 @@
> +        set(other.get());                                                                         \
> +        return *this;                                                                             \
> +    }                                                                                             \
> +  private:
> +

This is indeed unfortunate.

I'm not sure about the visibility stuff, though. Why not leave them out of the macro, and put the macro instantiation into the public: section of the actual class? I think you can even put a semicolon after the instantiation that way, too.

Also, _MIXIN seems a little weird for this one, given that you're not inheriting. How about DECLARE_POINTER_ASSIGN_OPS instead?

I understand that you're kind of thinking of this as a magical mixin thing, but personally I think I prefer to have it do less and be more understandable, even if it means

  public:
    DECLARE_POINTER_ASSIGN_OPS(X, T);
  private:

or not having the macro at the very beginning.

@@ +306,5 @@
> +        } else {
> +            ptr = newPtr;
> +        }
> +    }
> +

Just a random thought -- how often do we get self-assignments? Are there ever any correctness issues with them (probably not), or any perf gains from ignoring them (more likely, but still probably not)?

@@ +806,5 @@
>      const T &get() const { return ptr; }
>  
> +    /*
> +     * This method is public for Rooted so that Codegen.py can use a Rooted
> +     * interchangably with a MutableHandleValue.

*interchangeably

Comment 12

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

Comment on attachment 8559464 [details] [diff] [review]
4_nonpointer-v0.diff

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

::: js/public/RootingAPI.h
@@ +188,5 @@
> +#define NONPOINTER_CONST_ACCESSOR_METHODS(ptr)                                                    \
> +  public:                                                                                         \
> +    const T *address() const { return &(ptr); }                                                   \
> +    const T &get() const { return (ptr); }                                                        \
> +  private:

Same comment about public/private.

@@ +191,5 @@
> +    const T &get() const { return (ptr); }                                                        \
> +  private:
> +
> +#define NONPOINTER_MUTABLE_ACCESSOR_METHODS(ptr)                                                  \
> +    NONPOINTER_CONST_ACCESSOR_METHODS(ptr)                                                        \

Hmm... so _MUTABLE_ defines both? You don't want to just call it NONPOINTER_ACCESSOR_METHODS? (Or DECLARE_NONPOINTER_ACCESSOR_METHODS)

@@ +260,5 @@
>  class Heap : public js::HeapBase<T>,
>               public js::PointerConstRefOpsMixin<Heap, T>
>  {
>      POINTER_ASSIGN_OPS_MIXIN(Heap, T)
> +    NONPOINTER_CONST_ACCESSOR_METHODS(ptr);

So you *can* use semicolons without it complaining?

Comment 13

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

Comment on attachment 8559466 [details] [diff] [review]
5_nonassignable-v0.diff

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

::: js/public/RootingAPI.h
@@ +187,5 @@
>  
> +#define POINTER_NONASSIGNABLE_OPS_MIXIN(Wrapper, T)                                               \
> +  private:                                                                                        \
> +    template <typename S> Wrapper<T> &operator=(S) = delete;                                      \
> +    Wrapper<T> &operator=(const Wrapper<T> &) = delete;

Lose the private: and call it DELETE_ASSIGNMENT_OPS?

Comment 14

[Terrence Cole [:terrence]]

Attachment: 6_apply_review_comments-v0.diff

As discussed on IRC: msvc is only able to apply the empty base class optimization to a single base class, so this violates sizeof(T) == sizeof(Heap<T>) on windows. Thus, we, with great sadness, need to go with an all-MACROs solution. Oh, well. It actually reads a bit cleaner and should have less impact on compile times.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb99c505c6f

Comment 15

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

Comment on attachment 8560071 [details] [diff] [review]
6_apply_review_comments-v0.diff

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

::: js/public/RootingAPI.h
@@ +152,5 @@
>  // Important: Return a reference so passing a Rooted<T>, etc. to
>  // something that takes a |const T&| is not a GC hazard.
> +#define DECLARE_POINTER_CONSTREF_OPS(Wrapper, T)                                                  \
> +    operator const T &() const { return get(); }                                                  \
> +    const T &operator->() const { return get(); }

Neither DECLARE_POINTER_COMPARISON_OPS nor DECLARE_POINTER_CONSTREF_OPS use the Wrapper argument. I thought maybe you were passing it into everything for consistency, but DECLARE_NON_POINTER_ACCESSOR_METHODS only takes 1 arg. Kill it?

@@ +157,5 @@
>  
>  // Assignment operators on a base class are hidden by the implicitly defined
>  // operator= on the derived class. Thus, define the operator= directly on the
>  // class as we would need to manually pass it through anyway.
> +#define DECLARE_POINTER_ASSIGN_OPS(Wrapper, T)                                                    \

It would be cool if you could eliminate Wrapper here and use decltype(*this) or something, but it really doesn't matter.

@@ +170,2 @@
>  
> +#define DECLARE_DELETE_ASSIGNMENT_OPS(Wrapper, T)                                                 \

DECLARE_DELETE_*? Isn't DELETE_* sufficient?

Comment 16

[Terrence Cole [:terrence]]

The try run is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb99c505c6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f4d89fe8a2

Comment 17

[Terrence Cole [:terrence]]

Note, the final folded count is 79 lines saved.

Comment 18

[Phil Ringnalda (:philor)]

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