Bug 96650 – Web Inspector: move Vector memory instrumentation from core NMI code to MemoryInstrumentationVector.h

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

[Yury Semikhatsky]

Vector may have inline buffer that should be counted as part of the vector, this makes it hard to instrument from outside.

[Yury Semikhatsky]

Created attachment 163871 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 163871 [details]
Patch

lgtm

[WebKit Review Bot]

Comment on attachment 163871 [details]
Patch

Rejecting attachment 163871 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
.cpp
Hunk #1 FAILED at 360.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/network/FormData.cpp.rej
patching file Source/WebCore/rendering/style/RenderStyle.cpp
patching file Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp
patching file Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Vsevolod V..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13851408

[Yury Semikhatsky]

Created attachment 164066 [details]
Patch after rebase

[Yury Semikhatsky]

Antti, can you take a look at Source/WTF/wtf/Vector.h changes and tell us if you are ok with the approach?

[Yury Semikhatsky]

Created attachment 164493 [details]
Patch after rebase

[Antti Koivisto]

Comment on attachment 164493 [details]
Patch after rebase

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

Looks fine to me but it would be good if Maciej or Darin (who know the WTF design principles best) would take a look too.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:389
> +TEST(MemoryInstrumentationTest, vectorWithInlineCapacity1)
> +{
> +    VisitedObjects visitedObjects;
> +    MemoryInstrumentationImpl impl(visitedObjects);
> +    InstrumentedOwner<Vector<int, 4> > vectorOwner;
> +    impl.addRootObject(vectorOwner);
> +    EXPECT_EQ(static_cast<size_t>(0), impl.reportedSizeForAllTypes());
> +    EXPECT_EQ(0, visitedObjects.size());
> +}

You should also include a test with a heap allocated Vector that shows inline capacity and other fields are properly accounted for.

[Yury Semikhatsky]

Created attachment 164518 [details]
Patch with tests for heap allocated vector

[Yury Semikhatsky]

(In reply to comment #7)
> (From update of attachment 164493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164493&action=review
> 
> Looks fine to me but it would be good if Maciej or Darin (who know the WTF design principles best) would take a look too.
> 
> > Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:389
> > +TEST(MemoryInstrumentationTest, vectorWithInlineCapacity1)
> > +{
> > +    VisitedObjects visitedObjects;
> > +    MemoryInstrumentationImpl impl(visitedObjects);
> > +    InstrumentedOwner<Vector<int, 4> > vectorOwner;
> > +    impl.addRootObject(vectorOwner);
> > +    EXPECT_EQ(static_cast<size_t>(0), impl.reportedSizeForAllTypes());
> > +    EXPECT_EQ(0, visitedObjects.size());
> > +}
> 
> You should also include a test with a heap allocated Vector that shows inline capacity and other fields are properly accounted for.

Done.

[Yury Semikhatsky]

A bit of context. In order to collect memory usage data for WebKit we need to have a good coverage of the memory occupied by WTF containers and smart pointers. The most straitforward way would be to add reportMemoryUsage method to the corresponding WTF classes. But since all of them are templates it would require including MemoryInstrumentation.h into the base headers. We see the following options:

 1) Include MemoryInstrumentation.h into the corresponding WTF headers.
 2) Make reportMemoryUsage method a template as in the current patch.
 3) Declare reportMemoryUsage in Vector.h but put its implementation along with #include <wtf/MemoryInstrumentation.h> into a separate header(VectorMemoryInstrumentation.h).
 4) Introduce some helper friend classes that would implement the instrumentation. That would also require adding a new file where the instrumentation would live.

[Maciej Stachowiak]

Comment on attachment 164518 [details]
Patch with tests for heap allocated vector

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

> Source/WTF/wtf/Vector.h:390
> +        template<typename MemoryObjectInfo>
> +        void reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
> +        {
> +            typename MemoryObjectInfo::ClassInfo info(memoryObjectInfo, this);
> +            info.addRawBuffer(m_buffer, m_capacity * sizeof(T));
> +        }

I think this can be done without adding member functions to Vector. A template free function outside Vector has access to all this info. I think it would be better to not change a low-level class like Vector for this high-level concept.

[Ilya Tikhonovsky]

Created attachment 164743 [details]
patch with external instrumentation

[Build Bot]

Comment on attachment 164743 [details]
patch with external instrumentation

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

[Ilya Tikhonovsky]

Created attachment 164747 [details]
with fix for mac

[Ilya Tikhonovsky]

Created attachment 164749 [details]
with link time guard for Vector in MemoryInstrumentation.h

[Yury Semikhatsky]

Comment on attachment 164749 [details]
with link time guard for Vector in MemoryInstrumentation.h

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

