Bug 66849 – The GC does not have a facility for profiling the kinds of objects that occupy the heap

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

A good GC will have optimizations for common objects.  Two areas where optimizations are particularly useful are marking and sweeping.  But the GC does not currently have a way of profiling what kinds of objects are encountered during either of these two phases.

[Filip Pizlo]

Created attachment 104980 [details]
the patch

I'm pretty sure I have not yet taken care of the VC++/GNU/qmake side of the build yet.  I've also not verified that my changes don't perturb performance if profiling is disabled - which is a real risk given some subtle changes.  But other than that, it's in good shape to review.

[Geoffrey Garen]

Comment on attachment 104980 [details]
the patch

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". :)

[Filip Pizlo]

Created attachment 105062 [details]
the patch (fix review, build, conflicts)

[WebKit Review Bot]

Attachment 105062 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/heap/VTableSpectrum.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 105063 [details]
the patch (fix style)

[Geoffrey Garen]

Comment on attachment 105063 [details]
the patch (fix style)

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

r=me

> Source/JavaScriptCore/heap/VTableSpectrum.cpp:109
> +            fprintf(output, "    %s:%s(%p): %lu\n", strippedFileName, info.dli_sname, item.vtable, (long unsigned)item.count);

I think you can remove this "(long unsigned)" cast now.

[Filip Pizlo]

Created attachment 105066 [details]
the patch

[Early Warning System Bot]

Comment on attachment 105066 [details]
the patch

Attachment 105066 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9502015

[WebKit Review Bot]

Comment on attachment 105066 [details]
the patch

Attachment 105066 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9498024

[Filip Pizlo]

Created attachment 105087 [details]
the patch

[Filip Pizlo]

Comment on attachment 105087 [details]
the patch

Going to wait with commit until I'm sure that all of the build bots are happy with the #include's in VTableSpectrum.cpp

[WebKit Review Bot]

Comment on attachment 105087 [details]
the patch

Clearing flags on attachment: 105087

Committed r93918: <http://trac.webkit.org/changeset/93918>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.