Closed Bug 1287955 Opened 8 years ago Closed 8 years ago

Add mozilla::RefPtrIntUnion to MFBT

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(1 file)

This type acts as a tagged union between a RefPtr and an intptr_t, using the lowest bit as a discriminant to determine which is stored internally.

I wrote this as james wants it for his work on bug 651120.
I'm not sure I like the name, but I couldn't think of a better one before I had finished writing the class. I'm open to bikeshedding (as in me agreeing to all of your requests) on how the API looks.

I didn't decide to have assignment operators or constructors for pointers and integers, rather requiring explicit function calls.
Attachment #8772614 - Flags: review?(nfroyd)
I just want to confirm that this is what you want for your patch James.
Flags: needinfo?(jandreou)
Comment on attachment 8772614 [details] [diff] [review]
Add mozilla::RefPtrIntUnion to MFBT

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

I think the Right Way to do this is some *gnarly* template specializations on mozilla::Variant.  It's possible this is not super-practical.  Are we for sure that simply using Variant<RefPtr<T>, int> results in unacceptable space usage?

The efforts to add mozilla::Result (bug 1283562) are also adding something like this, so maybe it's worth considering the implementation there and possible ways to unify things.

Comments on the interface below.  I didn't really look at the tests or look too closely at the implementation.  I would really like a better way to build this out of things we already have, or making it more general.

::: mfbt/RefPtrIntUnion.h
@@ +101,5 @@
> +  SetAsInt(intptr_t aInt)
> +  // set as an integer from an integer
> +  {
> +    MaybeRelease();
> +    mInt = (aInt << 1) | 1;

Two things:

* left-shifting a signed value is undefined behavior;
* this transformation silently drops information from the argument, which seems unhelpful.

@@ +107,5 @@
> +  }
> +
> +  T*
> +  GetAsPtr() const
> +  // retrieve the value as a pointer, or nullptr is the value is not a pointer

Nit: "...if the value..."

This doesn't seem right...how do I distinguish between the case where I stored nullptr in the union versus the case where the union is actually an integer?

In mozilla::Variant, we MOZ_ASSERT that you have the correct type.

@@ +117,5 @@
> +  }
> +
> +  intptr_t
> +  GetAsInt() const
> +  // retrieve the value as an integer, or 0 is the value is not an integer

Same comments here as above, right down to the spelling nit.

@@ +130,5 @@
> +  already_AddRefed<T>
> +  ForgetPtr()
> +  // if the value is a pointer, return it as |already_AddRefed|, and clear
> +  // the stored pointer value to nullptr. If it is an integer, do nothing
> +  // and return nullptr.

Same comments as above, ambiguity-wise.

@@ +150,5 @@
> +
> +private:
> +  void
> +  MaybeRelease()
> +  // release the stored pointer if it is present and non-null

...and leaving the union in what state afterwards?  That is, we should probably null out the pointer...
Attachment #8772614 - Flags: review?(nfroyd)
(In reply to Michael Layzell [:mystor] from comment #1)
> I'm open to bikeshedding (as in me agreeing
> to all of your requests) on how the API looks.

I approve of this comment, btw.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8772614 [details] [diff] [review]
> Add mozilla::RefPtrIntUnion to MFBT
> 
> Review of attachment 8772614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the Right Way to do this is some *gnarly* template specializations
> on mozilla::Variant.  It's possible this is not super-practical.  Are we for
> sure that simply using Variant<RefPtr<T>, int> results in unacceptable space
> usage?

I'm sure that I could write up a template specialization on mozilla::Variant for Variant<RefPtr<T>, Q> where sizeof(T*) > sizeof(Q), but I feel like it would be unsafe to write one which would accept Variant<RefPtr<T>, uintptr_t>, because we wouldn't be able to fit an entire uintptr_t inside of the second variant option (this class only actually fits 31 bits on on 32 bit systems, and 63 bits on 64 bit systems).

The idea in this particular situation is to use this as a space saving mechanism in nsINode, so I believe that the amount of space which is being taken up is fairly important. This was intended as a tool to make writing that code safer. 

> The efforts to add mozilla::Result (bug 1283562) are also adding something
> like this, so maybe it's worth considering the implementation there and
> possible ways to unify things.

If they are directly copying Rust's Result<Ok, Err> then I assume that the null pointer optimization is being used. I suppose that they may be doing some tricks with pointers and nsresults potentially? Some code like that could be easily added to mozilla::Variant.

> Comments on the interface below.  I didn't really look at the tests or look
> too closely at the implementation.  I would really like a better way to
> build this out of things we already have, or making it more general.

I agree. It would be awesome if we could build it out of existing tools, and/or make it more general. Something like the variant optimization I talked about before could work, if James only requires 16-bits worth of numbers. I suppose we could also add a type like `int31_t` which is a 31 bit integer (implemented of course as a 32 bit integer), which can thus be stored inside of a Variant through even more specialization magic.

