Bug 176432 - Vector should be able to easily create from a list of movable only items
Summary: Vector should be able to easily create from a list of movable only items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-05 16:31 PDT by youenn fablet
Modified: 2017-10-09 21:45 PDT (History)
15 users (show)

See Also:


Attachments
Patch (3.65 KB, patch)
2017-09-05 16:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.70 KB, patch)
2017-09-06 10:27 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.87 KB, patch)
2017-09-12 12:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2017-09-15 13:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (6.44 KB, patch)
2017-10-09 21:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-09-05 16:31:26 PDT
Vector should be able to easily create from a list of movable only items
Comment 1 youenn fablet 2017-09-05 16:33:37 PDT
Created attachment 319953 [details]
Patch
Comment 2 Daniel Bates 2017-09-05 17:14:04 PDT
Comment on attachment 319953 [details]
Patch

The code change  looks good. We just need to add unit tests.
Comment 3 Darin Adler 2017-09-05 18:37:06 PDT
Comment on attachment 319953 [details]
Patch

Why does this have to be a named function rather than a constructor?
Comment 4 Darin Adler 2017-09-05 18:38:30 PDT
Comment on attachment 319953 [details]
Patch

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

> Source/WTF/wtf/Vector.h:632
> +        result.asanSetInitialBufferSizeTo(size);

I don’t understand why this is needed. The reserveInitialCapacity call should take care of this.

> Source/WTF/wtf/Vector.h:810
> +        uncheckedAppend(std::forward<U>(item));
> +        uncheckedAppend(std::forward<Items>(items)...);

Seems like we miss an opportunity here to optimize so we bump the size only once. This is going to increment the size over and over again, I think. Need to look at the generated code I guess.
Comment 5 youenn fablet 2017-09-06 10:27:14 PDT
Created attachment 320037 [details]
Patch
Comment 6 youenn fablet 2017-09-06 10:31:20 PDT
Thanks for the review.

(In reply to Daniel Bates from comment #2)
> Comment on attachment 319953 [details]
> Patch
> 
> The code change  looks good. We just need to add unit tests.

Added some.

(In reply to Darin Adler from comment #3)
> Comment on attachment 319953 [details]
> Patch
> 
> Why does this have to be a named function rather than a constructor?

It would be a bit nicer to use but it conflicts with other constructors.
There might be ways to disambiguate or restrict this constructor but I am not sure this is worth it.
I wonder whether latest C++ progress might help.

(In reply to Darin Adler from comment #4)
> Comment on attachment 319953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319953&action=review
> 
> > Source/WTF/wtf/Vector.h:632
> > +        result.asanSetInitialBufferSizeTo(size);
> 
> I don’t understand why this is needed. The reserveInitialCapacity call
> should take care of this.

Right, changed here and in initializer list constructor.

> 
> > Source/WTF/wtf/Vector.h:810
> > +        uncheckedAppend(std::forward<U>(item));
> > +        uncheckedAppend(std::forward<Items>(items)...);
> 
> Seems like we miss an opportunity here to optimize so we bump the size only
> once. This is going to increment the size over and over again, I think. Need
> to look at the generated code I guess.

Added the optimization
Comment 7 Sam Weinig 2017-09-06 11:54:34 PDT
Comment on attachment 320037 [details]
Patch

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

> Tools/ChangeLog:8
> +        fix-176432

This could be a better comment. :)
Comment 8 Saam Barati 2017-09-06 14:44:15 PDT
Comment on attachment 320037 [details]
Patch

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

> Source/WTF/wtf/Vector.h:817
> +        auto ptr = std::addressof(value);
> +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));

Why is this needed instead of just:
new (NotNull, begin() + position) T(std::forward<U>(value));
Comment 9 youenn fablet 2017-09-06 16:33:00 PDT
(In reply to Saam Barati from comment #8)
> Comment on attachment 320037 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320037&action=review
> 
> > Source/WTF/wtf/Vector.h:817
> > +        auto ptr = std::addressof(value);
> > +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));
> 
> Why is this needed instead of just:
> new (NotNull, begin() + position) T(std::forward<U>(value));

