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

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from Filip Pizlo <fpizlo at apple.com>  2012-04-27 11:38:27 PST ---
(In reply to comment #15)
> (From update of attachment 139140 [details])
> 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.

It's not that hard.  But I don't want to pile on more dangerous stuff all at once.

> 
> > 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.

Ah, good point about using an enum.  I'll do that.

> 
> > 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.

I'll add that.  Though, right now, nobody will use it.  I've gotten used to having !!foo.child1() in places.

> 
> > Source/JavaScriptCore/dfg/DFGGraph.cpp:130
> > -    if (mustGenerate) {
> > -        ASSERT(refCount);
> > +    if (mustGenerate)
> >          --refCount;
> 
> Seems reasonable to keep this assertion.

It isn't.  If you have this assertion and the validation fails, then instead of getting a nice pretty graph dump showing you why validation failed, you have a good chance of getting an assertion failure in the graph dump.

The graph dump should not try to validate the graph.  It should try hard to print it even if it's bogus.

> 
> > Source/JavaScriptCore/dfg/DFGNode.h:478
> > +            return 1;
> > +        case Branch:
> > +            return 2;
> 
> Mmmmmm tasty constants ;) (not saying that there's anything better though :-(

They're infallible constants of nature and truth.  A jump has one successor because that's the definition of a jump.  A branch has two successors because that's the definition of a branch.

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

Ooops!  Yes, will do that.

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