> ::: mfbt/RefPtrIntUnion.h
> @@ +101,5 @@
> > +  SetAsInt(intptr_t aInt)
> > +  // set as an integer from an integer
> > +  {
> > +    MaybeRelease();
> > +    mInt = (aInt << 1) | 1;
> 
> Two things:
> 
> * left-shifting a signed value is undefined behavior;

Oops. I'll make it a uintptr_t.

> * this transformation silently drops information from the argument, which
> seems unhelpful.

I can MOZ_ASSERT(aInt < 1<<31) ?

> @@ +107,5 @@
> > +  }
> > +
> > +  T*
> > +  GetAsPtr() const
> > +  // retrieve the value as a pointer, or nullptr is the value is not a pointer
> 
> Nit: "...if the value..."
> 
> This doesn't seem right...how do I distinguish between the case where I
> stored nullptr in the union versus the case where the union is actually an
> integer?
> 
> In mozilla::Variant, we MOZ_ASSERT that you have the correct type.

Sounds good. I'll change this interface to act like that.

> @@ +117,5 @@
> > +  }
> > +
> > +  intptr_t
> > +  GetAsInt() const
> > +  // retrieve the value as an integer, or 0 is the value is not an integer
> 
> Same comments here as above, right down to the spelling nit.
> 
> @@ +130,5 @@
> > +  already_AddRefed<T>
> > +  ForgetPtr()
> > +  // if the value is a pointer, return it as |already_AddRefed|, and clear
> > +  // the stored pointer value to nullptr. If it is an integer, do nothing
> > +  // and return nullptr.
> 
> Same comments as above, ambiguity-wise.
> 
> @@ +150,5 @@
> > +
> > +private:
> > +  void
> > +  MaybeRelease()
> > +  // release the stored pointer if it is present and non-null
> 
> ...and leaving the union in what state afterwards?  That is, we should
> probably null out the pointer...

It's private, so I was intending it to only be used internally. I can make it null it out because any extra pointer writes will almost certainly be optimized out.

Should I try to write that mozilla::Variant specialization, or keep going on this particular implementation?
Flags: needinfo?(nfroyd)
I should also mention that sizeof(mozilla::Variant<RefPtr<T>, int>) > sizeof(struct { int i; RefPtr<T> p; }) right now, due to mozilla::Variant using a size_t type tag, so using mozilla::Variant right now in that situation doesn't make much sense.
Looks good from an implementation point of view. Thanks Micheal!
Flags: needinfo?(jandreou)
(In reply to Michael Layzell [:mystor] from comment #5)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > I think the Right Way to do this is some *gnarly* template specializations
> > on mozilla::Variant.  It's possible this is not super-practical.  Are we for
> > sure that simply using Variant<RefPtr<T>, int> results in unacceptable space
> > usage?
> 
> I'm sure that I could write up a template specialization on mozilla::Variant
> for Variant<RefPtr<T>, Q> where sizeof(T*) > sizeof(Q), but I feel like it
> would be unsafe to write one which would accept Variant<RefPtr<T>,
> uintptr_t>, because we wouldn't be able to fit an entire uintptr_t inside of
> the second variant option (this class only actually fits 31 bits on on 32
> bit systems, and 63 bits on 64 bit systems).

This sounds like you are admitting this class is unsafe. ;)

> The idea in this particular situation is to use this as a space saving
> mechanism in nsINode, so I believe that the amount of space which is being
> taken up is fairly important. This was intended as a tool to make writing
> that code safer. 

Sure.  What I'm wondering is where this idea that we must take the minimal amount of space necessary is coming from.  Is this a relatively new requirement, or is it just cargo-culting old code...and where did the old code get its requirements from, etc.?  It's worth re-examining assumptions every so often.

> > @@ +150,5 @@
> > > +
> > > +private:
> > > +  void
> > > +  MaybeRelease()
> > > +  // release the stored pointer if it is present and non-null
> > 
> > ...and leaving the union in what state afterwards?  That is, we should
> > probably null out the pointer...
> 
> It's private, so I was intending it to only be used internally. I can make
> it null it out because any extra pointer writes will almost certainly be
> optimized out.

I missed the private part, my mistake.  I think it'd be good to make it null things out for defensiveness.

> Should I try to write that mozilla::Variant specialization, or keep going on
> this particular implementation?

I think if uint16_t is enough, you should try going the Variant route.  (Bonus points if you make Variant<RefPtr<T>, uint16_t> and Variant<uint16_t, RefPtr<T>> work!)  If you really need a bigger range, I think you should go the specialized class route, as it might be more obvious that sticking integers in the class has different requirements.

You don't have to put RefPtrIntUnion in MFBT, which might be an easier review hurdle to clear.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #8)
> I think if uint16_t is enough, you should try going the Variant route. 
> (Bonus points if you make Variant<RefPtr<T>, uint16_t> and Variant<uint16_t,
> RefPtr<T>> work!)  If you really need a bigger range, I think you should go
> the specialized class route, as it might be more obvious that sticking
> integers in the class has different requirements.
> 
> You don't have to put RefPtrIntUnion in MFBT, which might be an easier
> review hurdle to clear.

I think this is the approach James will probably take. This class probably won't see much use outside of his particular use case anyways, so it should probably be a utility class for the particular situation.

Closing this bug, as it doesn't make much sense to keep pushing on it. I might make the Variant specialization at some point, but my initial experimentation makes it seem pretty ugly :S. I imagine that the most progress in space saving with Variant could probably be done by being more clever about alignment, and using a smaller tag value when possible.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: