Bug 107651 – Add an API for retrieving native memory information without going through the remote inspecting protocol

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Marja Hölttä]

We'd like to retrieve native memory information collected by the Developer tools via a WebKit API. The information we'd like to get is similar to what the native memory snapshot in Developer tools has (category name - amount of memory pairs).

Possible use cases in Chrome:
- memory tests
- about:mem-internals page we're going to create ( crbug.com/168642 )

[Marja Hölttä]

Created attachment 184212 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 184212 [details]
Patch

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

Consider adding a sanity unit test for this under Source/WebKit/chromium/tests/

> Source/WebCore/inspector/InspectorMemoryAgent.h:66
> +    void getProcessMemoryDistributionAsMap(const bool* reportGraph, RefPtr<InspectorObject>& graph, HashMap<String, size_t>* memoryInfo);

You should change reportGraph to a bool, it is "const bool*" just to support optional parameters parsed by the generated protocol messages dispatcher. If you don't need to expose the graph to WebKit you could convert "RefPtr<InspectorObject>& graph" to "RefPtr<InspectorObject>* graph" and just pass 0.

> Source/WebKit/chromium/public/WebView.h:381
> +    virtual WebVector<std::pair<WebString, size_t> > getProcessMemoryDistribution() const = 0;

WebView may not be the best place for this, it should probably go to WebDevToolsAgent

[Marja Hölttä]

Created attachment 184242 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Marja Hölttä]

abarth@, could you have a look at the API additions, if they're acceptable?

[Adam Barth]

Comment on attachment 184242 [details]
Patch

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

> Source/WebKit/chromium/public/WebDevToolsAgent.h:80
> +    virtual WebVector<std::pair<WebString, size_t> > getProcessMemoryDistribution() const = 0;

Does this std::pair represent some sort of object?  Should we use a struct or a class in place of a std::pair?

> Source/WebKit/chromium/public/WebDevToolsAgent.h:106
> +

This change looks spurious.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:645
> +WebVector<std::pair<WebString, size_t> > WebDevToolsAgentImpl::getProcessMemoryDistribution() const
> +{
> +    HashMap<String, size_t> memoryInfo = m_webViewImpl->page()->inspectorController()->getProcessMemoryDistribution();
> +    WebVector<std::pair<WebString, size_t> > memoryInfoVector((size_t)memoryInfo.size());
> +    size_t i = 0;
> +    for (HashMap<String, size_t>::const_iterator it = memoryInfo.begin(); it != memoryInfo.end(); ++it)
> +        memoryInfoVector[i++] = std::make_pair(it->key, it->value);
> +    return memoryInfoVector;
> +}

This looks really inefficient.  Should we return an object that lets you query the map instead?  I guess it depends on how you plan to use this data.

[Marja Hölttä]

Created attachment 184701 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 184701 [details]
Patch

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

Can you do InspectorMemoryAgent refactorings in a separate change?

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:88
> +        m_sizesMap = sizesMap;

m_sizesMap should be a pointer or a reference, otherwise passed sizesMap will remain unchanged.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:645
> +    return memoryInfoVector;

You can pass reference to WebVector<WebMemoryUsageInfo> as a parameter to avoid potential extra copies, the same for hash map returned by getProcessMemoryDistribution.

[Marja Hölttä]

Comment on attachment 184701 [details]
Patch

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

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:88
>> +        m_sizesMap = sizesMap;
> 
> m_sizesMap should be a pointer or a reference, otherwise passed sizesMap will remain unchanged.

Offline discussion: That's how it was before this patch, not going to change it.

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:645
>> +    return memoryInfoVector;
> 
> You can pass reference to WebVector<WebMemoryUsageInfo> as a parameter to avoid potential extra copies, the same for hash map returned by getProcessMemoryDistribution.

Offline discussion: not going to change this, other API funcs return WebVectors, too, let's be consistent...

[Marja Hölttä]

Comment on attachment 184242 [details]
Patch

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

>> Source/WebKit/chromium/public/WebDevToolsAgent.h:80
>> +    virtual WebVector<std::pair<WebString, size_t> > getProcessMemoryDistribution() const = 0;
> 
> Does this std::pair represent some sort of object?  Should we use a struct or a class in place of a std::pair?

I added a struct for this.

>> Source/WebKit/chromium/public/WebDevToolsAgent.h:106
>> +
> 
> This change looks spurious.

Fixed

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:645
>> +}
> 
> This looks really inefficient.  Should we return an object that lets you query the map instead?  I guess it depends on how you plan to use this data.

The keys are arbitrary (and can change as the dev tools add new metrics). So it wouldn't make sense to create a wrapper object with fixed accessors like CSSMemory(), PageMemory() and so on.

Typical usage for this: E.g., in the memory tests, we'd loop through all the keys, and just input them to as data points to some entity which keeps track of memory metrics.

Returning a std::map would also be okay, but it doesn't look like any of the API functions return maps.

[Yury Semikhatsky]

Comment on attachment 184701 [details]
Patch

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

> Source/WebCore/inspector/InspectorController.cpp:438
> +HashMap<String, size_t> InspectorController::getProcessMemoryDistribution() const

style: getProcessMemoryDistribution->processMemoryDistribution

[Marja Hölttä]

Created attachment 184728 [details]
Patch

[Marja Hölttä]

Created attachment 184729 [details]
Patch

[Marja Hölttä]

Created attachment 184749 [details]
Patch

[Marja Hölttä]

I split out the dev tools refactoring parts, and committed them separately.

abarth@: can you have a look at the new patch?

[Adam Barth]

Comment on attachment 184749 [details]
Patch

ok.  Thanks for making an object instead of using a std::pair.

[WebKit Review Bot]

Comment on attachment 184749 [details]
Patch

Clearing flags on attachment: 184749

Committed r140951: <http://trac.webkit.org/changeset/140951>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.