Bug 115033 – Add move semantics to RefPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mikhail Pozdnyakov]

SSIA. That would allow us to obviate unnecessary reffing/ureffing when RefPtr is created or assigned from rvalue references.

[Mikhail Pozdnyakov]

Created attachment 199233 [details]
patch

[WebKit Commit Bot]

Attachment 199233 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:84:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 199233 [details]
patch

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

This is great, but I'd appreciate if you could upload a new patch so we can let the EWS bots chew at it - it's a pretty fundamental change and it could break the build in weird wais.

> Source/WTF/wtf/RefPtr.h:47
> +        ALWAYS_INLINE RefPtr(RefPtr&& o) : m_ptr(o.get()) { o.m_ptr = 0; }

I think this would read better as 

RefPtr(RefPtr&& o) : m_ptr(o.release().leakRef()) { }

> Source/WTF/wtf/RefPtr.h:48
> +        template<typename U> RefPtr(RefPtr<U>&& o) : m_ptr(o.get()) { o.m_ptr = 0; }

Same thing here.

> Source/WTF/wtf/RefPtr.h:163
> +        if (m_ptr != o.get()) {
> +            derefIfNotNull(m_ptr);
> +            m_ptr = o.get();
> +            o.m_ptr = 0;
> +        }
> +        return *this;

I think this could just be m_ptr = o.release();

> Source/WTF/wtf/RefPtr.h:172
> +        if (m_ptr != o.get()) {
> +            derefIfNotNull(m_ptr);
> +            m_ptr = o.get();
> +            o.m_ptr = 0;
> +        }

Ditto.

[Mikhail Pozdnyakov]

Created attachment 199236 [details]
patch v2

Updated due to comments from Anders. Thanks Anders!

[Anders Carlsson]

Comment on attachment 199236 [details]
patch v2

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

> Source/WTF/wtf/RefPtr.h:85
> +        RefPtr& operator=(RefPtr&& o) { m_ptr = o.release(); }
> +        template<typename U> RefPtr& operator=(RefPtr<U>&& o) { m_ptr = o.release(); }

These will cause a memory leak now, and return *this is gone. Let's change them back into what you had before; something like:

