Bug 215936 – Add CompactUniquePtrTuple

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ryosuke Niwa]

I want to have the ability to use free 8-bits in pointer for std::unique_ptr.

[Ryosuke Niwa]

Should I wrap unique_ptr around or just write a new template class that behaves like std::unique_ptr?

[Darin Adler]

Not sure how to make something a tuple if you can’t get a reference to one of the things inside.

[Darin Adler]

If it was a moved-out copy of the pointer then clearly unique_ptr is what we’d want. But for a reference to the thing inside the tuple, I don’t see how it can be unique_ptr.

[Ryosuke Niwa]

Created attachment 407752 [details]
Patch

[Saam Barati]

Comment on attachment 407752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407752&action=review

> Source/WTF/ChangeLog:8
> +        Added a new template class, CompactUniquePtrTuple, which stores a pointer and up to 16-bits of

why just 16? On iOS, we have more free space than that.

> Source/WTF/wtf/CompactUniquePtrTuple.h:75
> +    template <typename U>
> +    CompactUniquePtrTuple<T, Type>& operator=(CompactUniquePtrTuple<U, Type>&& other)
> +    {
> +        deletePointerIfNotNull();
> +        m_data = other.m_data;
> +        other.m_data = CompactPointerTuple<T*, Type>();
> +        return *this;
> +    }

Never sure how to proceed with this, but this doesn't seem right if we end up moving into ourselves.

> Source/WTF/wtf/CompactUniquePtrTuple.h:77
> +    T* pointer() const { return m_data.pointer(); }

call this get() to match unique_ptr API?

> Source/WTF/wtf/CompactUniquePtrTuple.h:84
> +    std::unique_ptr<T> moveToUniquePtr()
> +    {
> +        T* pointer = m_data.pointer();
> +        m_data.setPointer(nullptr);
> +        return std::unique_ptr<T, Deleter>(pointer);
> +    }

maybe call this release?

> Source/WTF/wtf/CompactUniquePtrTuple.h:93
> +    void setPointer(std::unique_ptr<U>&& pointer)

not sure you need && here since this function is taking ownership of this unique_ptr. If you just have it as "std::unique_ptr<U> pointer", the caller will be forced to relinquish ownership

[Saam Barati]

Comment on attachment 407752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407752&action=review

> Source/WTF/wtf/CompactUniquePtrTuple.h:37
> +ALWAYS_INLINE CompactUniquePtrTuple<T, Type> makeCompactUniquePtr(Args&&... args)

do we want versions of these functions that allow us to pass in Type to the "make" functions?

[Saam Barati]

Comment on attachment 407752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407752&action=review

>> Source/WTF/wtf/CompactUniquePtrTuple.h:77
>> +    T* pointer() const { return m_data.pointer(); }
> 
> call this get() to match unique_ptr API?

maybe this is confusing given it's a tuple. I think pointer() is better

[Ryosuke Niwa]

