Bug 85945 - DFG variable capture analysis should work even if the variables arose through inlining
: DFG variable capture analysis should work even if the variables arose through...
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 87205
  Show dependency treegraph
 
Reported: 2012-05-08 18:51 PST by
Modified: 2012-05-23 00:29 PST (History)


Attachments
the patch (34.76 KB, patch)
2012-05-08 18:59 PST, Filip Pizlo
oliver: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-08 18:51:56 PST
Currently we cannot inline functions that create arguments or access arguments reflectively, principally because variable capture analysis is not inlining-aware.  Patch forthcoming.
------- Comment #1 From 2012-05-08 18:59:36 PST -------
Created an attachment (id=140849) [details]
the patch
------- Comment #2 From 2012-05-08 22:14:49 PST -------
(From update of attachment 140849 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140849&action=review

In general this patch looks fine (aside from the bool vs. enum issue i mention), but i'm somewhat tired so don't feel sufficiently focussed to r+ it.  Will re-review in the morning.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:114
> -    VariableAccessData* newVariableAccessData(int operand)
> +    VariableAccessData* newVariableAccessData(int operand, bool isCaptured)

We try to use enums rather than bools in cases like this as they're more descriptive, and you also escape the automatic int->bool conversion purgatory
------- Comment #3 From 2012-05-08 22:43:26 PST -------
(In reply to comment #2)
> (From update of attachment 140849 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140849&action=review
> 
> In general this patch looks fine (aside from the bool vs. enum issue i mention), but i'm somewhat tired so don't feel sufficiently focussed to r+ it.  Will re-review in the morning.
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:114
> > -    VariableAccessData* newVariableAccessData(int operand)
> > +    VariableAccessData* newVariableAccessData(int operand, bool isCaptured)
> 
> We try to use enums rather than bools in cases like this as they're more descriptive, and you also escape the automatic int->bool conversion purgatory

I agree that in general that is the right way to do it.

But here a bool is really a lot better.  Consider what would happen if this was an enum.  The most common idiom for this method is to say:

bool isCaptured = something->isCaptured();
... // bunch of code
if (isCaptured) { ... /* do special things */ ... }
... // a lot of other code
stuff = newVariableAccessData(thingy, isCaptured);

If newVariableAccessData() took an enum instead, then I'd either have to make isCaptured() return that enum, or I'd have to make this code absolutely horrible:

stuff = newVariableAccessData(thingy, isCaptured ? Captured : NotCaptured);

That is clearly a regression.  The alternative is to have isCaptured() and mergeIsCaptured() use the enum, but then I'd lose the clarify of using boolean arithmetic.  Notice that mergeIsCaptured() does things like:

bool newIsCaptured = m_isCaptured | isCaptured

Do you really want this to become:

CaptureMode newIsCaptured = ((m_isCaptured == Captured) | (isCaptured == Captured)) ? Captured : NotCaptured;

Finally, there is only *one* place where newVariableAccessData() is called in a manner that leads the second argument to be confusing (i.e. where we pass false as the second argument).  In all other places we pass a variable called "isCaptured" as the second argument.

So, I just don't buy that turning this into an enum is going to make anyone's life any easier.
------- Comment #4 From 2012-05-09 14:06:10 PST -------
Landed in Landed in http://trac.webkit.org/changeset/116555
------- Comment #5 From 2012-05-23 00:28:53 PST -------
Merged in http://trac.webkit.org/changeset/118136