Bug 115033 - Add move semantics to RefPtr
Summary: Add move semantics to RefPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-23 06:32 PDT by Mikhail Pozdnyakov
Modified: 2013-04-26 07:55 PDT (History)
8 users (show)

See Also:


Attachments
patch (2.92 KB, patch)
2013-04-23 07:33 PDT, Mikhail Pozdnyakov
andersca: review-
Details | Formatted Diff | Diff
patch v2 (2.20 KB, patch)
2013-04-23 07:52 PDT, Mikhail Pozdnyakov
andersca: review-
Details | Formatted Diff | Diff
patch v3 (2.90 KB, patch)
2013-04-23 08:28 PDT, Mikhail Pozdnyakov
benjamin: review-
Details | Formatted Diff | Diff
patch v4 (2.84 KB, patch)
2013-04-24 04:38 PDT, Mikhail Pozdnyakov
benjamin: review-
Details | Formatted Diff | Diff
patch v5 (2.93 KB, patch)
2013-04-25 12:11 PDT, Mikhail Pozdnyakov
benjamin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v6 (2.93 KB, patch)
2013-04-26 02:09 PDT, Mikhail Pozdnyakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2013-04-23 06:32:20 PDT
SSIA. That would allow us to obviate unnecessary reffing/ureffing when RefPtr is created or assigned from rvalue references.
Comment 1 Mikhail Pozdnyakov 2013-04-23 07:33:46 PDT
Created attachment 199233 [details]
patch
Comment 2 WebKit Commit Bot 2013-04-23 07:35:49 PDT
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.
Comment 3 Anders Carlsson 2013-04-23 07:44:02 PDT
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.
Comment 4 Mikhail Pozdnyakov 2013-04-23 07:52:44 PDT
Created attachment 199236 [details]
patch v2

Updated due to comments from Anders. Thanks Anders!
Comment 5 Anders Carlsson 2013-04-23 08:02:31 PDT
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;
Comment 6 Mikhail Pozdnyakov 2013-04-23 08:19:04 PDT
(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 :(
Comment 7 Mikhail Pozdnyakov 2013-04-23 08:28:40 PDT
Created attachment 199244 [details]
patch v3

fixed assignment operators :/
Comment 8 WebKit Commit Bot 2013-04-23 08:29:38 PDT
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.
Comment 9 Alexey Proskuryakov 2013-04-23 10:30:56 PDT
> 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.
Comment 10 Benjamin Poulain 2013-04-23 11:10:22 PDT
(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.
Comment 11 Benjamin Poulain 2013-04-23 11:19:09 PDT
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
Comment 12 Mikhail Pozdnyakov 2013-04-24 04:38:19 PDT
Created attachment 199413 [details]
patch v4

Addresses Benjamin's concern.
Comment 13 WebKit Commit Bot 2013-04-24 04:40:28 PDT
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.
Comment 14 Benjamin Poulain 2013-04-24 14:42:05 PDT
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);
Comment 15 Mikhail Pozdnyakov 2013-04-25 01:26:23 PDT
(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);
    }
Comment 16 Anders Carlsson 2013-04-25 08:18:08 PDT
(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) {
...
}
Comment 17 Mikhail Pozdnyakov 2013-04-25 08:42:15 PDT
(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.
Comment 18 Mikhail Pozdnyakov 2013-04-25 12:11:26 PDT
Created attachment 199733 [details]
patch v5

Fixed '=' operator.
Comment 19 WebKit Commit Bot 2013-04-25 12:12:26 PDT
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.
Comment 20 Build Bot 2013-04-25 13:16:04 PDT
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
Comment 21 Build Bot 2013-04-25 13:24:26 PDT
Comment on attachment 199733 [details]
patch v5

Attachment 199733 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/175534
Comment 22 Darin Adler 2013-04-25 14:22:03 PDT
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.
Comment 23 Build Bot 2013-04-25 14:23:56 PDT
Comment on attachment 199733 [details]
patch v5

Attachment 199733 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/19238
Comment 24 Anders Carlsson 2013-04-25 14:27:34 PDT
(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);
Comment 25 Mikhail Pozdnyakov 2013-04-26 01:54:10 PDT
(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;
Comment 26 Mikhail Pozdnyakov 2013-04-26 02:09:44 PDT
Created attachment 199804 [details]
patch v6

Canonical '=' operator implementation.
Comment 27 WebKit Commit Bot 2013-04-26 02:12:14 PDT
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.
Comment 28 Anders Carlsson 2013-04-26 07:36:27 PDT
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.
Comment 29 Mikhail Pozdnyakov 2013-04-26 07:55:37 PDT
Committed r149184: <http://trac.webkit.org/changeset/149184>