We need to be sure that the memory instrumentation only counts size of objects allocated in the heap. To test that we could request a set of all allocated objects from the memory allocator and check if the counted objects set is a subset of the set of all heap allocated objects.
Created attachment 165743 [details] Possible implementation draft
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.
Attachment 165743 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/MemoryInstrumentation.h', u..." exit_code: 1 Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:80: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/inspector/InspectorMemoryAgent.cpp:43: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:109: The parameter name "set" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/src/InspectorClientImpl.h:81: The parameter name "set" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 165743 [details] Possible implementation draft Attachment 165743 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14027415
Comment on attachment 165743 [details] Possible implementation draft Attachment 165743 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14021406
Comment on attachment 165743 [details] Possible implementation draft Attachment 165743 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14018429
Comment on attachment 165743 [details] Possible implementation draft Attachment 165743 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14004033
Created attachment 165753 [details] Patch
Comment on attachment 165753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165753&action=review > Source/WTF/wtf/MemoryInstrumentation.h:110 > +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION) I would only guard the backing implementation for better readability. > Source/WebCore/inspector/InspectorClient.h:75 > +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION) I would even remove this guard. > Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:86 > + if (!m_allocatedObjects.contains(object)) { You could only consider non-empty arrays valid.
Created attachment 165763 [details] Patch
(In reply to comment #9) > (From update of attachment 165753 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165753&action=review > > > Source/WTF/wtf/MemoryInstrumentation.h:110 > > +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION) > > I would only guard the backing implementation for better readability. > Done. > > Source/WebCore/inspector/InspectorClient.h:75 > > +#if ENABLE(DEBUG_MEMORY_INSTRUMENTATION) > > I would even remove this guard. > Done. > > Source/WebCore/inspector/MemoryInstrumentationImpl.cpp:86 > > + if (!m_allocatedObjects.contains(object)) { > > You could only consider non-empty arrays valid. Done.
Comment on attachment 165763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165763&action=review > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:492 > + : m_totalObjectsCount(totalObjectsCount + 1) I would multiply it by 2 and add a comment that the first pass was only estimating the number of objects for this allocation. If the *2 size is not enough, you could double it again and re-run as a fall back.
(In reply to comment #12) > (From update of attachment 165763 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165763&action=review > > > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:492 > > + : m_totalObjectsCount(totalObjectsCount + 1) > > I would multiply it by 2 and add a comment that the first pass was only estimating the number of objects for this allocation. If the *2 size is not enough, you could double it again and re-run as a fall back. Done.
Created attachment 165765 [details] Patch
Comment on attachment 165765 [details] Patch Attachment 165765 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14035385
Comment on attachment 165765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165765&action=review > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:506 > + printf("Adding more than %lu pointers.", m_maxObjectsCount); Please remove this.
Committed r129623: <http://trac.webkit.org/changeset/129623>