[webkit-reviews] review granted: [Bug 170110] DFG and B3 SSA conversion should use a faster dominance frontier calculator : [Attachment 305442] the patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 170110: DFG and B3 SSA conversion should use a faster dominance frontier
calculator
https://bugs.webkit.org/show_bug.cgi?id=170110

Attachment 305442: the patch

https://bugs.webkit.org/attachment.cgi?id=305442&action=review




--- Comment #5 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 305442
  --> https://bugs.webkit.org/attachment.cgi?id=305442
the patch

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

r=me. Just style comments below.

> Source/JavaScriptCore/ChangeLog:9
> +	   precomputes all dominance frontiers to make them much faster to
query. This is a 2.5% overall

Awesome!

> Source/JavaScriptCore/b3/B3SSACalculator.h:113
> +	   m_dominanceFrontiers = &m_proc.dominanceFrontiers();

Style: Why does this need to be a member variable?

> Source/WTF/wtf/DominanceFrontiers.h:46
> +	   for (unsigned blockIndex = m_graph.numNodes(); blockIndex--;) {

Maybe it's worth a link to where this algorithm is described, something like:
http://www.hipersoft.rice.edu/grads/publications/dom14.pdf ?

> Source/WTF/wtf/DominanceFrontiers.h:51
> +	       m_iterationSet.clear();

Maybe it's also worth clearing this at after the last iteration of the loop?

> Source/WTF/wtf/DominanceFrontiers.h:58
> +		       [&] (typename Graph::Node dominatorBlock) {
> +			   if
(!m_iterationSet.add(m_graph.index(dominatorBlock)))
> +			       return;

If we have already seen this, we've already seen all its dominators too. Maybe
it's worth having forAllDominatorsOf take an IterationStatus so we can return
early here and cut off the entire iteration to prevent repeated hash set
accesses?

> Source/WTF/wtf/Vector.h:965
> +template<typename Func>
> +void Vector<T, inlineCapacity, OverflowHandler, minCapacity>::forEach(const
Func& func) const
> +{
> +    size_t size = this->size();
> +    for (size_t i = 0; i < size; ++i)
> +	   func(Base::buffer()[i]);
> +}
> +
> +template<typename T, size_t inlineCapacity, typename OverflowHandler, size_t
minCapacity>
> +template<typename Func>
> +void Vector<T, inlineCapacity, OverflowHandler, minCapacity>::forEach(const
Func& func)
> +{
> +    size_t size = this->size();
> +    for (size_t i = 0; i < size; ++i)
> +	   func(Base::buffer()[i]);
> +}

Style: Maybe this should be a free function?