Bug 179361 – Web Inspector: Eliminate unnecessary hash lookups with NetworkResourceData

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 326187 [details]
[PATCH] Proposed Fix

[Sam Weinig]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

(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);

[Joseph Pecoraro]

Created attachment 326233 [details]
[PATCH] Proposed Fix

[Build Bot]

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.

[Blaze Burg]

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

[WebKit Commit Bot]

Comment on attachment 326233 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 326233

Committed r224596: <https://trac.webkit.org/changeset/224596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35562212>