Bug 179361 - Web Inspector: Eliminate unnecessary hash lookups with NetworkResourceData
Summary: Web Inspector: Eliminate unnecessary hash lookups with NetworkResourceData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-06 19:54 PST by Joseph Pecoraro
Modified: 2017-11-15 09:40 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (8.05 KB, patch)
2017-11-06 19:55 PST, Joseph Pecoraro
sam: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (12.37 KB, patch)
2017-11-07 11:56 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-11-06 19:54:27 PST
Eliminate unnecessary hash lookups with NetworkResourceData

There are a few places where we do multiple hash lookups on NetworkResourceData that we don't need:

    willSendRequest: (combine these into 1 call)

        m_resourcesData->resourceCreated(requestId, m_pageAgent->loaderId(&loader));
        m_resourcesData->setResourceType(requestId, type);

    didReceiveResponse: (combine these into 1 call)

        m_resourcesData->responseReceived(requestId, m_pageAgent->frameId(loader.frame()), response);
        m_resourcesData->setResourceType(requestId, type);

    didLoadResourceFromMemoryCache: (combine these into 1 call)

        m_resourcesData->resourceCreated(requestId, loaderId);
        m_resourcesData->addCachedResource(requestId, &resource);

Combining will eliminate a hash lookup in each case.
Comment 1 Joseph Pecoraro 2017-11-06 19:55:50 PST
Created attachment 326187 [details]
[PATCH] Proposed Fix
Comment 2 Sam Weinig 2017-11-07 01:11:15 PST
Comment on attachment 326187 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/NetworkResourcesData.cpp:131
> +    auto* resourceData = new ResourceData(requestId, loaderId);
> +    resourceData->setType(type);
> +    m_requestIdToResourceDataMap.set(requestId, resourceData);

An additional cleanup that you might want to do is change m_requestIdToResourceDataMap to store a std::unique_ptr rather than a raw ptr for its value.

> Source/WebCore/inspector/NetworkResourcesData.cpp:134
> +void NetworkResourcesData::resourceCreated(const String& requestId, const String& loaderId, CachedResource* cachedResource)

Can this take the CachedResource by reference?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:351
> +    String loaderId = m_pageAgent->loaderId(&loader);

Can use auto.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:444
> +    String frameId = m_pageAgent->frameId(loader.frame());
> +    String loaderId = m_pageAgent->loaderId(&loader);

Can use auto.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:531
> +    String loaderId = m_pageAgent->loaderId(&loader);
> +    String frameId = m_pageAgent->frameId(loader.frame());

Can use auto.
Comment 3 Joseph Pecoraro 2017-11-07 11:32:31 PST
Comment on attachment 326187 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/inspector/NetworkResourcesData.cpp:131
>> +    m_requestIdToResourceDataMap.set(requestId, resourceData);
> 
> An additional cleanup that you might want to do is change m_requestIdToResourceDataMap to store a std::unique_ptr rather than a raw ptr for its value.

This is an excellent idea, I should have addressed this in this change anyways instead of allowing `new` / `delete` in our code. I'll put up a new patch.

>> Source/WebCore/inspector/NetworkResourcesData.cpp:134
>> +void NetworkResourcesData::resourceCreated(const String& requestId, const String& loaderId, CachedResource* cachedResource)
> 
> Can this take the CachedResource by reference?

Yep.
Comment 4 Joseph Pecoraro 2017-11-07 11:50:21 PST
(In reply to Joseph Pecoraro from comment #0)
> Eliminate unnecessary hash lookups with NetworkResourceData
> 
> There are a few places where we do multiple hash lookups on
> NetworkResourceData that we don't need:

Another multiple-hash-lookup we can remove is:

    ensureNoDataForRequestId

        ResourceData* resourceData = resourceDataForRequestId(requestId);
        ...
        m_requestIdToResourceDataMap.remove(requestId);
Comment 5 Joseph Pecoraro 2017-11-07 11:56:41 PST
Created attachment 326233 [details]
[PATCH] Proposed Fix
Comment 6 Build Bot 2017-11-07 11:58:16 PST
Attachment 326233 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/NetworkResourcesData.cpp:273:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 BJ Burg 2017-11-08 13:40:50 PST
Comment on attachment 326233 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebCore/inspector/NetworkResourcesData.cpp:266
> +            return entry.value->loaderId() != loaderId;

v. cool
Comment 8 WebKit Commit Bot 2017-11-08 14:01:18 PST
Comment on attachment 326233 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 326233

Committed r224596: <https://trac.webkit.org/changeset/224596>
Comment 9 WebKit Commit Bot 2017-11-08 14:01:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2017-11-15 09:40:45 PST
<rdar://problem/35562212>