Bug 107369 – Web Inspector: Native Memory Instrumentation: provide edge names to class members in all WebCore instrumented classes.

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

[Ilya Tikhonovsky]

Heap graph snapshot is absolutely unusable if the edges between objects have no names.

[Ilya Tikhonovsky]

Created attachment 183615 [details]
Patch

[Adam Barth]

Comment on attachment 183615 [details]
Patch

This memory instrumentation is kind of getting out of control.  Whenever I work on a data structure, I need to update this bookkeeping code that I don't really understand.  I'm sure I'm doing it wrong.

[Ilya Tikhonovsky]

As you know the purpose of the reportMemoryUsage is to report a) size and name of the class and b) all the pointers from an object of the class to other heap allocated objects and also the pointers declared in the ancestor class(es).

reportMemoryUsage method does the first part automatically with help of MemoryClassInfo object declaration
and does the second part with help of info.addMember calls and call to the base class instrumentation.

This is enough for generating the heap objects graph. See https://bug-107254-attachments.webkit.org/attachment.cgi?id=183490

Corner cases: 
a) non virtual classes with ancestors - we simulated virtual inheritance for them. As far as I know we have two such cases. One for CSS StyleRule hierarchy and the second for CSSValue hierarchy.

b) c style arrays - we need to iterate through them manually.

c) dynamically allocated byte buffers - we report them with help of addRawPointer method.

d) chunks of data with known size and unknown pointer - it happens with classes which we can't instrument for some reason. For that case we use addPrivateBuffer method.

e) a raw not null pointer to an object that already deleted - For this case we use ignoreMember method.
I've found two such cases in StyleResolver and in CachedResourceLoader. Seem to me the both cases are the bugs.
https://bugs.webkit.org/show_bug.cgi?id=106768
https://bugs.webkit.org/show_bug.cgi?id=106759

f) a pointer to an object that is a part of a bigger object - ditto

g) classes with custom new operator - we need to calculate the object size manually. As far as I can remember we have 3 such cases. For example see MemoryInstrumentationString.h

We need to keep all these reportMemoryUsage methods under control. 

1) I use clang plugin which reports me all the members that not yet reported in the classes which have reportMemoryUsage method. I'll extend the list of checks and I hope include it into official builds.

2) we use private build bot which reports the number of pointers in the graph that don't match with tcmalloc data. We've fixed all such cases.

3) the same bot has coverage metric.

[Simon Fraser (smfr)]

What's the end goal? A tool for web developers, or a tool for WebKit developers?

[Ilya Tikhonovsky]

(In reply to comment #4)
> What's the end goal? A tool for web developers, or a tool for WebKit developers?

The goal is a tool for web developers.
We've discussed this question in https://bugs.webkit.org/show_bug.cgi?id=103564 in comments 38-41.

Probably I couldn't convince you that it can be useful even now when it uses edge names and class names.

I have one example:

There is a bug https://code.google.com/p/chromium/issues/detail?id=165126 where inspector crashes with OOM

I tried to troubleshoot the problem with help of native memory snapshot with aggregated memory data and found that it is almost useless.
I knew that the process used a lot of memory. I looked at the snapshot and found that all the memory was used by DOM and Cached resources. It gave me almost no new information.

Then I switched to the detailed version of native heap snapshot which uses all the ui designed for JS heap snapshot. See https://bug-107254-attachments.webkit.org/attachment.cgi?id=183490

In 5 minutes I found that there were two major memory consumers:
The first was half thousand of CachedImage objects with data urls.
The second was the same count of HTMLImageElements that had huge data urls set as their attributes.

Now it is clear that fetching an image and showing it as a thumbnail 32x32 in the network panel was not a good idea. Looking at the detailed snapshot we can say that there was 1) a web developer problem which can be fixed by manually resizing loaded images before setting them as data urls; 2) WebCore uses at least twice as much memory as needed in case of data urls. It is a problem for WebKit developer and needs additional investigation.

Another example: I found that almost any site uses about 6 MB for an image without any url. With help of Native Heap snapshot I found that it was a canvas made by inspector for rulers.
It was a problem in the inspector backend code but I can imagine that a site for photo editing may have the same problem.

We haven't been instrumenting new classes for the last two months. Instead we focused on presenting the collected information in a useful way. In this series of patches I added WebCore class names and WebCore edge names to the heap graph. It seems to me that these names make the tool useful at least for WebKit developers. As the next step we will expose the user generated data in the snapshot. We already expose urls that's why I managed to see the problem with them. Now I see that we need to expose element id, css selectors, etc. After that a web developer will have more clue to connect his page with the memory in the browser.

We already exposed urls for CachedImages in the heap graph that's why I managed to see the problem with the urls. Now I think that exposing element id, css selectors, etc. would let web developers
connect entities in their page code with the memory used by WebKit to represent them.