I am not sure WebKit is using Vector of classes with overloaded &.
Anders might know more about this since he added it.
Comment 10 Anders Carlsson 2017-09-07 11:48:21 PDT
(In reply to youenn fablet from comment #9)
> (In reply to Saam Barati from comment #8)
> > Comment on attachment 320037 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320037&action=review
> > 
> > > Source/WTF/wtf/Vector.h:817
> > > +        auto ptr = std::addressof(value);
> > > +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));
> > 
> > Why is this needed instead of just:
> > new (NotNull, begin() + position) T(std::forward<U>(value));
> 
> I am not sure WebKit is using Vector of classes with overloaded &.
> Anders might know more about this since he added it.

COMPtr has an overloaded & operator, and I had planned to add one to RetainPtr as well, since a lot of the CF APIs use out-pointers.
Comment 11 Saam Barati 2017-09-07 15:29:37 PDT
(In reply to youenn fablet from comment #9)
> (In reply to Saam Barati from comment #8)
> > Comment on attachment 320037 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320037&action=review
> > 
> > > Source/WTF/wtf/Vector.h:817
> > > +        auto ptr = std::addressof(value);
> > > +        new (NotNull, begin() + position) T(std::forward<U>(*ptr));
> > 
> > Why is this needed instead of just:
> > new (NotNull, begin() + position) T(std::forward<U>(value));
> 
> I am not sure WebKit is using Vector of classes with overloaded &.
> Anders might know more about this since he added it.

Right, that makes sense. But why do we even bother getting the address then dereferencing it? Why can't we just forward it along? Maybe I'm missing something
Comment 12 youenn fablet 2017-09-12 08:17:43 PDT
> Right, that makes sense. But why do we even bother getting the address then
> dereferencing it? Why can't we just forward it along? Maybe I'm missing
> something

I read too fast, you are right, I will simplify uncheckedAppend and uncheckedInitialize accordingly.
Comment 13 youenn fablet 2017-09-12 12:44:34 PDT
Created attachment 320563 [details]
Patch
Comment 14 Saam Barati 2017-09-13 18:10:56 PDT
Comment on attachment 320563 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Vector should be able to easily create from a list of movable only items

I think the name of this bug is slightly misleading. At least I misread it in my head. I don't think you're not requiring things that are rvalue references here. It looks like you're just using normal forwarding semantics.

> Source/WTF/wtf/Vector.h:1185
> +    asanSetInitialBufferSizeTo(initialCapacity);

This seems wrong if the above branch is not taken.
Also, I think this would break asan finding bugs like:
```
Vector<T, 0> v;
v.reserveInitialCapacity(10);
v[0] = T();
```
Comment 15 Saam Barati 2017-09-13 18:11:31 PDT
Comment on attachment 320563 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:111
> +TEST(WTF_Vector, ConstructWithFromMoveOnly)

I think it's worth adding tests for non rvalue references.
Comment 16 youenn fablet 2017-09-15 13:41:42 PDT
Created attachment 320959 [details]
Patch
Comment 17 youenn fablet 2017-09-15 13:44:22 PDT
Thanks Saam, I updated the patch according your comments.

As of the name of the bug, the issue was brought up for Movable only types although you are right this routine is applicable to more than that.
Comment 18 Saam Barati 2017-10-03 13:08:46 PDT
(In reply to youenn fablet from comment #16)
> Created attachment 320959 [details]
> Patch

Did you want this patch reviewed?
Comment 19 youenn fablet 2017-10-04 05:41:30 PDT
(In reply to Saam Barati from comment #18)
> (In reply to youenn fablet from comment #16)
> > Created attachment 320959 [details]
> > Patch
> 
> Did you want this patch reviewed?

After some refactoring, I no longer need this.
It makes sense to have it in bugzilla if somebody in the future needs that but I am not sure we should land it.
Comment 20 youenn fablet 2017-10-09 12:38:01 PDT
I have a use case for it.
Comment 21 youenn fablet 2017-10-09 21:01:36 PDT
Created attachment 323282 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2017-10-09 21:44:54 PDT
Comment on attachment 323282 [details]
Patch for landing

Clearing flags on attachment: 323282

Committed r223122: <http://trac.webkit.org/changeset/223122>
Comment 23 WebKit Commit Bot 2017-10-09 21:44:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2017-10-09 21:45:56 PDT
<rdar://problem/34903219>