[webkit-reviews] review granted: [Bug 84553] DFG should have control flow graph simplification : [Attachment 139140] the patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 84553: DFG should have control flow graph simplification
https://bugs.webkit.org/show_bug.cgi?id=84553

Attachment 139140: the patch
https://bugs.webkit.org/attachment.cgi?id=139140&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=139140&action=review


r=me, but make sure we don't have any custom toBoolean(CallFrame*) overrides in
WebCore -- I recall that there was one, something like document.all -- but it's
possible we hanlde that with the MasqueradesAsUndefined flag.

r+ with the few changes i suggested.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:153
> +	       root->valuesAtHead.local(i).makeTop();

Sadness.  Maybe one day we'll be able to do some analysis of capture sites.  Oh
well.

> Source/JavaScriptCore/dfg/DFGAdjacencyList.h:45
> +    #define AdjacencyListSize 3

could we use enum { AdjacencyListSize = 3 } rather than a #define here?  Aside
from anything else #define's are hard to see in the debugger. (i'd suggest
static const but i see we use it as an array length below. Silly C++ const expr
requirements.

> Source/JavaScriptCore/dfg/DFGEdge.h:83
> +    // use an Edge where you meant to use Edge::index(), you will instead
get Edge::isSet(). Gross!

There's a horrible trick we use in other parts of the code where you can do
this:

    typedef void* (DFGEdge::*UnspecifiedBoolType);
    operator UnspecifiedBoolType*() const { return isSet ?
reinterpret_cast<UnspecifiedBoolType*>(1) : 0; }

This gives you (essentially) operator bool() but without the horrible implied
int conversions.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:130
> -    if (mustGenerate) {
> -	   ASSERT(refCount);
> +    if (mustGenerate)
>	   --refCount;

Seems reasonable to keep this assertion.

> Source/JavaScriptCore/dfg/DFGNode.h:478
> +	       return 1;
> +	   case Branch:
> +	       return 2;

Mmmmmm tasty constants ;) (not saying that there's anything better though :-(

> Source/JavaScriptCore/heap/Heap.cpp:860
>      double lastGCEndTime = WTF::currentTime();
>      m_lastGCLength = lastGCEndTime - lastGCStartTime;
>      JAVASCRIPTCORE_GC_END();
> -
> +    
>      (*m_activityCallback)();
>  }
>  

revert Heap.cpp?