> Source/WTF/wtf/MemoryInstrumentation.h:105
> +    template<typename T> static void selectInstrumentationMethod(const T* const& object, MemoryObjectInfo* memoryObjectInfo)

Why did this method move?

> Source/WTF/wtf/MemoryInstrumentationVector.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

I'd rather call it VectorMemoryInstrumentation.h

> Source/WTF/wtf/MemoryInstrumentationVector.h:39
> +template<typename T, size_t inline_capacity>

style: inline_capacity -> inlineCapacity

> Source/WTF/wtf/MemoryInstrumentationVector.h:48
> +template<typename T, size_t inline_capacity>

style: inline_capacity -> inlineCapacity

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:431
> +    typedef Vector<String> ValueType;

ValueType -> StringVector?

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:435
> +        value->append(String("string"));

No need to call String( explicitly.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:438
> +    EXPECT_EQ(sizeof(ValueType) + sizeof(String) * value->capacity() + sizeof(StringImpl) * value->size(), impl.reportedSizeForAllTypes());

What about the size of "string" characters, should they also be counted?

[Ilya Tikhonovsky]

Created attachment 164901 [details]
comments addressed

[Ilya Tikhonovsky]

(In reply to comment #16)
> (From update of attachment 164749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164749&action=review
> 
> > Source/WTF/wtf/MemoryInstrumentation.h:105
> > +    template<typename T> static void selectInstrumentationMethod(const T* const& object, MemoryObjectInfo* memoryObjectInfo)
> 
> Why did this method move?

done

> 
> > Source/WTF/wtf/MemoryInstrumentationVector.h:2
> > + * Copyright (C) 2012 Google Inc. All rights reserved.
> 
> I'd rather call it VectorMemoryInstrumentation.h

I'm not sure that it is a good idea. We will have many instrumentation headers
and I'd prefer to keep them together in the file system, in the build system files and in the list of includes.

> 
> > Source/WTF/wtf/MemoryInstrumentationVector.h:39
> > +template<typename T, size_t inline_capacity>
> 
> style: inline_capacity -> inlineCapacity
> 
> > Source/WTF/wtf/MemoryInstrumentationVector.h:48
> > +template<typename T, size_t inline_capacity>
> 
> style: inline_capacity -> inlineCapacity

done

> 
> > Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:431
> > +    typedef Vector<String> ValueType;
> 
> ValueType -> StringVector?

done


> 
> > Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:435
> > +        value->append(String("string"));
> 
> No need to call String( explicitly.

done

> 
> > Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:438
> > +    EXPECT_EQ(sizeof(ValueType) + sizeof(String) * value->capacity() + sizeof(StringImpl) * value->size(), impl.reportedSizeForAllTypes());
> 
> What about the size of "string" characters, should they also be counted?

nope. When we construct a String object from a constant string literal StringImpl doesn't copy it but keeps a pointer to it.

[Yury Semikhatsky]

Maciej, can you take another look?

[Ilya Tikhonovsky]

Created attachment 165098 [details]
rebaselined. Old addVector and addVectorPtr also were removed.

[Gyuyoung Kim]

Comment on attachment 165098 [details]
rebaselined. Old addVector and addVectorPtr also were removed.

Attachment 165098 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13981042

[Build Bot]

Comment on attachment 165098 [details]
rebaselined. Old addVector and addVectorPtr also were removed.

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

[Ilya Tikhonovsky]

Created attachment 165133 [details]
with fixes for mac and elf.

[Build Bot]

Comment on attachment 165133 [details]
with fixes for mac and elf.

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

[Ilya Tikhonovsky]

Created attachment 165140 [details]
with fixes for mac and elf.

[Maciej Stachowiak]

It looks like the latest version addresses my previous comments. I'm fine with someone else reviewing the new version for correctness in detail.

[Vsevolod Vlasov]

Comment on attachment 165140 [details]
with fixes for mac and elf.

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

> Source/WTF/wtf/MemoryInstrumentationVector.h:48
> +template<size_t inlineCapacity> void instrumentVectorValues(MemoryClassInfo&, const Vector<char*, inlineCapacity>* const&) { }

Is this char* special case handler always correct?

[WebKit Review Bot]

Comment on attachment 165140 [details]
with fixes for mac and elf.

Rejecting attachment 165140 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ching file Source/WebCore/platform/network/FormData.cpp
patching file Source/WebCore/platform/network/ResourceRequestBase.cpp
patching file Source/WebCore/rendering/style/RenderStyle.cpp
patching file Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp
patching file Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Vsevolod V..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13988938

[Ilya Tikhonovsky]

Committed r129466: <http://trac.webkit.org/changeset/129466>