Some time ago you mentioned that graphic layers may consume considerable amount of memory. Could you provide links to the pages with such problem so we can use them to improve the tool.

[Ilya Tikhonovsky]

Created attachment 184249 [details]
Accelerated Compositing memory screenshot

2simon.fraser

I found a page which uses accelerated compositing and after small fixes in reportMemoryUsage methods and in the UI part
Native Memory Instrumentation is able to show the memory consumed by GraphicsLayer objects.

On the screenshot you can see the number of GraphicsLayerPixels objects, the size of memory they are using. (I should provide a better name for it.)
also you can easily associate a GraphicsLayer item with the DOM element which keeps it alive.

[Simon Fraser (smfr)]

It's nice that your instrumentation shows graphics layer memory now (in chromium at least). However, I still don't think this view is useful for web developers.

[Mike West]

(In reply to comment #7)
> It's nice that your instrumentation shows graphics layer memory now (in chromium at least). However, I still don't think this view is useful for web developers.

It depends on what you mean by "web developers". Game developers want every last ounce of detail we can give them, and then they want more. They're absolutely willing to tune for specific engines, and giving them access to internal bits is valuable.

Moving up the stack, framework developers often want just as much detail about how their UI components effect paint/redraw times, scrolling, etc. We've specifically had conversations with Sencha about this sort of thing (though not this bug in particular). Giving these folks insight into memory usage in a way that maps the data to something they can act upon is valuable. It looks like this patch brings us closer to that than we are now.

How exactly we expose the data to developers is certainly debatable, but I'd argue for making our collection as detailed as we can (within reason) to ensure that we have the raw materials for a view on the data that makes sense to developers.

[Ilya Tikhonovsky]

(In reply to comment #7)
> It's nice that your instrumentation shows graphics layer memory now (in chromium at least). However, I still don't think this view is useful for web developers.

Actually it is called GraphicsLayerChromium because the topmost descendant class with reportMemoryUsage method is GraphicsLayerChromium. The bitmap size data was collected in GraphicsLayer class. So in Safari this object will be called GraphicsLayer.

Did you see Loreena Lee's slides about memory leaks in gmail
https://docs.google.com/presentation/d/1wUVmf78gG-ra5aOxvTfYdiLkdGaR9OhXRnOlIcEmu2s/pub?start=false&loop=false&delayms=3000#slide=id.g1d65bdf6_0_0

They used JS Heap Snapshots with the same UI. And they managed to find and fix more than 10 memory leak points in 2 days without the deep knowledge of the huge codebase.

So I think the current UI is usable. Definitely it is not the best but it could be and will be polished. If you have a better idea about UI feel free to make your proposal.

The amount of the collected data is big and may confuse anybody but from the other hand only 5 days passed since the moment when I saw the full picture the first time.

[Yury Semikhatsky]

(In reply to comment #2)
> (From update of attachment 183615 [details])
> This memory instrumentation is kind of getting out of control.  Whenever I work on a data structure, I need to update this bookkeeping code that I don't really understand.  I'm sure I'm doing it wrong.

We tried to keep it as simple as possible to maintain. For already instrumented classes it should be enough to at most add/remove addMember calls when the members change. For each object we need to know its address in the memory, class name, size and objects that it references. All this multiplied by the problems caused by inheritance lead to the current approach. We may publish some Q&A wiki page describing current instrumentation, would it resolve your concerns?


(In reply to comment #4)
> What's the end goal? A tool for web developers, or a tool for WebKit developers?

The goal is to provide a tool for web developers. But the analizing memory usage is a tough problem especially given that average web developer is not expected to understand low level details of WebKit implementation. The problem can be decomposed into two:
1) collecting as much useful data as we can about the memory graph and
2) figuring out how to represent it in a way that the user can understand.
We have a solution for the first one. Ad for the second part we started with a high-level overview of the memory distribution between components like CSS, DOM, JavaScript etc. and learnt that it provide too few details. Having a powerful JavaScript heap profiler it seems like a natural step to reuse it for the native heap graph. It may not be that useful for web developers but it looks promising from WebKit developer stand point. So we are going to make it possible to use JS heap profiler front-end on the native heap snapshot data at least for browser developers and web developers that express interest and then based on their feedback try to figure out what we could show to web developers.

This change adds useful information to the native heap graph we collect. Potential problems that I see with it are:
* Binary size bloat because many static strings are being added (seem negligible to me).
* Maintenance cost mentioned by Adam, but this is a question about the approach to the instrumentation in general and we are open to suggestions on how to simplify it. This patch doesn't make things worse from this perspective.

Do you have strong objections/concerns on these items? Do you feel that we are on a wrong track with this memory instrumentation overall? If no I think we should go ahead and land this and a couple other patches that make native heap graph more informative and let users try it.

[Simon Fraser (smfr)]

I think this exhaustive memory instrumentation is not the correct approach. I think you should only instrument objects that are known to entrain large amounts of data (e.g. GraphicsLayers, Images etc), and present it in the inspector in such a way that a web author would know what to change to eliminate the memory use.

[Ilya Tikhonovsky]

(In reply to comment #11)
> I think this exhaustive memory instrumentation is not the correct approach. I think you should only instrument objects that are known to entrain large amounts of data (e.g. GraphicsLayers, Images etc), and present it in the inspector in such a way that a web author would know what to change to eliminate the memory use.


When I started to think about memory in WebKit the first thought which came to my mind looked like your suggestion. After M rounds of discussions I came to the conclusion that such ad-hoc solution wouldn’t work properly and is not scalable in a long term. Three possible approaches are described below.




Current approach:
We made a generic visitor class, implemented the visit method in the interesting classes which reports all the class members to the visitor. 

Pros:
1) Instrumentation methods can be validated by clang plugin to make sure e.g. that all members are reported and that base class visit methods are also called.
2) The reported data can be validated against tcmalloc profiler data.
3) It gives the full picture of memory used by WebKit.
4) Collected data can be visualized with a well known schema (like yourkit for java, net. memory profiler for c# and DevTools for Javascript).
5) It has no custom traversal code in the instrumented classes. Insrumentation method is expected to report only objects referenced directly.
6) Containers can be instrumented in a generic way.
7) In all cases except classes with custom new operator it doesn’t require any special counting logic.

