Summary: | Avoid copying ResourceLoadStatistics objects | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, dbates, ggaren, japhet, wilander | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Chris Dumez
2017-06-29 09:58:42 PDT
Created attachment 314139 [details]
Patch
Comment on attachment 314139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314139&action=review > Source/WebCore/loader/ResourceLoadObserver.cpp:-155 > - auto targetStatistics = m_store->ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain); Note that I stopped calling ensureResourceStatisticsForPrimaryDomain() here. Instead, I merely construct a ResourceLoadStatistics... > Source/WebCore/loader/ResourceLoadObserver.cpp:204 > + m_store->setResourceStatisticsForPrimaryDomain(targetPrimaryDomain, WTFMove(targetStatistics)); ... which gets inserted here. This avoids copying the statistics object and I don't think the call to ensureResourceStatisticsForPrimaryDomain() was really required here. Comment on attachment 314139 [details]
Patch
r=me
Comment on attachment 314139 [details] Patch Clearing flags on attachment: 314139 Committed r218944: <http://trac.webkit.org/changeset/218944> All reviewed patches have been landed. Closing bug. Comment on attachment 314139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314139&action=review >> Source/WebCore/loader/ResourceLoadObserver.cpp:-155 >> - auto targetStatistics = m_store->ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain); > > Note that I stopped calling ensureResourceStatisticsForPrimaryDomain() here. Instead, I merely construct a ResourceLoadStatistics... 'ensureResourceStatisticsForPrimaryDomain()' only returned a new ResourceLoadStatistics if it didn't already exist. I think your revised code prevents us from properly counting, because we never retrieve an existing ResourceLoadStatistic object for a domain so that we can update its counts. Looks like we don't have a good test for that behavior! Reverted r218944 for reason: Optimization is incorrect Committed r218955: <http://trac.webkit.org/changeset/218955> Created attachment 314154 [details]
Patch
Comment on attachment 314154 [details]
Patch
r=me
Comment on attachment 314154 [details] Patch Clearing flags on attachment: 314154 Committed r218966: <http://trac.webkit.org/changeset/218966> All reviewed patches have been landed. Closing bug. |