(In reply to Saam Barati from comment #5)
> Comment on attachment 407752 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407752&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        Added a new template class, CompactUniquePtrTuple, which stores a pointer and up to 16-bits of
> 
> why just 16? On iOS, we have more free space than that.

That's the restriction CompactPointerTuple puts. In general, I don't think it's a good idea to make these classes accept different number of bits in different platforms as we want to share the same code across platforms as much as possible.

> > Source/WTF/wtf/CompactUniquePtrTuple.h:75
> > +    template <typename U>
> > +    CompactUniquePtrTuple<T, Type>& operator=(CompactUniquePtrTuple<U, Type>&& other)
> > +    {
> > +        deletePointerIfNotNull();
> > +        m_data = other.m_data;
> > +        other.m_data = CompactPointerTuple<T*, Type>();
> > +        return *this;
> > +    }
> 
> Never sure how to proceed with this, but this doesn't seem right if we end
> up moving into ourselves.

Oh right, I should have moved to a temporary variable first. Will fix.

> > Source/WTF/wtf/CompactUniquePtrTuple.h:77
> > +    T* pointer() const { return m_data.pointer(); }
> 
> call this get() to match unique_ptr API?

What you said. Other Compact*Tuple class use pointer().

> > Source/WTF/wtf/CompactUniquePtrTuple.h:84
> > +    std::unique_ptr<T> moveToUniquePtr()
> > +    {
> > +        T* pointer = m_data.pointer();
> > +        m_data.setPointer(nullptr);
> > +        return std::unique_ptr<T, Deleter>(pointer);
> > +    }
> 
> maybe call this release?

I think it's in line with pointer/type naming to name this differently from release.
I initially called this release as well but saw UniqueRef use moveToUniquePtr.

> > Source/WTF/wtf/CompactUniquePtrTuple.h:93
> > +    void setPointer(std::unique_ptr<U>&& pointer)
> 
> not sure you need && here since this function is taking ownership of this
> unique_ptr. If you just have it as "std::unique_ptr<U> pointer", the caller
> will be forced to relinquish ownership

That's true but I think it's communicative / self explanatory to use rvalue reference here
than to force the creation of a phantom temporary and force the caller to use WTFMove anyway.

[Ryosuke Niwa]

Created attachment 407847 [details]
Fixed operator=

[Darin Adler]

Comment on attachment 407847 [details]
Fixed operator=

View in context: https://bugs.webkit.org/attachment.cgi?id=407847&action=review

> Source/WTF/wtf/CompactUniquePtrTuple.h:40
> +    static_assert(std::is_same<typename T::webkitFastMalloced, int>::value, "T is FastMalloced");
> +    return CompactUniquePtrTuple<T, Type>(std::make_unique<T>(*new T(std::forward<Args>(args)...)));

Can’t this use WTF::makeUnique instead of repeating its assertion?

> Source/WTF/wtf/CompactUniquePtrTuple.h:47
> +    static_assert(std::is_same<typename T::webkitFastMalloced, int>::value, "T is FastMalloced");
> +    return CompactUniquePtrTuple<T, Type, Deleter>(std::make_unique<T>(*new T(std::forward<Args>(args)...)));

Ditto.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #10)
> Comment on attachment 407847 [details]
> Fixed operator=
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407847&action=review
> 
> > Source/WTF/wtf/CompactUniquePtrTuple.h:40
> > +    static_assert(std::is_same<typename T::webkitFastMalloced, int>::value, "T is FastMalloced");
> > +    return CompactUniquePtrTuple<T, Type>(std::make_unique<T>(*new T(std::forward<Args>(args)...)));
> 
> Can’t this use WTF::makeUnique instead of repeating its assertion?

Oh oops, I forgot fo fix that. Doing that now.

[Ryosuke Niwa]

Created attachment 407870 [details]
Addresed Darin's review comment

[Darin Adler]

Comment on attachment 407870 [details]
Addresed Darin's review comment

View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review

> Source/WTF/wtf/CompactUniquePtrTuple.h:59
> +        other.m_data = CompactPointerTuple<T*, Type>();

Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax.

> Source/WTF/wtf/CompactUniquePtrTuple.h:74
> +        return *this;

Seems like this should be written more simply:

    auto moved = WTFMove(other);
    std:swap(m_data, moved.m_data);
    return *this;

Or this more efficient version that does not null out the other but probably you like nulling out the other:

    std::swap(*this.m_data, other.m_data);
    return *this;

> Source/WTF/wtf/CompactUniquePtrTuple.h:79
> +    std::unique_ptr<T> moveToUniquePtr()

Seems wrong that this does not use the proper Deleter.

> Source/WTF/wtf/CompactUniquePtrTuple.h:86
> +    void setPointer(nullptr_t)

Is this really needed? Won’t the other setPointer do this just as efficiently?

> Source/WTF/wtf/CompactUniquePtrTuple.h:112
> +    void deletePointerIfNotNull()

Since the built in delete operator does nothing when passed null it seems this could just be named deletePointer

> Source/WTF/wtf/CompactUniquePtrTuple.h:119
> +    template<typename U, typename E, typename D, class... Args> friend CompactUniquePtrTuple<U, E, D> makeCompactUniquePtr(Args&&... args);

Not sure why these use class... rather than typename...

[Ryosuke Niwa]

Comment on attachment 407870 [details]
Addresed Darin's review comment

View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review

>> Source/WTF/wtf/CompactUniquePtrTuple.h:59
>> +        other.m_data = CompactPointerTuple<T*, Type>();
> 
> Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax.

I'm not sure what you mean. If you mean that:
m_data = WTFMove(other.m_data)
then it won't work because WTFMove(other.m_data) is basically a no-op.

>> Source/WTF/wtf/CompactUniquePtrTuple.h:74
>> +        return *this;
> 
> Seems like this should be written more simply:
> 
>     auto moved = WTFMove(other);
>     std:swap(m_data, moved.m_data);
>     return *this;
> 
> Or this more efficient version that does not null out the other but probably you like nulling out the other:
> 
>     std::swap(*this.m_data, other.m_data);
>     return *this;

That's a good point. Done the variant which clears out "other".

>> Source/WTF/wtf/CompactUniquePtrTuple.h:79
>> +    std::unique_ptr<T> moveToUniquePtr()
> 
> Seems wrong that this does not use the proper Deleter.

Oops, fixed. I had specified this in the return statement but forgot it here.

>> Source/WTF/wtf/CompactUniquePtrTuple.h:86
>> +    void setPointer(nullptr_t)
> 
> Is this really needed? Won’t the other setPointer do this just as efficiently?

Yes, otherwise, we'd get a build error like:

CompactUniquePtrTuple.cpp:67:7: error: no matching member function for call to 'setPointer'
    a.setPointer(nullptr);
    ~~^~~~~~~~~~
In file included from Tools/TestWebKitAPI/Tests/WTF/CompactUniquePtrTuple.cpp:30:
CompactUniquePtrTuple.h:91:10: note: candidate template ignored: could not match 'unique_ptr<type-parameter-0-0, default_delete<type-parameter-0-0> >' against 'nullptr_t'
    void setPointer(std::unique_ptr<U>&& pointer)
         ^
The issue is that the compiler can't convert nullptr_t to std::unique_ptr<U>.

>> Source/WTF/wtf/CompactUniquePtrTuple.h:112
>> +    void deletePointerIfNotNull()
> 
> Since the built in delete operator does nothing when passed null it seems this could just be named deletePointer

Sure, done.

>> Source/WTF/wtf/CompactUniquePtrTuple.h:119
>> +    template<typename U, typename E, typename D, class... Args> friend CompactUniquePtrTuple<U, E, D> makeCompactUniquePtr(Args&&... args);
> 
> Not sure why these use class... rather than typename...

Fixed. Not sure what happened there.

[Darin Adler]

Comment on attachment 407870 [details]
Addresed Darin's review comment

View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review

>>> Source/WTF/wtf/CompactUniquePtrTuple.h:59
>>> +        other.m_data = CompactPointerTuple<T*, Type>();
>> 
>> Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax.
> 
> I'm not sure what you mean. If you mean that:
> m_data = WTFMove(other.m_data)
> then it won't work because WTFMove(other.m_data) is basically a no-op.

I mean:

    m_data = std::exchange(other.m_data, { });

[Darin Adler]

Comment on attachment 407870 [details]
Addresed Darin's review comment

View in context: https://bugs.webkit.org/attachment.cgi?id=407870&action=review

>>>> Source/WTF/wtf/CompactUniquePtrTuple.h:59
>>>> +        other.m_data = CompactPointerTuple<T*, Type>();
>>> 
>>> Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax.
>> 
>> I'm not sure what you mean. If you mean that:
>> m_data = WTFMove(other.m_data)
>> then it won't work because WTFMove(other.m_data) is basically a no-op.
> 
> I mean:
> 
>     m_data = std::exchange(other.m_data, { });

Or actually, more like this:

    : m_data { std::exchange(other.m_data, { }) }

[Ryosuke Niwa]

(In reply to Darin Adler from comment #16)
> Comment on attachment 407870 [details]
> Addresed Darin's review comment
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407870&action=review
> 
> >>>> Source/WTF/wtf/CompactUniquePtrTuple.h:59
> >>>> +        other.m_data = CompactPointerTuple<T*, Type>();
> >>> 
> >>> Could we use std::exchange here? It would make this a one-liner and we could use construction syntax rather than assignment syntax.
> >> 
> >> I'm not sure what you mean. If you mean that:
> >> m_data = WTFMove(other.m_data)
> >> then it won't work because WTFMove(other.m_data) is basically a no-op.
> > 
> > I mean:
> > 
> >     m_data = std::exchange(other.m_data, { });
> 
> Or actually, more like this:
> 
>     : m_data { std::exchange(other.m_data, { }) }

Oh I see what you're saying. Fixed.

[Ryosuke Niwa]

Committed r266573: <https://trac.webkit.org/changeset/266573>

[Radar WebKit Bug Importer]

<rdar://problem/68318595>