Bug 107262

Summary:

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

Product:

WebKit

Reporter:

Ilya Tikhonovsky <loislo>

Component:

Web Inspector (Deprecated)

Assignee:

Ilya Tikhonovsky <loislo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, benjamin, buildbot, eric, inferno, keishi, loislo, ojan.autocc, pfeldman, pmuellr, rniwa, vsevik, web-inspector-bugs, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

107254

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
screenshot of Native Memory Snapshot for Angry Birds	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
with fix for windows bot	none
with small fix for msvc version	none
comments addressed	none
comments addressed	yurys: review+

Ilya Tikhonovsky

Reported 2013-01-18 05:06:23 PST

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

Attachments
Patch (3.98 KB, patch) 2013-01-18 05:53 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
Patch (3.98 KB, patch) 2013-01-18 06:06 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
Patch (7.15 KB, patch) 2013-01-18 07:47 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
Patch (7.37 KB, patch) 2013-01-18 08:23 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
screenshot of Native Memory Snapshot for Angry Birds (1.07 MB, image/png) 2013-01-18 09:30 PST, Ilya Tikhonovsky	no flags	Details
Patch (8.19 KB, patch) 2013-01-18 09:42 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
Patch (8.10 KB, patch) 2013-01-18 09:43 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
Patch (9.50 KB, patch) 2013-02-02 05:16 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
Patch (9.48 KB, patch) 2013-02-02 05:40 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
Patch (9.42 KB, patch) 2013-02-04 04:29 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
with fix for windows bot (9.84 KB, patch) 2013-02-04 06:19 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
with small fix for msvc version (9.80 KB, patch) 2013-02-04 23:51 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
comments addressed (10.55 KB, patch) 2013-02-06 01:00 PST, Ilya Tikhonovsky	no flags	Details Formatted Diff Diff
comments addressed (10.94 KB, patch) 2013-02-06 01:34 PST, Ilya Tikhonovsky	yurys: review+	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Ilya Tikhonovsky

Comment 1 2013-01-18 05:53:39 PST

Created attachment 183434 [details] Patch

Yury Semikhatsky

Comment 2 2013-01-18 06:02:17 PST

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

Comment 3 2013-01-18 06:06:47 PST

Created attachment 183437 [details] Patch

Ilya Tikhonovsky

Comment 4 2013-01-18 06:07:37 PST

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

Build Bot

Comment 5 2013-01-18 06:31:01 PST

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

Yury Semikhatsky

Comment 6 2013-01-18 06:37:08 PST

(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 7 2013-01-18 06:45:37 PST

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 8 2013-01-18 07:45:11 PST

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

Comment 9 2013-01-18 07:47:02 PST

Created attachment 183458 [details] Patch

Ilya Tikhonovsky

Comment 10 2013-01-18 08:23:12 PST

Created attachment 183465 [details] Patch

Build Bot

Comment 11 2013-01-18 09:17:59 PST

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 12 2013-01-18 09:26:55 PST

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

Comment 13 2013-01-18 09:30:25 PST

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

Comment 14 2013-01-18 09:34:22 PST

It doesn't include V8 memory _yet_.

Ilya Tikhonovsky

Comment 15 2013-01-18 09:42:09 PST

Created attachment 183485 [details] Patch

Ilya Tikhonovsky

Comment 16 2013-01-18 09:43:44 PST

Created attachment 183486 [details] Patch

Build Bot

Comment 17 2013-01-18 12:21:36 PST

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 18 2013-01-18 13:20:52 PST

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 19 2013-01-18 14:56:53 PST

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

Comment 20 2013-02-01 03:22:38 PST

(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

Comment 21 2013-02-02 05:16:13 PST

Created attachment 186225 [details] Patch

Ilya Tikhonovsky

Comment 22 2013-02-02 05:30:20 PST

(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

Comment 23 2013-02-02 05:40:06 PST

Created attachment 186227 [details] Patch

Build Bot

Comment 24 2013-02-02 07:15:18 PST

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 25 2013-02-02 07:39:33 PST

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 26 2013-02-02 07:46:36 PST

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

Comment 27 2013-02-04 04:29:08 PST

Created attachment 186348 [details] Patch

Build Bot

Comment 28 2013-02-04 06:13:13 PST

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

Ilya Tikhonovsky

Comment 29 2013-02-04 06:19:21 PST

Created attachment 186365 [details] with fix for windows bot

Eric Seidel (no email)

Comment 30 2013-02-04 08:33:08 PST

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

Comment 31 2013-02-04 21:28:05 PST

(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

Comment 32 2013-02-04 22:10:13 PST

(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

Comment 33 2013-02-04 23:51:01 PST

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

Yury Semikhatsky

Comment 34 2013-02-05 07:49:11 PST

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

Comment 35 2013-02-06 01:00:19 PST

Created attachment 186778 [details] comments addressed

Yury Semikhatsky

Comment 36 2013-02-06 01:23:38 PST

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

Comment 37 2013-02-06 01:34:05 PST

Created attachment 186785 [details] comments addressed

Ilya Tikhonovsky

Comment 38 2013-02-06 05:31:13 PST

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

Note You need to log in before you can comment on or make changes to this bug.