Bug 176860 - Move code using Vector::map to WTF:map
Summary: Move code using Vector::map to WTF:map
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-13 10:56 PDT by youenn fablet
Modified: 2017-10-01 20:19 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.87 KB, patch)
2017-09-13 15:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2017-09-14 08:34 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2017-09-14 10:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2017-09-15 08:50 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.37 MB, application/zip)
2017-09-15 10:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.50 MB, application/zip)
2017-09-15 10:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (2.17 MB, application/zip)
2017-09-15 10:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.36 MB, application/zip)
2017-09-15 10:31 PDT, Build Bot
no flags Details
Patch for landing (5.94 KB, patch)
2017-09-15 12:38 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-13 10:56:07 PDT
Migrate WebCore code from Vector::map to std:map
Comment 1 youenn fablet 2017-09-13 15:40:44 PDT
Created attachment 320700 [details]
Patch
Comment 2 Jer Noble 2017-09-13 20:27:27 PDT
Comment on attachment 320700 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:803
> -    m_mediaContentTypesRequiringHardwareSupport = contentTypes.split(":").map(ContentType::create);
> +    m_mediaContentTypesRequiringHardwareSupport = WTF::map(contentTypes.split(":"), [] (auto&& type) {
> +        return ContentType { WTFMove(type) };
> +    });

I think that the aesthetics of this call are reason enough to leave Vector::map() alone.  You've taken a reasonably readable line of code and made it almost incomprehensible.
Comment 3 Jer Noble 2017-09-13 23:23:27 PDT
Comment on attachment 320700 [details]
Patch

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

>> Source/WebCore/page/Settings.cpp:803
>> -    m_mediaContentTypesRequiringHardwareSupport = contentTypes.split(":").map(ContentType::create);
>> +    m_mediaContentTypesRequiringHardwareSupport = WTF::map(contentTypes.split(":"), [] (auto&& type) {
>> +        return ContentType { WTFMove(type) };
>> +    });
> 
> I think that the aesthetics of this call are reason enough to leave Vector::map() alone.  You've taken a reasonably readable line of code and made it almost incomprehensible.

Also, your new WTF::map() function doesn't work when the transformer is a static function taking a &&, or this could have more easily been written by changing ContentType::create() to take a &&.
Comment 4 youenn fablet 2017-09-14 08:28:57 PDT
(In reply to Jer Noble from comment #3)
> Comment on attachment 320700 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320700&action=review
> 
> >> Source/WebCore/page/Settings.cpp:803
> >> -    m_mediaContentTypesRequiringHardwareSupport = contentTypes.split(":").map(ContentType::create);
> >> +    m_mediaContentTypesRequiringHardwareSupport = WTF::map(contentTypes.split(":"), [] (auto&& type) {
> >> +        return ContentType { WTFMove(type) };
> >> +    });
> > 
> > I think that the aesthetics of this call are reason enough to leave Vector::map() alone.  You've taken a reasonably readable line of code and made it almost incomprehensible.
> 
> Also, your new WTF::map() function doesn't work when the transformer is a
> static function taking a &&, or this could have more easily been written by
> changing ContentType::create() to take a &&.

Yes, I believe this can be added to WTF::map by taking a Transformer&&.
I'll add this.

That said, rereading the code here, we should not create a Vector<String> in the first place.
It is just better to use the lambda-version of split so that we only create a Vector<ContentType>.
I'll rewrite the patch accordingly.
Comment 5 youenn fablet 2017-09-14 08:34:29 PDT
Created attachment 320768 [details]
Patch
Comment 6 youenn fablet 2017-09-14 08:54:35 PDT
Filed bug 176909 for improving WTF::map
Comment 7 youenn fablet 2017-09-14 08:55:42 PDT
Patch fails, apparently there is a new use of Vector::map() in the code base.
I'll rebase this patch.
Let me know in the meantime if the split() approach works for you.
Comment 8 youenn fablet 2017-09-14 10:29:41 PDT
Created attachment 320786 [details]
Patch
Comment 9 Jer Noble 2017-09-14 11:07:09 PDT
Comment on attachment 320786 [details]
Patch

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

> Source/WTF/wtf/Vector.h:-1484
> -template<typename MapFunction, typename R>
> -inline Vector<R> Vector<T, inlineCapacity, OverflowHandler, minCapacity, Malloc>::map(MapFunction mapFunction) const
> -{
> -    Vector<R> result;
> -    result.reserveInitialCapacity(size());
> -    for (size_t i = 0; i < size(); ++i)
> -        result.uncheckedAppend(mapFunction(at(i)));
> -    return result;
> -}

I still don't understand why you're deleting the Vector::map function.

> Source/WebCore/page/Settings.cpp:804
> -    m_mediaContentTypesRequiringHardwareSupport = contentTypes.split(":").map(ContentType::create);
> +    m_mediaContentTypesRequiringHardwareSupport = { };
> +    contentTypes.split(':', false, [&] (StringView view) {
> +        m_mediaContentTypesRequiringHardwareSupport.append(ContentType { view.toString() });
> +    });

This is arguably a performance degradation, not an improvement. You've eliminated one refcount churn per string, but at the cost of eliminating a reserveInitialCapacity() and uncheckedAppend() for the resulting array. (The Vector holding the results of split() still doesn't get its initial capacity reserved, but since it's effectively a Vector of Ref's, the performance implications there are less than resizing a Vector of much larger structs.) And the cost of a Vector resize will always be larger than a refCount change.