Cons:
1) Each of the interesting classes and its retainers need to be instrumented, although the instrumentation is quite simple.




Ad-hoc solution: 
Traverse through webkit objects to the interesting objects via public methods and count the size of memory used by the object and the objects it owns.

Pros:
1) It is easy to provide a tool that would help to detect a particular well-known pattern of excessive memory usage.
2) The instrumentation code can be localized to inspector if the classes provide public API to the corresponding private members.

Cons:
1) Many classes don’t provide access to the internal members. So we can’t calculate exact size and need to extend their public API.
2) In many cases we will have pointer to the base class and will miss everything in the descendant class.
3) We will have no means to validate if the counted size matches actual one.
4) Every time when one changes something in the ownership scheme we will have to reflect the change on the inspector side. It would be hard to have a test checking that the instrumentation is kept in sync with the codebase.
5) Custom traversal code for different memory consumers.
6) Custom counting logic for different classes implemented on inspector side.




Partial ad-hoc solution:

Traverse through webkit objects to the interesting objects via public methods and call an objectSize method on them. The method could access any private data of the instrumented class.

Pros:
1) Memory size can be counted with a better precision.

Cons:
2) All the shared objects (RefPtr) might be counted 2 or more times;
3) The same problem as N3 in ad-hoc solution.
4) The same problem as N4 in ad-hoc solution.
5) The same problem as N5 in ad-hoc solution.

[Eric Seidel (no email)]

I think this is a fine step to try.  It's not clear to me that we'll want this level of detail in all parts, but it's probably better to have it than not.

My only question here is how much of a binary size increase is this?  I can't imagine it's much (strings are small), but I feel like we should check just to make sure.

Otherwise I'm OK with this.  It's no more complicated than any of the rest of what we have.

[Ilya Tikhonovsky]

the binary size increase for this patch is 20120 bytes

[Yury Semikhatsky]

Simon, what are your thoughts on the arguments for the exhaustive instrumentation vs. ad-hoc solutions that Ilya posted? Do they sound convincing to you or do you disagree with them? Please speak up.

This change blocks us. We'd like to land it and collect some real world feedback on using JS heap profiler UI with the native graph.

[Simon Fraser (smfr)]

I'm OK with the exhaustive instrumentation. How you display the results in the UI is up to you.

In future, I think it would be good to mail webkit-dev before making a change so extensive and invasive as the memory instrumentation.

[Yury Semikhatsky]

Comment on attachment 183615 [details]
Patch

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

> Source/WebCore/inspector/HeapGraphSerializer.cpp:109
> +    static int unknownClassNameId = addString("unknown");

If it is static then "unknown" string will only be added first time HeapGraphSerializer is created while we need it to be added in all serializers. Please change it to a field. r- for this

> Source/WebCore/rendering/style/DataRef.h:27
> +#include <wtf/MemoryObjectInfo.h>

Why is it required?

[Ilya Tikhonovsky]

Created attachment 185969 [details]
comments addressed

[Ilya Tikhonovsky]

Created attachment 185972 [details]
unnecessary includes were removed

[Ilya Tikhonovsky]

Created attachment 185980 [details]
setClassName calls were restored

[Ilya Tikhonovsky]

Committed r141570: <http://trac.webkit.org/changeset/141570>