Bug 98005 – Web Inspector: provide memory instrumentation for HashMap

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

[Yury Semikhatsky]

Currently the HashMap support consists of MemoryInstrumentation::addHashMap/addInstrumentedMapEntries/addInstrumentedMapValues which should be called if we want to instrument HashMap internals. This approach is error prone. We should detect HashMap type automatically instead and collect data for the map content if it is instrumented.

[Yury Semikhatsky]

Created attachment 166433 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #1)
> Created an attachment (id=166433) [details]
> Patch

No XCode project changes in this patch. Will add them later.

[Build Bot]

Comment on attachment 166433 [details]
Patch

Attachment 166433 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14118030

[Ilya Tikhonovsky]

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

> Source/WTF/wtf/MemoryInstrumentation.h:159
>      template<typename HashMapType> void addHashMap(const HashMapType&, MemoryObjectType, bool contentOnly = false);

unnecessary line

> Source/WTF/wtf/MemoryInstrumentation.h:215
>      template<typename HashSetType> void addHashCountedSet(const HashSetType& set) { m_memoryInstrumentation->addHashMap(set, m_objectType, true); }

addMember?

> Source/WTF/wtf/MemoryInstrumentationHashMap.h:70
> +    // Check if type is convertible to integer to handle enum keys and values.
> +    SequenceMemoryInstrumentationTraits<typename Conditional<IsConvertibleToInteger<KeyArg>::value, int, KeyArg>::Type>::reportMemoryUsage(hashMap->begin().keys(), hashMap->end().keys(), info);
> +    SequenceMemoryInstrumentationTraits<typename Conditional<IsConvertibleToInteger<MappedArg>::value, int, MappedArg>::Type>::reportMemoryUsage(hashMap->begin().values(), hashMap->end().values(), info);
> +//    SequenceMemoryInstrumentationTraits<KeyArg>::reportMemoryUsage(hashMap->begin().keys(), hashMap->end().keys(), info);
> +//    SequenceMemoryInstrumentationTraits<MappedArg>::reportMemoryUsage(hashMap->begin().values(), hashMap->end().values(), info);

please remove commented code.

Is IsConvertibleToInteger works correctly for classes which have operator int()?

the code hashMap->begin().keys(), hashMap->end().keys() looks strange. I'd prefer hashMap->keys().begin();  etc.

> Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:534
> +TEST(MemoryInstrumentationTest, hashMapWithWnumKeysAndInstrumentedValues)

typo: WithEnum

[Yury Semikhatsky]

Created attachment 166456 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #4)
> View in context: https://bugs.webkit.org/attachment.cgi?id=166433&action=review
> 
> > Source/WTF/wtf/MemoryInstrumentation.h:159
> >      template<typename HashMapType> void addHashMap(const HashMapType&, MemoryObjectType, bool contentOnly = false);
> 
> unnecessary line
> 
> > Source/WTF/wtf/MemoryInstrumentation.h:215
> >      template<typename HashSetType> void addHashCountedSet(const HashSetType& set) { m_memoryInstrumentation->addHashMap(set, m_objectType, true); }
> 
> addMember?
> 
We need to get rid of addHashCountedSet method which I'm planning to do in a separate patch. I changed the name of addHashMap to addCountedSet to reflect that it is only used for HashCountedSet instrumentation.

> > Source/WTF/wtf/MemoryInstrumentationHashMap.h:70
> > +    // Check if type is convertible to integer to handle enum keys and values.
> > +    SequenceMemoryInstrumentationTraits<typename Conditional<IsConvertibleToInteger<KeyArg>::value, int, KeyArg>::Type>::reportMemoryUsage(hashMap->begin().keys(), hashMap->end().keys(), info);
> > +    SequenceMemoryInstrumentationTraits<typename Conditional<IsConvertibleToInteger<MappedArg>::value, int, MappedArg>::Type>::reportMemoryUsage(hashMap->begin().values(), hashMap->end().values(), info);
> > +//    SequenceMemoryInstrumentationTraits<KeyArg>::reportMemoryUsage(hashMap->begin().keys(), hashMap->end().keys(), info);
> > +//    SequenceMemoryInstrumentationTraits<MappedArg>::reportMemoryUsage(hashMap->begin().values(), hashMap->end().values(), info);
> 
> please remove commented code.
> 
Done.

> Is IsConvertibleToInteger works correctly for classes which have operator int()?
> 
No. It fails for such classes stored by value. Added a test for tracking this. I think we may live with that rare case.


> the code hashMap->begin().keys(), hashMap->end().keys() looks strange. I'd prefer hashMap->keys().begin();  etc.
> 
> > Source/WebKit/chromium/tests/MemoryInstrumentationTest.cpp:534
> > +TEST(MemoryInstrumentationTest, hashMapWithWnumKeysAndInstrumentedValues)
> 
> typo: WithEnum
Fixed.

[Build Bot]

Comment on attachment 166456 [details]
Patch

Attachment 166456 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14075908

[Ilya Tikhonovsky]

Comment on attachment 166456 [details]
Patch

lgtm

[Yury Semikhatsky]

(In reply to comment #7)
> (From update of attachment 166456 [details])
> Attachment 166456 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/14075908

The build error on Windows should disappear after http://trac.webkit.org/changeset/130047

[Yury Semikhatsky]

Committed r130048: <http://trac.webkit.org/changeset/130048>

[WebKit Review Bot]

Re-opened since this is blocked by bug 98041