[Webkit-unassigned] [Bug 84553] DFG should have control flow graph simplification

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 27 10:50:26 PDT 2012


https://bugs.webkit.org/show_bug.cgi?id=84553


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #139140|review?                     |review+
               Flag|                            |




--- Comment #15 from Oliver Hunt <oliver at apple.com>  2012-04-27 10:50:26 PST ---
(From update of attachment 139140)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list