Bug 97198 – Web Inspector: extract HashSet instrumentation from core NMI code and put it into MemoryInstrumentationHashSet.h

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

[Ilya Tikhonovsky]

Current implementation has overloads for HashSet.
This prevents us from instrumenting complex cases like Vector<HashSet<...> >.

[Ilya Tikhonovsky]

Created attachment 165077 [details]
patch for try bots. HashSet + Vector.

[Yury Semikhatsky]

Comment on attachment 165077 [details]
patch for try bots. HashSet + Vector.

Clearing r?. Please remove changes to vector.

[Ilya Tikhonovsky]

Created attachment 165558 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 165558 [details]
Patch

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

> Source/WTF/wtf/MemoryInstrumentationHashSet.h:57
> +void reportMemoryUsage(const HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>* const& hashTable, MemoryObjectInfo* memoryObjectInfo)

Please put HashTable memory instrumentation into a separate file as it is going to be reused by HashMap and LinkedHashSet instrumentations.

[Yury Semikhatsky]

Comment on attachment 165558 [details]
Patch

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

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:446
> +    VisitedObjects visitedObjects;

Please add a test for HashTable instrumentation and a test for HashSet owned by value.

[Ilya Tikhonovsky]

Created attachment 165587 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #4)
> (From update of attachment 165558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165558&action=review
> 
> > Source/WTF/wtf/MemoryInstrumentationHashSet.h:57
> > +void reportMemoryUsage(const HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>* const& hashTable, MemoryObjectInfo* memoryObjectInfo)
> 
> Please put HashTable memory instrumentation into a separate file as it is going to be reused by HashMap and LinkedHashSet instrumentations.

done.

> > Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:446
> > +    VisitedObjects visitedObjects;
> 
> Please add a test for HashTable instrumentation and a test for HashSet owned by value.

HashTable is a wtf internal class which is used by HashSet, HashMap, etc and is never used directly in WebCore. Also it knows nothing about the internal structure of the values inside it. Thus it is not necessary to test its instrumentation directly.

[Yury Semikhatsky]

(In reply to comment #7)
> > Please add a test for HashTable instrumentation and a test for HashSet owned by value.
> 
> HashTable is a wtf internal class which is used by HashSet, HashMap, etc and is never used directly in WebCore. Also it knows nothing about the internal structure of the values inside it. Thus it is not necessary to test its instrumentation directly.

What about the second part of the commentary?

[Ilya Tikhonovsky]

(In reply to comment #8)
> (In reply to comment #7)
> > > Please add a test for HashTable instrumentation and a test for HashSet owned by value.
> > 
> > HashTable is a wtf internal class which is used by HashSet, HashMap, etc and is never used directly in WebCore. Also it knows nothing about the internal structure of the values inside it. Thus it is not necessary to test its instrumentation directly.
> 
> What about the second part of the commentary?

addMember function has its own mechanic for pointer vs reference instrumentation. And it is covered by ptrVsRef test.

[Yury Semikhatsky]

Comment on attachment 165587 [details]
Patch

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

> Source/WTF/wtf/MemoryInstrumentation.h:193
> +    template<typename I> void addMembers(I iterator, I end)

Can you change this to addCollectionElements(const T& collection) so that we don't need to .begin() and .end() in all call sites?

[Ilya Tikhonovsky]

Committed r129637: <http://trac.webkit.org/changeset/129637>

[Yury Semikhatsky]

Reopening to attach new patch.

[Yury Semikhatsky]

(In reply to comment #12)
> Reopening to attach new patch.

Sorry, wrong bug.