Bug 66849 - The GC does not have a facility for profiling the kinds of objects that occupy the heap
Summary: The GC does not have a facility for profiling the kinds of objects that occup...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-24 04:20 PDT by Filip Pizlo
Modified: 2011-08-26 15:24 PDT (History)
4 users (show)

See Also:


Attachments
the patch (16.95 KB, patch)
2011-08-24 04:26 PDT, Filip Pizlo
ggaren: review-
Details | Formatted Diff | Diff
the patch (fix review, build, conflicts) (19.98 KB, patch)
2011-08-24 13:48 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix style) (19.98 KB, patch)
2011-08-24 13:54 PDT, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff
the patch (19.97 KB, patch)
2011-08-24 14:25 PDT, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
the patch (19.97 KB, patch)
2011-08-24 15:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-08-24 04:20:29 PDT
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.
Comment 1 Filip Pizlo 2011-08-24 04:26:22 PDT
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.
Comment 2 Geoffrey Garen 2011-08-24 12:15:04 PDT
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". :)
Comment 3 Filip Pizlo 2011-08-24 13:48:11 PDT
Created attachment 105062 [details]
the patch (fix review, build, conflicts)
Comment 4 WebKit Review Bot 2011-08-24 13:51:00 PDT
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.
Comment 5 Filip Pizlo 2011-08-24 13:54:58 PDT
Created attachment 105063 [details]
the patch (fix style)
Comment 6 Geoffrey Garen 2011-08-24 14:15:52 PDT
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.
Comment 7 Filip Pizlo 2011-08-24 14:25:27 PDT
Created attachment 105066 [details]
the patch
Comment 8 Early Warning System Bot 2011-08-24 14:54:21 PDT
Comment on attachment 105066 [details]
the patch

Attachment 105066 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9502015
Comment 9 WebKit Review Bot 2011-08-24 15:32:54 PDT
Comment on attachment 105066 [details]
the patch

Attachment 105066 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9498024
Comment 10 Filip Pizlo 2011-08-24 15:49:37 PDT
Created attachment 105087 [details]
the patch
Comment 11 Filip Pizlo 2011-08-24 15:51:44 PDT
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
Comment 12 WebKit Review Bot 2011-08-26 15:23:57 PDT
Comment on attachment 105087 [details]
the patch

Clearing flags on attachment: 105087

Committed r93918: <http://trac.webkit.org/changeset/93918>
Comment 13 WebKit Review Bot 2011-08-26 15:24:03 PDT
All reviewed patches have been landed.  Closing bug.