Vector may have inline buffer that should be counted as part of the vector, this makes it hard to instrument from outside.
Created attachment 163871 [details] Patch
Comment on attachment 163871 [details] Patch lgtm
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
Created attachment 164066 [details] Patch after rebase
Antti, can you take a look at Source/WTF/wtf/Vector.h changes and tell us if you are ok with the approach?
Created attachment 164493 [details] Patch after rebase
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.
Created attachment 164518 [details] Patch with tests for heap allocated vector
(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.
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.
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.
Created attachment 164743 [details] patch with external instrumentation
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
Created attachment 164747 [details] with fix for mac
Created attachment 164749 [details] with link time guard for Vector in MemoryInstrumentation.h
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?
Created attachment 164901 [details] comments addressed
(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.
Maciej, can you take another look?
Created attachment 165098 [details] rebaselined. Old addVector and addVectorPtr also were removed.
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
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
Created attachment 165133 [details] with fixes for mac and elf.
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
Created attachment 165140 [details] with fixes for mac and elf.
It looks like the latest version addresses my previous comments. I'm fine with someone else reviewing the new version for correctness in detail.
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?
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
Committed r129466: <http://trac.webkit.org/changeset/129466>