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 )
Created attachment 184212 [details] Patch
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
Created attachment 184242 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
abarth@, could you have a look at the API additions, if they're acceptable?
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.
Created attachment 184701 [details] Patch
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.
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...
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.
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
Created attachment 184728 [details] Patch
Created attachment 184729 [details] Patch
Created attachment 184749 [details] Patch
I split out the dev tools refactoring parts, and committed them separately. abarth@: can you have a look at the new patch?
Comment on attachment 184749 [details] Patch ok. Thanks for making an object instead of using a std::pair.
Comment on attachment 184749 [details] Patch Clearing flags on attachment: 184749 Committed r140951: <http://trac.webkit.org/changeset/140951>
All reviewed patches have been landed. Closing bug.