Bug 96650 - Web Inspector: move Vector memory instrumentation from core NMI code to MemoryInstrumentationVector.h
Summary: Web Inspector: move Vector memory instrumentation from core NMI code to Memor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks: 87262 97198
  Show dependency treegraph
 
Reported: 2012-09-13 07:36 PDT by Yury Semikhatsky
Modified: 2012-09-25 00:54 PDT (History)
26 users (show)

See Also:


Attachments
Patch (25.37 KB, patch)
2012-09-13 07:42 PDT, Yury Semikhatsky
vsevik: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch after rebase (24.11 KB, patch)
2012-09-14 00:35 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch after rebase (23.64 KB, patch)
2012-09-17 22:18 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch with tests for heap allocated vector (24.59 KB, patch)
2012-09-18 02:10 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch with external instrumentation (35.70 KB, patch)
2012-09-19 08:35 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
with fix for mac (35.69 KB, patch)
2012-09-19 09:17 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
with link time guard for Vector in MemoryInstrumentation.h (35.50 KB, patch)
2012-09-19 09:30 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
comments addressed (34.54 KB, patch)
2012-09-20 05:53 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
rebaselined. Old addVector and addVectorPtr also were removed. (49.15 KB, patch)
2012-09-21 02:52 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
with fixes for mac and elf. (49.31 KB, patch)
2012-09-21 08:04 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
with fixes for mac and elf. (49.49 KB, patch)
2012-09-21 08:54 PDT, Ilya Tikhonovsky
vsevik: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-09-13 07:36:59 PDT
Vector may have inline buffer that should be counted as part of the vector, this makes it hard to instrument from outside.
Comment 1 Yury Semikhatsky 2012-09-13 07:42:59 PDT
Created attachment 163871 [details]
Patch
Comment 2 Ilya Tikhonovsky 2012-09-13 08:09:16 PDT
Comment on attachment 163871 [details]
Patch

lgtm
Comment 3 WebKit Review Bot 2012-09-13 23:54:45 PDT
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
Comment 4 Yury Semikhatsky 2012-09-14 00:35:02 PDT
Created attachment 164066 [details]
Patch after rebase
Comment 5 Yury Semikhatsky 2012-09-14 00:37:00 PDT
Antti, can you take a look at Source/WTF/wtf/Vector.h changes and tell us if you are ok with the approach?
Comment 6 Yury Semikhatsky 2012-09-17 22:18:35 PDT
Created attachment 164493 [details]
Patch after rebase
Comment 7 Antti Koivisto 2012-09-18 01:42:28 PDT
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.
Comment 8 Yury Semikhatsky 2012-09-18 02:10:31 PDT
Created attachment 164518 [details]
Patch with tests for heap allocated vector
Comment 9 Yury Semikhatsky 2012-09-18 02:10:45 PDT
(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.
Comment 10 Yury Semikhatsky 2012-09-18 02:25:58 PDT
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 11 Maciej Stachowiak 2012-09-18 21:52:16 PDT
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.
Comment 12 Ilya Tikhonovsky 2012-09-19 08:35:03 PDT
Created attachment 164743 [details]
patch with external instrumentation
Comment 13 Build Bot 2012-09-19 09:09:08 PDT
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
Comment 14 Ilya Tikhonovsky 2012-09-19 09:17:31 PDT
Created attachment 164747 [details]
with fix for mac
Comment 15 Ilya Tikhonovsky 2012-09-19 09:30:00 PDT
Created attachment 164749 [details]
with link time guard for Vector in MemoryInstrumentation.h
Comment 16 Yury Semikhatsky 2012-09-20 05:37:38 PDT
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?
Comment 17 Ilya Tikhonovsky 2012-09-20 05:53:24 PDT
Created attachment 164901 [details]
comments addressed
Comment 18 Ilya Tikhonovsky 2012-09-20 06:02:12 PDT
(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.
Comment 19 Yury Semikhatsky 2012-09-20 07:44:19 PDT
Maciej, can you take another look?
Comment 20 Ilya Tikhonovsky 2012-09-21 02:52:37 PDT
Created attachment 165098 [details]
rebaselined. Old addVector and addVectorPtr also were removed.
Comment 21 Gyuyoung Kim 2012-09-21 03:19:19 PDT
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 22 Build Bot 2012-09-21 03:37:56 PDT
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
Comment 23 Ilya Tikhonovsky 2012-09-21 08:04:40 PDT
Created attachment 165133 [details]
with fixes for mac and elf.
Comment 24 Build Bot 2012-09-21 08:50:23 PDT
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
Comment 25 Ilya Tikhonovsky 2012-09-21 08:54:39 PDT
Created attachment 165140 [details]
with fixes for mac and elf.
Comment 26 Maciej Stachowiak 2012-09-24 21:32:03 PDT
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 27 Vsevolod Vlasov 2012-09-24 23:24:13 PDT
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 28 WebKit Review Bot 2012-09-24 23:27:27 PDT
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
Comment 29 Ilya Tikhonovsky 2012-09-25 00:54:37 PDT
Committed r129466: <http://trac.webkit.org/changeset/129466>