Bug 202090

Summary: clang-tidy: Fix unnecessary copy/ref churn of for loop variables in WebCore
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebCore Misc.Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, calvaris, cdumez, cfleizach, commit-queue, dbates, dino, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jcraig, jdiggs, jer.noble, pdr, philipj, rniwa, sabouhallawa, samuel_white, schenney, sergio, webkit-bug-importer, wenson_hsieh, youennf, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=202069
https://bugs.webkit.org/show_bug.cgi?id=202096
Attachments:
Description Flags
Patch v1
none
Patch v2 none

Description David Kilzer (:ddkilzer) 2019-09-22 04:47:43 PDT
Running clang-tidy on WebCore resulted in these potential performance improvements to prevent object copies or reference churn in for loop variables:

Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:174:27: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
                for (auto request : fpsPssh.initDataBox().requests()) {
                          ^
                     const  &
--
Source/WebCore/svg/SVGStringList.h:78:19: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
        for (auto string : m_items) {
                  ^
             const  &
--
Source/WebCore/./platform/ios/PlatformPasteboardIOS.mm:729:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto type : types)
              ^
         const  &
--
Source/WebCore/./accessibility/AccessibilityObject.cpp:493:19: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
        for (auto misspelling : misspellings) {
                  ^
             const  &
--
Source/WebCore/./accessibility/AccessibilityObject.cpp:985:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto textRange : operation.textRanges) {
              ^
         const  &
--
Source/WebCore/./html/HTMLSlotElement.cpp:119:23: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (RefPtr<Node> node : *assignedNodes) {
                      ^
         const       &
--
Source/WebCore/./layout/inlineformatting/InlineFormattingContextLineLayout.cpp:368:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto floatItem : floats) {
              ^
         const  &
--
Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:664:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto loadResult : _loadResults) {
              ^
         const  &
--
Source/WebCore/html/track/WebVTTParser.cpp:392:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto rule : childRules) {
              ^
         const  &
--
Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:81:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto transceiver : m_transceivers)
              ^
         const  &
Comment 1 Radar WebKit Bug Importer 2019-09-22 04:50:14 PDT
<rdar://problem/55600665>
Comment 2 David Kilzer (:ddkilzer) 2019-09-22 04:58:51 PDT
Created attachment 379343 [details]
Patch v1
Comment 3 Daniel Bates 2019-09-22 09:20:00 PDT
Comment on attachment 379343 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=379343&action=review
This patch looks good.

> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:80
> +    std::vector<rtc::scoped_refptr<webrtc::RtpTransceiverInterface>> transceivers(m_transceivers.size());

This change doubles the size of the final vector. This constructor takes a vector size (read: not capacity).
Comment 4 David Kilzer (:ddkilzer) 2019-09-22 10:13:35 PDT
Comment on attachment 379343 [details]
Patch v1

This causes failures in fast/mediastream tests.  Removing r+/cq? flags.
Comment 5 David Kilzer (:ddkilzer) 2019-09-22 10:29:01 PDT
Created attachment 379347 [details]
Patch v2
Comment 6 David Kilzer (:ddkilzer) 2019-09-22 10:30:59 PDT
Comment on attachment 379343 [details]
Patch v1

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

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:80
>> +    std::vector<rtc::scoped_refptr<webrtc::RtpTransceiverInterface>> transceivers(m_transceivers.size());
> 
> This change doubles the size of the final vector. This constructor takes a vector size (read: not capacity).

It also instantiates m_transceivers.size() objects using the default constructor, which is not what I wanted!

Switched to use std::vector<>::reserve() in Patch v2.

Thanks!
Comment 7 Daniel Bates 2019-09-22 19:12:08 PDT
Comment on attachment 379347 [details]
Patch v2

Thanks for updating the patch. Looks good.
Comment 8 David Kilzer (:ddkilzer) 2019-09-22 19:45:43 PDT
(In reply to David Kilzer (:ddkilzer) from comment #5)
> Created attachment 379347 [details]
> Patch v2

ios-wk2 ews failure:

media/presentationmodechanged-fired-once.html

This test is crashing intermittently without this patch:

<https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media/presentationmodechanged-fired-once.html>
Comment 9 WebKit Commit Bot 2019-09-22 20:46:19 PDT
The commit-queue encountered the following flaky tests while processing attachment 379347 [details]:

imported/w3c/web-platform-tests/websockets/bufferedAmount-unchanged-by-sync-xhr.any.worker.html bug 202003 (author: youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2019-09-22 20:47:03 PDT
Comment on attachment 379347 [details]
Patch v2

Clearing flags on attachment: 379347

Committed r250201: <https://trac.webkit.org/changeset/250201>
Comment 11 WebKit Commit Bot 2019-09-22 20:47:05 PDT
All reviewed patches have been landed.  Closing bug.