Bug 202096 - clang-tidy: Fix unnecessary copy/ref churn of for loop variables in WebKit
Summary: clang-tidy: Fix unnecessary copy/ref churn of for loop variables in WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-22 20:33 PDT by David Kilzer (:ddkilzer)
Modified: 2019-09-25 09:56 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (4.34 KB, patch)
2019-09-22 20:37 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2019-09-22 20:33:03 PDT
Running clang-tidy on WebKit resulted in these potential performance improvements to prevent object copies or reference churn in for loop variables:

Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:514:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto domainName : domains) {
              ^
         const  &
--
Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:215:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto path : WebCore::PathUtilities::pathsWithShrinkWrappedRects(indicatedRects, 0)) {
              ^
         const  &
--
Source/WebKit/UIProcess/ios/WebDataListSuggestionsDropdownIOS.mm:172:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto suggestion : _suggestions) {
              ^
         const  &
--
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2491:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto action : actionsToPerform)
              ^
         const  &
Comment 1 David Kilzer (:ddkilzer) 2019-09-22 20:37:34 PDT
Created attachment 379354 [details]
Patch v1
Comment 2 Radar WebKit Bug Importer 2019-09-22 20:38:02 PDT
<rdar://problem/55609903>
Comment 3 David Kilzer (:ddkilzer) 2019-09-23 06:43:16 PDT
iOS WK2 EWS test failure (timeout):

fast/scrolling/ios/click-events-after-long-press-during-momentum-scroll-in-main-frame.html

This test failure seems like a recent regression (happening in trunk), not related to this patch:

<https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fscrolling%2Fios%2Fclick-events-after-long-press-during-momentum-scroll-in-main-frame.html>
Comment 4 Darin Adler 2019-09-23 09:08:47 PDT
Comment on attachment 379354 [details]
Patch v1

In all these cases i would just use auto& rather than const auto&.
Comment 5 WebKit Commit Bot 2019-09-23 10:05:01 PDT
Comment on attachment 379354 [details]
Patch v1

Clearing flags on attachment: 379354

Committed r250239: <https://trac.webkit.org/changeset/250239>
Comment 6 WebKit Commit Bot 2019-09-23 10:05:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 David Kilzer (:ddkilzer) 2019-09-23 16:13:11 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 379354 [details]
> Patch v1
> 
> In all these cases i would just use auto& rather than const auto&.

Why?  You don't feel `const` adds anything?
Comment 8 Darin Adler 2019-09-25 09:56:12 PDT
(In reply to David Kilzer (:ddkilzer) from comment #7)
> (In reply to Darin Adler from comment #4)
> > In all these cases i would just use auto& rather than const auto&.
> 
> Why?  You don't feel `const` adds anything?

Generally speaking we could put const in many more places in our C++ on local variables and non-reference arguments. In all those cases the benefit is similar: it will catch mistakes where we accidentally modify something that we don't intend to. And in all those cases the cost is similar: makes the code more verbose. In other languages like Swift, where it's easier to define things that are never modified (let vs. var) programmers typically make the other choice. But const in C++ is not the default so we generally don’t use it for simple cases like that. I kind of wish the default was reversed, but this is the language we have.

Here’s an example: Let’s say I am getting a pointer to an object and I plan not to use any non-const functions on it. Typically we don’t use const in a case like that:

    Element* focusedElement = document->focusedElement();
    if (focusedElement && focusedElement->isHappy() && focusedElement->isPatient())

We could add additional const, but we don’t because the benefit is small. It could be written like this:

    const Element* const focusedElement = document->focusedElement();
    if (focusedElement && focusedElement->isHappy() && focusedElement->isPatient())

But typically it’s not.

In the future I hope that most range-based for loops don't mention the type at all, and automatically use a type that won't cause any copying or other changes (like upcasting). Adding this to the language is proposed as N3994 <http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n3994.htm> and <http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n3853.htm> and I hope will eventually get into C++. The most generic way to do this safely is to use "auto&&", but in most cases "auto&" works fine and it's a little bit less exotic looking. Read the rationale in the N3853 document. Until this is available, when I read range-based for statements I see "auto&" and "auto&&" as "almost always right" and also brief. And I see "auto" as "almost always wrong" but also brief.

One other issue: in some of the cases where we are using "const auto&", the expression on the right side is already const. Typing "const auto&" in a case like this communicates "we will not modify the elements in this loop" but it has no effect on how the code is treated.