Migrate WebCore code from Vector::map to std:map
Created attachment 320700 [details] Patch
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 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 &&.
(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.
Created attachment 320768 [details] Patch
Filed bug 176909 for improving WTF::map
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.
Created attachment 320786 [details] Patch
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);
> 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.
(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.
Created attachment 320904 [details] Patch
(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 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 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
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 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
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 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
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 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
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 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...
(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
Created attachment 320945 [details] Patch for landing
Comment on attachment 320945 [details] Patch for landing Clearing flags on attachment: 320945 Committed r222107: <http://trac.webkit.org/changeset/222107>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34693579>
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".