if (m_ptr != o.m_ptr)) {
    derefIfNotNull(m_ptr);
    m_ptr = o.release().leakRef();
}
return *this;

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (From update of attachment 199236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199236&action=review
> 
> > Source/WTF/wtf/RefPtr.h:85
> > +        RefPtr& operator=(RefPtr&& o) { m_ptr = o.release(); }
> > +        template<typename U> RefPtr& operator=(RefPtr<U>&& o) { m_ptr = o.release(); }
> 
> These will cause a memory leak now, and return *this is gone.
omg! that's my absent-mindedness :(

[Mikhail Pozdnyakov]

Created attachment 199244 [details]
patch v3

fixed assignment operators :/

[WebKit Commit Bot]

Attachment 199244 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:86:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

> That would allow us to obviate unnecessary reffing/ureffing when RefPtr is created or assigned from rvalue references.

Do we have examples of that in WebKit now? We are supposed to use PassRefPtr for poor man's move semantics.

To be clear, I certainly don't object to this change.

[Benjamin Poulain]

(In reply to comment #9)
> > That would allow us to obviate unnecessary reffing/ureffing when RefPtr is created or assigned from rvalue references.
> 
> Do we have examples of that in WebKit now? We are supposed to use PassRefPtr for poor man's move semantics.

There is no explicit use of rvalue reference, but clang is smart enough to use it in some cases.

[Benjamin Poulain]

Comment on attachment 199244 [details]
patch v3

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

> Source/WTF/wtf/RefPtr.h:160
> +        if (m_ptr != o.get()) {
> +            derefIfNotNull(m_ptr);
> +            m_ptr = o.release().leakRef();
> +        }
> +        return *this;

I don't understand why you do this.

If you have
    RefPtr a = [foobar];
    RefPtr b = a;
    a = std::move(b);

"b" is unchanged, and the refcount is +2. This is counter-intuitive for move semantic.

> Source/WTF/wtf/RefPtr.h:169
> +        if (m_ptr != o.get()) {
> +            derefIfNotNull(m_ptr);
> +            m_ptr = o.release().leakRef();
> +        }
> +        return *this;

ditto

[Mikhail Pozdnyakov]

Created attachment 199413 [details]
patch v4

Addresses Benjamin's concern.

[WebKit Commit Bot]

Attachment 199413 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:86:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 199413 [details]
patch v4

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

> Source/WTF/wtf/RefPtr.h:159
> +        T* ptr = m_ptr;
> +        m_ptr = o.release().leakRef();
> +        derefIfNotNull(ptr);
> +        return *this;

With this, what if I do this?

RefPtr a = [...];
a = std::move(a);

[Mikhail Pozdnyakov]

(In reply to comment #14)
> (From update of attachment 199413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199413&action=review
> 
> > Source/WTF/wtf/RefPtr.h:159
> > +        T* ptr = m_ptr;
> > +        m_ptr = o.release().leakRef();
> > +        derefIfNotNull(ptr);
> > +        return *this;
> 
> With this, what if I do this?
> 
> RefPtr a = [...];
> a = std::move(a);

mm, wouldn't it mean that we do not care about 'a' anymore? so that looks like caller's fault.
btw, in RetainPtr we have exactly same implementation:
    template<typename T> inline RetainPtr<T>& RetainPtr<T>::operator=(RetainPtr<T>&& o)
    {
        adoptCF(o.leakRef());
        return *this;
    }

and

    template<typename T> inline void RetainPtr<T>::adoptCF(PtrType optr)
    {
        PtrType ptr = m_ptr;
        m_ptr = optr;
        if (ptr)
            CFRelease(ptr);
    }

[Anders Carlsson]

(In reply to comment #11)
> (From update of attachment 199244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199244&action=review
> 
> > Source/WTF/wtf/RefPtr.h:160
> > +        if (m_ptr != o.get()) {
> > +            derefIfNotNull(m_ptr);
> > +            m_ptr = o.release().leakRef();
> > +        }
> > +        return *this;
> 
> I don't understand why you do this.

My mistake, I think this check should just be:

if (this != &o) {
...
}

[Mikhail Pozdnyakov]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 199413 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=199413&action=review
> > 
> > > Source/WTF/wtf/RefPtr.h:159
> > > +        T* ptr = m_ptr;
> > > +        m_ptr = o.release().leakRef();
> > > +        derefIfNotNull(ptr);
> > > +        return *this;
> > 
> > With this, what if I do this?
> > 
> > RefPtr a = [...];
> > a = std::move(a);
> 
> mm, wouldn't it mean that we do not care about 'a' anymore? so that looks like caller's fault.
> btw, in RetainPtr we have exactly same implementation:
>     template<typename T> inline RetainPtr<T>& RetainPtr<T>::operator=(RetainPtr<T>&& o)
>     {
>         adoptCF(o.leakRef());
>         return *this;
>     }
> 
> and
> 
>     template<typename T> inline void RetainPtr<T>::adoptCF(PtrType optr)
>     {
>         PtrType ptr = m_ptr;
>         m_ptr = optr;
>         if (ptr)
>             CFRelease(ptr);
>     }
yeah, with RetainPtr it's different as it\s releasing m_ptr which is 0 after leakPtr() call.

[Mikhail Pozdnyakov]

Created attachment 199733 [details]
patch v5

Fixed '=' operator.

[WebKit Commit Bot]

Attachment 199733 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:86:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 199733 [details]
patch v5

Attachment 199733 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/144200

[Build Bot]

Comment on attachment 199733 [details]
patch v5

Attachment 199733 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/175534

[Darin Adler]

Comment on attachment 199733 [details]
patch v5

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

> Source/WTF/wtf/RefPtr.h:168
> +        if (this != &o) {
> +            derefIfNotNull(m_ptr);
> +            m_ptr = o.release().leakRef();
> +        }

It’s difficult to get this right; often this and &o are not objects of the same type, so we can’t compare them.

I suggest we instead consider the canonical operator= implementation:

    RefPtr<T> ptr = o;
    swap(ptr);
    return *this;

We could use this for both operator= overloads. As long as it compiles to efficient-enough code.

[Build Bot]

Comment on attachment 199733 [details]
patch v5

Attachment 199733 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/19238

[Anders Carlsson]

(In reply to comment #22)
.
> 
> I suggest we instead consider the canonical operator= implementation:
> 
>     RefPtr<T> ptr = o;
>     swap(ptr);
>     return *this;

I like that. The first row should be

RefPtr<T> ptr = std::move(o);

[Mikhail Pozdnyakov]

(In reply to comment #24)
> (In reply to comment #22)
> .
> > 
> > I suggest we instead consider the canonical operator= implementation:
> > 
> >     RefPtr<T> ptr = o;
> >     swap(ptr);
> >     return *this;
> 
> I like that. The first row should be
> 
> RefPtr<T> ptr = std::move(o);

That's great! but how about just
        
        RefPtr<T>(std::move(o)).swap(*this);
        return *this;

[Mikhail Pozdnyakov]

Created attachment 199804 [details]
patch v6

Canonical '=' operator implementation.

[WebKit Commit Bot]

Attachment 199804 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:87:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 199804 [details]
patch v6

(In reply to comment #25)
> That's great! but how about just
> 
>         RefPtr<T>(std::move(o)).swap(*this);
>         return *this;

I think it's easier to read with an explicit temporary. Please make that change, r=me.

[Mikhail Pozdnyakov]

Committed r149184: <http://trac.webkit.org/changeset/149184>