Bug 118866 – fourthTier: each DFG node that relies on other nodes to do their type checks should be able to tell you if those type checks happened

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

[Filip Pizlo]

Fun.

[Filip Pizlo]

Created attachment 207203 [details]
the patch

[Sam Weinig]

Comment on attachment 207203 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:199
> +    case InstanceOf: // This might be a bit of a lie. I'm not sure.

This is a disconcerting comment.  If you aren't sure, who will be?

[Filip Pizlo]

(In reply to comment #2)
> (From update of attachment 207203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207203&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSafeToExecute.h:199
> > +    case InstanceOf: // This might be a bit of a lie. I'm not sure.
> 
> This is a disconcerting comment.  If you aren't sure, who will be?

Actually, it's not a lie.  InstanceOf relies on CheckHasInstance to be correct, so we may have a weird situation - entirely theoretical - where we transform:

o = ...
loop {
   CheckHashIntance(..., o, ...)
   InstanceOf(..., o, ...)
}

into:

t = InstanceOf(..., o, ...)
loop {
   CheckHasInstance(..., o, ...)
   // use t
}

We cannot ever do that because if InstanceOf is hoistable, CheckHasInstance will be, also.  But even if we did do that, this would still be safe: InstanceOf might return a bogus result for those cases that CheckHasInstance would have caught, but then the CheckHasInstance will still run before anyone uses those results.

So, all good.

I'll remove the comment.

[Filip Pizlo]

Created attachment 207206 [details]
the patch

Patch without the wrong comment.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/152953