Bug 154273 – Reduce uses of PassRefPtr in indexeddb

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 271403 [details]
Patch

[Gyuyoung Kim]

Created attachment 271426 [details]
Patch

[Brady Eidson]

Comment on attachment 271426 [details]
Patch

Please hold off on this.

This will interfere with at least one feature patch I have in progress, and also touches "Dead-code-walking" (https://bugs.webkit.org/show_bug.cgi?id=150854)

After 150584 is resolved (Soon!), this would be a great task to revisit.

[Gyuyoung Kim]

(In reply to comment #3)
> Comment on attachment 271426 [details]
> Patch
> 
> Please hold off on this.
> 
> This will interfere with at least one feature patch I have in progress, and
> also touches "Dead-code-walking"
> (https://bugs.webkit.org/show_bug.cgi?id=150854)
> 
> After 150584 is resolved (Soon!), this would be a great task to revisit.

I see. Let me update this clean up after landing it.

[Gyuyoung Kim]

Created attachment 272324 [details]
Patch

[Darin Adler]

Comment on attachment 272324 [details]
Patch

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

Thanks for tackling this.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:94
> +    return IDBKeyRange::create(bound.get(), 0, open ? LowerBoundOpen : LowerBoundClosed, UpperBoundOpen);

This is a case where we’d like to hand over ownership, which shows that we could get better efficiency if IDBKeyRange::create took RefPtr&& arguments rather than pointers; we would use WTFMove here instead of .get(). Also, when touching this line of code, the 0 should be changed to nullptr.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:106
> +    return IDBKeyRange::create(0, bound.get(), LowerBoundOpen, open ? UpperBoundOpen : UpperBoundClosed);

Ditto.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:128
> +    return IDBKeyRange::create(lower.get(), upper.get(), lowerOpen ? LowerBoundOpen : LowerBoundClosed, upperOpen ? UpperBoundOpen : UpperBoundClosed);

Same thing again.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.h:51
> +    static Ref<IDBKeyRange> create(IDBKey* lower, IDBKey* upper, LowerBoundType lowerType, UpperBoundType upperType)

The argument type here should be RefPtr&&, not raw pointers.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.h:55
> +    static Ref<IDBKeyRange> create(IDBKey* prpKey);

The argument type here should be RefPtr&&, not a raw pointer.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.h:56
>      ~IDBKeyRange() { }

This line of code should be removed; does no good.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.h:59
> +    RefPtr<IDBKey> lower() const { return m_lower; }
> +    RefPtr<IDBKey> upper() const { return m_upper; }

The return types here should be raw pointers, not RefPtr.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.h:82
> +    IDBKeyRange(IDBKey* lower, IDBKey* upper, LowerBoundType lowerType, UpperBoundType upperType);

These arguments should be RefPtr&&, not *.

> Source/WebCore/Modules/indexeddb/IDBKeyRangeData.cpp:71
> -    return IDBKeyRange::create(lowerKey.maybeCreateIDBKey(), upperKey.maybeCreateIDBKey(), lowerOpen ? IDBKeyRange::LowerBoundOpen : IDBKeyRange::LowerBoundClosed, upperOpen ? IDBKeyRange::UpperBoundOpen : IDBKeyRange::UpperBoundClosed);
> +    return IDBKeyRange::create(lowerKey.maybeCreateIDBKey().get(), upperKey.maybeCreateIDBKey().get(), lowerOpen ? IDBKeyRange::LowerBoundOpen : IDBKeyRange::LowerBoundClosed, upperOpen ? IDBKeyRange::UpperBoundOpen : IDBKeyRange::UpperBoundClosed);

This change adds reference count churn. We should not make this change.

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:314
> +    return IDBKeyRange::create(idbLower.get(), idbUpper.get(), lowerBoundType, upperBoundType);

This should be using WTFMove, not get().

[Gyuyoung Kim]

Created attachment 272398 [details]
Patch

[Build Bot]

Comment on attachment 272398 [details]
Patch

Attachment 272398 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/889060

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 272401 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272398 [details]
Patch

Attachment 272398 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/889061

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 272403 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272398 [details]
Patch

Attachment 272398 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/889078

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 272404 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 272398 [details]
Patch

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

review+ assuming you fix the "double move" mistake in the IDBKeyRange::create and IDBKeyRange::only functions that is causing tests to fail

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:38
> +Ref<IDBKeyRange> IDBKeyRange::create(RefPtr<IDBKey>&& key)

I don’t think it’s legal to call this function with null. Might be worth coming back here later and changing this to take Ref instead of RefPtr.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:40
> -    RefPtr<IDBKey> key = prpKey;
> -    return adoptRef(*new IDBKeyRange(key, key, LowerBoundClosed, UpperBoundClosed));
> +    return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed));

This is wrong. Can’t modify something in two different arguments because the order is undefined. One of the two will probably show up as null. I think that’s why the tests are failing on EWS. Need something like this:

    RefPtr<IDBKey> upperBound = key;
    return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(upperBound), LowerBoundClosed, UpperBoundClosed));

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:70
>      return IDBKeyRange::create(key, key, LowerBoundClosed, UpperBoundClosed);

