Bug 107262 – Web Inspector: Native Memory Instrumentation: assign class name to the heap graph node automatically

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

[Ilya Tikhonovsky]

I found that class name could be calculated automatically even if we have no-rtti.

patch to follow.

[Ilya Tikhonovsky]

Created attachment 183434 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 183434 [details]
Patch

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

How does it affect the binary size?

> Source/WTF/wtf/MemoryInstrumentation.cpp:140
> +    buffer[length] = 0;

You may be writing to buffer[maxLength] here which is out of the array bounds.

[Ilya Tikhonovsky]

Created attachment 183437 [details]
Patch

[Ilya Tikhonovsky]

comments addressed.

the footprint is about 174240 bytes. 0.1% of binary size.

[Build Bot]

Comment on attachment 183437 [details]
Patch

Attachment 183437 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15938657

[Yury Semikhatsky]

(In reply to comment #4)
> comments addressed.
> 
> the footprint is about 174240 bytes. 0.1% of binary size.

We may want put this behind a preprocessor guard to avoid the overhead when the data is not used.

[Yury Semikhatsky]

Comment on attachment 183437 [details]
Patch

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

> Source/WTF/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Please add an explanation on why need this change.

[Build Bot]

Comment on attachment 183437 [details]
Patch

Attachment 183437 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15947452

New failing tests:
inspector/profiler/memory-instrumentation-cached-images.html

[Ilya Tikhonovsky]

Created attachment 183458 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 183465 [details]
Patch

[Build Bot]

Comment on attachment 183465 [details]
Patch

Attachment 183465 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15945575

[Build Bot]

Comment on attachment 183465 [details]
Patch

Attachment 183465 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15939667

New failing tests:
inspector/profiler/memory-instrumentation-cached-images.html

[Ilya Tikhonovsky]

Created attachment 183483 [details]
screenshot of Native Memory Snapshot for Angry Birds

The final version looks like this.

Interesting facts:

Instrumentation collects data about 76MB of memory.
It is not included V8 memory.
About half of that memory is used for audio Head-related transfer function.
Inspector may affect inspected page memory with his overlay page which is used by Elements panel.
It uses about 13 MB for images.

[Ilya Tikhonovsky]

It doesn't include V8 memory _yet_.

[Ilya Tikhonovsky]

Created attachment 183485 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 183486 [details]
Patch

[Build Bot]

Comment on attachment 183486 [details]
Patch

Attachment 183486 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15938769

New failing tests:
inspector/profiler/memory-instrumentation-cached-images.html

[Build Bot]

Comment on attachment 183486 [details]
Patch

Attachment 183486 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15938793

New failing tests:
inspector/profiler/memory-instrumentation-cached-images.html

[Build Bot]

Comment on attachment 183486 [details]
Patch

Attachment 183486 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15940875

New failing tests:
inspector/profiler/memory-instrumentation-cached-images.html

[Yury Semikhatsky]

(In reply to comment #4)
> comments addressed.
> 
> the footprint is about 174240 bytes. 0.1% of binary size.

How come it is way bigger than the size of all edge names which according to   https://bugs.webkit.org/show_bug.cgi?id=107369#c14 increased the binary size just by 20120 bytes. Given that there are more instrumented fields than instrumented classes I believe we can reduce the overhead.

[Ilya Tikhonovsky]

Created attachment 186225 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #21)
> Created an attachment (id=186225) [details]
> Patch

I made an investigation and found that we calculates the class names for 624 classes, instrumented and not instrumented.

I reduced the signature of the function and moved the type independent code from the header to cpp. As a result the patch adds to the binary about 114k.

We could reduce the footprint of the instrumentation but it should be a separate patch.

[Ilya Tikhonovsky]

Created attachment 186227 [details]
Patch

[Build Bot]

Comment on attachment 186227 [details]
Patch

Attachment 186227 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16340819

[Build Bot]

Comment on attachment 186227 [details]
Patch

Attachment 186227 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16342789

New failing tests:
inspector/profiler/memory-instrumentation-cached-images.html

[Build Bot]

Comment on attachment 186227 [details]
Patch

Attachment 186227 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16336828

New failing tests:
inspector/profiler/memory-instrumentation-cached-images.html

[Ilya Tikhonovsky]

Created attachment 186348 [details]
Patch

[Build Bot]

Comment on attachment 186348 [details]
Patch

Attachment 186348 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16370196

[Ilya Tikhonovsky]

Created attachment 186365 [details]
with fix for windows bot

[Eric Seidel (no email)]

Comment on attachment 186365 [details]
with fix for windows bot

How much does this change binary size?  The reason why we disabled RTTI was that it reduced our binary size by 1/3rd :)

[Ilya Tikhonovsky]

(In reply to comment #30)
> (From update of attachment 186365 [details])
> How much does this change binary size?  The reason why we disabled RTTI was that it reduced our binary size by 1/3rd :)

it is about 119.5MB at the moment. So with this patch it will increase by 0.09%

[Ilya Tikhonovsky]

(In reply to comment #31)
> (In reply to comment #30)
> > (From update of attachment 186365 [details] [details])
> > How much does this change binary size?  The reason why we disabled RTTI was that it reduced our binary size by 1/3rd :)
> 
> it is about 119.5MB at the moment. So with this patch it will increase by 0.09%

According to the build-bot data  http://build.chromium.org/f/chromium/perf/xp-release/sizes/report.html?history=150&rev=-1&graph=chrome.dll the size on win32 is about 54MB.

Yeah, I know this side effect of RTTI flag :)

The alternative solution is to hardcode the class names in reportMemoryUsage and use <unknown> class name for the classes without instrumentation. Also template container class names will loose details like HashSet instead of HashSet<AnObject>.

[Ilya Tikhonovsky]

Created attachment 186558 [details]
with small fix for msvc version

[Yury Semikhatsky]

Comment on attachment 186558 [details]
with small fix for msvc version

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

> Source/WTF/wtf/MemoryInstrumentation.h:-226
> -    callReportObjectInfo(memoryObjectInfo, object, 0, sizeof(T));

callReportObjectInfo seems to be unused, can we remove it?

> Source/WTF/wtf/MemoryInstrumentation.h:254
> +    WTF_EXPORT_PRIVATE static void setInfo(MemoryObjectInfo*, const void* pointer, const char* stringWithClassName, MemoryObjectType, size_t actualSize);

maybe callReportObjectInfo ?

[Ilya Tikhonovsky]

Created attachment 186778 [details]
comments addressed

[Yury Semikhatsky]

Comment on attachment 186778 [details]
comments addressed

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

> Source/WTF/wtf/MemoryInstrumentation.cpp:-64
> -    memoryObjectInfo->reportObjectInfo(pointer, objectType, objectSize);

You need to remove it from header as well.

[Ilya Tikhonovsky]

Created attachment 186785 [details]
comments addressed

[Ilya Tikhonovsky]

Committed r141992: <http://trac.webkit.org/changeset/141992>