Bug 173748

Summary: Avoid moving the same vector multiple times
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bfulgham, cdumez, darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch cdumez: review+

Brent Fulgham
Reported 2017-06-22 16:58:56 PDT
Code inspection revealed that a Vector of String was being WTFMove'd multiple times in a loop. Don't do that!
Attachments
Patch (1.89 KB, patch)
2017-06-22 17:04 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch (7.86 KB, patch)
2017-06-23 10:15 PDT, Brent Fulgham 		cdumez: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2017-06-22 16:59:20 PDT
<rdar://problem/32936804>
Brent Fulgham
Comment 2 2017-06-22 17:04:21 PDT
Created attachment 313675 [details] Patch
Chris Dumez
Comment 3 2017-06-22 18:38:38 PDT
Comment on attachment 313675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313675&action=review > Source/WebKit2/UIProcess/WebProcessProxy.cpp:261 > + Vector<String> topDomains = topPrivatelyControlledDomains; Brady or Darin may have a better idea. Personally, I would pass the Vector as a const Vector& until it actually needs copying (inside WebsiteDataStore::fetchDataForTopPrivatelyControlledDomains()).
Brent Fulgham
Comment 4 2017-06-23 09:34:38 PDT
Comment on attachment 313675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313675&action=review >> Source/WebKit2/UIProcess/WebProcessProxy.cpp:261 >> + Vector<String> topDomains = topPrivatelyControlledDomains; > > Brady or Darin may have a better idea. Personally, I would pass the Vector as a const Vector& until it actually needs copying (inside WebsiteDataStore::fetchDataForTopPrivatelyControlledDomains()). OK. That will make the patch bigger, but might avoid a number of copies if the lowest level of the 'fetch' algorithm skip this work for some reason. I'll try that.
Brent Fulgham
Comment 5 2017-06-23 10:15:02 PDT
Created attachment 313726 [details] Patch
Chris Dumez
Comment 6 2017-06-23 10:34:55 PDT
Comment on attachment 313726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313726&action=review > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:516 > + for (const auto& topPrivatelyControlledDomain : topPrivatelyControlledDomains) { I think we prefer just "auto&" without const.
Brent Fulgham
Comment 7 2017-06-23 11:05:24 PDT
Committed r218753: <http://trac.webkit.org/changeset/218753>
Note You need to log in before you can comment on or make changes to this bug.