Better to write:

    return create(key);

Unless we are going to delete the create function above that takes only a single argument, we should use it.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:82
> +    return IDBKeyRange::create(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed);

Same problem as above, but since we have an overloaded create function, I think the right way to deal with it is to just write this:

    return create(WTFMove(key));

Unless we are going to delete the create function above that takes only a single argument, we should use it.

I also noticed that many call sites throughout this file call IDBKeyRange::create, but could just call create, since the call sites are inside IDBKeyRange member functions.

[Gyuyoung Kim]

Created attachment 272441 [details]
Patch

[Gyuyoung Kim]

Created attachment 272446 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #14)
> Comment on attachment 272398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272398&action=review
> 
> review+ assuming you fix the "double move" mistake in the
> IDBKeyRange::create and IDBKeyRange::only functions that is causing tests to
> fail
> 
> > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:38
> > +Ref<IDBKeyRange> IDBKeyRange::create(RefPtr<IDBKey>&& key)
> 
> I don’t think it’s legal to call this function with null. Might be worth
> coming back here later and changing this to take Ref instead of RefPtr.
> 
> > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:40
> > -    RefPtr<IDBKey> key = prpKey;
> > -    return adoptRef(*new IDBKeyRange(key, key, LowerBoundClosed, UpperBoundClosed));
> > +    return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed));
> 
> This is wrong. Can’t modify something in two different arguments because the
> order is undefined. One of the two will probably show up as null. I think
> that’s why the tests are failing on EWS. Need something like this:
> 
>     RefPtr<IDBKey> upperBound = key;
>     return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(upperBound),
> LowerBoundClosed, UpperBoundClosed));
> 
> > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:70
> >      return IDBKeyRange::create(key, key, LowerBoundClosed, UpperBoundClosed);
> 
> Better to write:
> 
>     return create(key);
> 
> Unless we are going to delete the create function above that takes only a
> single argument, we should use it.
> 
> > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:82
> > +    return IDBKeyRange::create(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed);
> 
> Same problem as above, but since we have an overloaded create function, I
> think the right way to deal with it is to just write this:
> 
>     return create(WTFMove(key));
> 
> Unless we are going to delete the create function above that takes only a
> single argument, we should use it.
> 
> I also noticed that many call sites throughout this file call
> IDBKeyRange::create, but could just call create, since the call sites are
> inside IDBKeyRange member functions.

Yes, I noticed to miss that *key* was already passed to the create() through first WTFMove(). Fixed all. Thanks.

[WebKit Commit Bot]

Comment on attachment 272446 [details]
Patch

Clearing flags on attachment: 272446

Committed r197306: <http://trac.webkit.org/changeset/197306>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.