[webkit-reviews] review denied: [Bug 66849] The GC does not have a facility for profiling the kinds of objects that occupy the heap : [Attachment 104980] the patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 66849: The GC does not have a facility for profiling the kinds of objects
that occupy the heap
https://bugs.webkit.org/show_bug.cgi?id=66849

Attachment 104980: the patch
https://bugs.webkit.org/attachment.cgi?id=104980&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104980&action=review


As you say, needs more build foo, plus perf testing. Review comments below.

> Source/JavaScriptCore/heap/VTableSpectrum.cpp:56
> +    HashMap<void*, uintptr_t>::iterator iter = m_map.find(vTablePointer);
> +    if (iter == m_map.end())
> +	   m_map.add(vTablePointer, 1);
> +    else
> +	   iter->second++;

Our idiom for this kind of operation is:

pair<HashMap<void*, uintptr_t>::iterator, bool> result =
m_map.add(vTablePointer, 1);
if (!result.second) // pre-existing item in the table
    ++result.first->second;

This avoids a double hash lookup in the case of the first entry in the table.
(This isn't performance-critical code, but it's good to use the right idioms
everywhere.)

> Source/JavaScriptCore/heap/VTableSpectrum.cpp:66
> +    uintptr_t count;

uintptr_t is only needed if you're going to cast between unsigned and a pointer
type. I think this should just be unsigned long, since that's how count is
used, and then you can remove the cast from printf.

> Source/JavaScriptCore/runtime/JSCell.h:218
> +	   visitor.noticeAnthracite(this);
>	   visitor.append(&m_structure);

Slightly better to put this operation inside SlotVisitor::visitChildren. That
way, when we optimize to skip calling visitChildren on some cells, this will
still work. Also, you can then remove the noticeAnthracite API altogether,
which is nice, since I looked up "anthracite" and got "a hard, compact variety
of mineral coal that has a high luster". :)