Bug 176860 – Move code using Vector::map to WTF:map

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Migrate WebCore code from Vector::map to std:map

[youenn fablet]

Created attachment 320700 [details]
Patch

[Jer Noble]

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.

[Jer Noble]

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 &&.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 320768 [details]
Patch

[youenn fablet]

Filed bug 176909 for improving WTF::map

[youenn fablet]

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.

[youenn fablet]

Created attachment 320786 [details]
Patch

[Jer Noble]

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);

[youenn fablet]

> 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.

[Jer Noble]

(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.

[youenn fablet]

Created attachment 320904 [details]
Patch

[youenn fablet]

(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.

[Jer Noble]

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.)

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

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...

[youenn fablet]

(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

[youenn fablet]

Created attachment 320945 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 320945 [details]
Patch for landing

Clearing flags on attachment: 320945

Committed r222107: <http://trac.webkit.org/changeset/222107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693579>

[Darin Adler]

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".