Also, since this function is literally only ever called once at startup, eliminating refCount churn is not the most productive thing to do here.

You didn't change the ContentType::create() method, like I suggested.  But if you had, this could have been written:

m_mediaContentTypesRequiringHardwareSupport = WTF::map(contentTypes.split(':'), ContentType::create);

Or better yet:

m_mediaContentTypesRequiringHardwareSupport = contentTypes.split(':').moveMap(ContentType::create);
Comment 10 youenn fablet 2017-09-14 11:20:15 PDT
> Also, since this function is literally only ever called once at startup,
> eliminating refCount churn is not the most productive thing to do here.

The goal is not really eliminating count churning here.

When doing contentTypes.split(':'), we create a Vector<String>, allocate memory for it, call append strings on it several times.
Then we map it efficiently to Vector<ContentType> and we release the Vector<String> memory shortly after.

When doing contentTypes.split(':', []...), there is no Vector<String> allocated.
We are just calling the lambda taking a StringView as parameter, which will thus feed the Vector<ContentType> directly.
Comment 11 Jer Noble 2017-09-14 11:41:31 PDT
(In reply to youenn fablet from comment #10)
> > Also, since this function is literally only ever called once at startup,
> > eliminating refCount churn is not the most productive thing to do here.
> 
> The goal is not really eliminating count churning here.
> 
> When doing contentTypes.split(':'), we create a Vector<String>, allocate
> memory for it, call append strings on it several times.

Sure, but as I pointed out, the memory for the Vector is the size of a RefPtr per item.

> Then we map it efficiently to Vector<ContentType> and we release the
> Vector<String> memory shortly after.
> 
> When doing contentTypes.split(':', []...), there is no Vector<String>
> allocated.
> We are just calling the lambda taking a StringView as parameter, which will
> thus feed the Vector<ContentType> directly.

But again, as I pointed out, you will likely have to reallocate the resulting Vector at least once, and depending on the number of content types, possibly multiple times, thus negating any performance benefit of removing the allocation for the first Vector, since the second vector is larger.
Comment 12 youenn fablet 2017-09-15 08:50:13 PDT
Created attachment 320904 [details]
Patch
Comment 13 youenn fablet 2017-09-15 08:55:46 PDT
(In reply to youenn fablet from comment #12)
> Created attachment 320904 [details]
> Patch

Moved code from String::split to StringView::split, which helps a bit in term of readability.
Did not remove Vector::map implementation although I do not really see the benefit to keep it as of now.

I plan at some point to beef up WTF::map so that it can take a HashMap as input which would be converted to Vector. Current implementation probably almost works except for inferring iterated item and result item types.

I also needed yesterday to do a cross thread safe copy of a HashMap.
Maybe a map taking an HashMap and returning a HashMap would be good too.
Could be the same WTF::map based on the result item type or another function.
Comment 14 Jer Noble 2017-09-15 09:04:17 PDT
Comment on attachment 320904 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:805
> -    m_mediaContentTypesRequiringHardwareSupport = contentTypes.split(":").map(ContentType::create);
> +    StringView contentTypesView { contentTypes };
> +
> +    m_mediaContentTypesRequiringHardwareSupport.shrink(0);
> +    for (auto type : contentTypesView.split(':'))
> +        m_mediaContentTypesRequiringHardwareSupport.append(ContentType { type.toString() });

nit: I don't think you need the local variable here; this could be written:

for (auto type : StringView(contentTypes).split(':'))

I'm pretty sure the right hand side of the iterator statement stays alive across the whole for loop. (I could be wrong though.)
Comment 15 Build Bot 2017-09-15 10:01:42 PDT
Comment on attachment 320904 [details]
Patch

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

New failing tests:
media/media-can-play-mpeg4-video.html
media/media-can-play-wav-audio.html
media/W3C/audio/canPlayType/canPlayType_supported_but_no_codecs_parameter_2.html
media/W3C/video/canPlayType/canPlayType_supported_but_no_codecs_parameter_3.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/mime-types/canPlayType.html
media/media-can-play-mpeg-audio.html
Comment 16 Build Bot 2017-09-15 10:01:44 PDT
Created attachment 320921 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-09-15 10:06:58 PDT
Comment on attachment 320904 [details]
Patch

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

New failing tests:
media/media-can-play-mpeg4-video.html
media/media-can-play-wav-audio.html
media/W3C/audio/canPlayType/canPlayType_supported_but_no_codecs_parameter_2.html
media/W3C/video/canPlayType/canPlayType_supported_but_no_codecs_parameter_3.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/mime-types/canPlayType.html
media/media-can-play-mpeg-audio.html
Comment 18 Build Bot 2017-09-15 10:06:59 PDT
Created attachment 320922 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-09-15 10:21:07 PDT
Comment on attachment 320904 [details]
Patch

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

New failing tests:
media/media-can-play-mpeg4-video.html
media/media-can-play-wav-audio.html
media/W3C/audio/canPlayType/canPlayType_supported_but_no_codecs_parameter_2.html
media/W3C/video/canPlayType/canPlayType_supported_but_no_codecs_parameter_3.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/mime-types/canPlayType.html
media/media-can-play-mpeg-audio.html
Comment 20 Build Bot 2017-09-15 10:21:09 PDT
Created attachment 320924 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Build Bot 2017-09-15 10:31:36 PDT
Comment on attachment 320904 [details]
Patch

Attachment 320904 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4559375

New failing tests:
media/media-can-play-mpeg4-video.html
media/W3C/video/canPlayType/canPlayType_supported_but_no_codecs_parameter_3.html
media/media-can-play-mpeg-audio.html
media/W3C/audio/canPlayType/canPlayType_supported_but_no_codecs_parameter_2.html
Comment 22 Build Bot 2017-09-15 10:31:37 PDT
Created attachment 320925 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 23 youenn fablet 2017-09-15 11:34:01 PDT
Comment on attachment 320904 [details]
Patch

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

> Source/WebCore/platform/ContentType.cpp:-100
> -    return parameter(codecsParameter()).split(',').map(stripHTMLWhiteSpace);

Forgot to wrap through parameter(), hence the test failures...
Comment 24 youenn fablet 2017-09-15 11:35:00 PDT
(In reply to Jer Noble from comment #14)
> Comment on attachment 320904 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320904&action=review
> 
> > Source/WebCore/page/Settings.cpp:805
> > -    m_mediaContentTypesRequiringHardwareSupport = contentTypes.split(":").map(ContentType::create);
> > +    StringView contentTypesView { contentTypes };
> > +
> > +    m_mediaContentTypesRequiringHardwareSupport.shrink(0);
> > +    for (auto type : contentTypesView.split(':'))
> > +        m_mediaContentTypesRequiringHardwareSupport.append(ContentType { type.toString() });
> 
> nit: I don't think you need the local variable here; this could be written:
> 
> for (auto type : StringView(contentTypes).split(':'))
> 
> I'm pretty sure the right hand side of the iterator statement stays alive
> across the whole for loop. (I could be wrong though.)

OK
Comment 25 youenn fablet 2017-09-15 12:38:38 PDT
Created attachment 320945 [details]
Patch for landing
Comment 26 WebKit Commit Bot 2017-09-15 13:09:19 PDT
Comment on attachment 320945 [details]
Patch for landing

Clearing flags on attachment: 320945

Committed r222107: <http://trac.webkit.org/changeset/222107>
Comment 27 WebKit Commit Bot 2017-09-15 13:09:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Radar WebKit Bug Importer 2017-09-27 12:36:13 PDT
<rdar://problem/34693579>
Comment 29 Darin Adler 2017-10-01 20:19:37 PDT
Comment on attachment 320904 [details]
Patch

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

>>> Source/WebCore/page/Settings.cpp:805
>>> +        m_mediaContentTypesRequiringHardwareSupport.append(ContentType { type.toString() });
>> 
>> nit: I don't think you need the local variable here; this could be written:
>> 
>> for (auto type : StringView(contentTypes).split(':'))
>> 
>> I'm pretty sure the right hand side of the iterator statement stays alive across the whole for loop. (I could be wrong though.)
> 
> OK

Here‘s the tricky part: The ultimate result of the right hand side does stay alive across the whole for loop. The same is not true for subexpressions, though. This case is fine; it’s easy to get it wrong with more complex expressions.

The boost people ran into this here: <https://svn.boost.org/trac10/ticket/7630>. Here <http://en.cppreference.com/w/cpp/language/range-for> the warning consists of the following words "beware that the lifetime of any temporary within range_expression is not extended".