Bug 169590 – Record the HashSet/HashMap operations in DFG/FTL/B3 and replay them in a benchmark

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

[Filip Pizlo]

As the title says.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 304418 [details]
the patch

[WebKit Commit Bot]

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.

[Saam Barati]

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

[Filip Pizlo]

(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

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/213939