Bug 169590 - Record the HashSet/HashMap operations in DFG/FTL/B3 and replay them in a benchmark
Summary: Record the HashSet/HashMap operations in DFG/FTL/B3 and replay them in a benc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-13 22:39 PDT by Filip Pizlo
Modified: 2017-03-14 14:38 PDT (History)
1 user (show)

See Also:


Attachments
it begins (14.44 KB, patch)
2017-03-13 22:42 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (413.20 KB, patch)
2017-03-14 13:44 PDT, Filip Pizlo
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-03-13 22:39:13 PDT
As the title says.
Comment 1 Filip Pizlo 2017-03-13 22:42:49 PDT
Created attachment 304356 [details]
it begins

This will be so much fun.  Once LoggingHashMap and LoggingHashSet are done, I'll drop them into a bunch of places in the DFG, FTL, and B3.

Note that I don't intend to check in the code where the compilers actually use the logging hashtables. But I guess it's worth checking in the logging hashtables themselves along with the benchmarks that the logging creates.
Comment 2 Filip Pizlo 2017-03-14 13:44:42 PDT
Created attachment 304418 [details]
the patch
Comment 3 WebKit Commit Bot 2017-03-14 13:47:19 PDT
Attachment 304418 [details] did not pass style-queue:


ERROR: Source/WTF/benchmarks/HashSetDFGReplay.cpp:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Saam Barati 2017-03-14 14:04:59 PDT
Comment on attachment 304418 [details]
the patch

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

r=me. Nice!

> Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.cpp:45
> +// static const char templateString[] = "unsigned, DefaultHash<unsigned>::Hash, WTF::UnsignedWithZeroKeyHashTraits<unsigned>";
> +// typedef LoggingHashSet<templateString, unsigned, DefaultHash<unsigned>::Hash, WTF::UnsignedWithZeroKeyHashTraits<unsigned>> LiveSet;

Maybe worth a comment here for what uncommenting this code does?
Or perhaps have it behind an ifdef?

> Source/WTF/wtf/HashSet.h:91
> +        void addVoid(const ValueType&);
> +        void addVoid(ValueType&&);

This name is a bit confusing to me. Now sure if there is a better way to describe it
Comment 5 Filip Pizlo 2017-03-14 14:33:32 PDT
(In reply to comment #4)
> Comment on attachment 304418 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304418&action=review
> 
> r=me. Nice!
> 
> > Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.cpp:45
> > +// static const char templateString[] = "unsigned, DefaultHash<unsigned>::Hash, WTF::UnsignedWithZeroKeyHashTraits<unsigned>";
> > +// typedef LoggingHashSet<templateString, unsigned, DefaultHash<unsigned>::Hash, WTF::UnsignedWithZeroKeyHashTraits<unsigned>> LiveSet;
> 
> Maybe worth a comment here for what uncommenting this code does?

I'll do that!

> Or perhaps have it behind an ifdef?

I don't want a global ifdef because I want to be able to cherry pick what to log. I don't want a local ifdef because then you still have to change code to enable it. If you're going to change code then comments are easier.

> 
> > Source/WTF/wtf/HashSet.h:91
> > +        void addVoid(const ValueType&);
> > +        void addVoid(ValueType&&);
> 
> This name is a bit confusing to me. Now sure if there is a better way to
> describe it
Comment 6 Filip Pizlo 2017-03-14 14:38:00 PDT
Landed in https://trac.webkit.org/changeset/213939