Bug 145192

Summary: Object allocation sinking phase should explicitly create bottom values for CreateActivation sink candidates and CreateActivation should have SymbolTable as a child node
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: basile_clement, commit-queue, fpizlo, ggaren, mark.lam, mmirman, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 142944    
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch none

Description Saam Barati 2015-05-19 16:16:10 PDT
This is in preparation for ES6 where the default values for CreateActivation variable slots might be undefined or might be the empty TDZ value.

Also, this is preparing CreateActivation to be slightly more general by taking in a SymbolTable as a child node
because with ES6 block scoping, op_create_lexical_environment won't be the only op code lowered to CreateActivation.
Comment 1 Saam Barati 2015-05-19 19:36:46 PDT
Created attachment 253421 [details]
patch
Comment 2 Filip Pizlo 2015-05-19 22:16:28 PDT
Comment on attachment 253421 [details]
patch

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

I think you did this right, but are we really sure that we want to introduce an edge to the graph that has a magical requirement that one of the nodes is a constant?

> Source/JavaScriptCore/ChangeLog:16
> +       This patch also adds the the constant SymbolTable as child2 of CreateActivation 
> +       nodes. This is in preparation for ES6 block scoping which will introduce a new 
> +       op code that gets lowered to CreateActivation.

I have to say that I'm disturbed by CreateActivation taking a mandatory-JSConstant child.  This feels like an antipattern, because we use Edge and Node* to mean "something that might have to be computed at run-time".  That's not the case here.  You are requiring the SymbolTable to be a constant, because otherwise you wouldn't know how to allocate the activation.

Is it *really* necessary to do it this way?  Can we have CreateActivation use OpInfo for the SymbolTable*?  This would mean that object allocation sinking has to materialize a JSConstant for the SymbolTable* when creating PutHints, and then: MaterializeCreateActivation will also have a SymbolTable* OpInfo, and OSR-exit can use something like an ActivationSymbolTablePLoc "field" that it deduces from the PutHints.  The point of doing it this way is that so long as we have a CreateActivation - that is, an operation that requires optimized code to allocate and then access an activation object - we make it easy to access the SymbolTable* and also o be confident that this is legal to do so and that the symbol table won't be something that is evaluated at runtime.

> Source/JavaScriptCore/dfg/DFGNode.h:2005
> +
> +    SymbolTable* symbolTableForActivation()
> +    {
> +        ASSERT(op() == CreateActivation);
> +        return child2()->castConstant<SymbolTable*>();
> +    }

I would get rid of this helper.  We don't ordinarily give Node methods that dereference another node and do things to it.
Comment 3 Saam Barati 2015-05-19 22:49:46 PDT
> I have to say that I'm disturbed by CreateActivation taking a
> mandatory-JSConstant child.  This feels like an antipattern, because we use
> Edge and Node* to mean "something that might have to be computed at
> run-time".  That's not the case here.  You are requiring the SymbolTable to
> be a constant, because otherwise you wouldn't know how to allocate the
> activation.
> 
> Is it *really* necessary to do it this way?  Can we have CreateActivation
> use OpInfo for the SymbolTable*?  This would mean that object allocation
> sinking has to materialize a JSConstant for the SymbolTable* when creating
> PutHints, and then: MaterializeCreateActivation will also have a
> SymbolTable* OpInfo, and OSR-exit can use something like an
> ActivationSymbolTablePLoc "field" that it deduces from the PutHints.  The
> point of doing it this way is that so long as we have a CreateActivation -
> that is, an operation that requires optimized code to allocate and then
> access an activation object - we make it easy to access the SymbolTable* and
> also o be confident that this is legal to do so and that the symbol table
> won't be something that is evaluated at runtime.
> 
I agree with your argument. I think it's a bit silly to mandate that a child 
must be a constant. I will go with the approach of storing the SymbolTable in OpInfo
and materializing it during allocation sinking.

> > Source/JavaScriptCore/dfg/DFGNode.h:2005
> > +
> > +    SymbolTable* symbolTableForActivation()
> > +    {
> > +        ASSERT(op() == CreateActivation);
> > +        return child2()->castConstant<SymbolTable*>();
> > +    }
> 
> I would get rid of this helper.  We don't ordinarily give Node methods that
> dereference another node and do things to it.
I will keep this after switching to the OpInfo approach. This seems more in line with how
Node handles these situations.
Comment 4 Saam Barati 2015-05-19 23:34:30 PDT
> > > Source/JavaScriptCore/dfg/DFGNode.h:2005
> > > +
> > > +    SymbolTable* symbolTableForActivation()
> > > +    {
> > > +        ASSERT(op() == CreateActivation);
> > > +        return child2()->castConstant<SymbolTable*>();
> > > +    }
> > 
> > I would get rid of this helper.  We don't ordinarily give Node methods that
> > dereference another node and do things to it.
> I will keep this after switching to the OpInfo approach. This seems more in
> line with how
> Node handles these situations.
Scratch that, there is already stuff in place to make this convenient for cell operand.
Comment 5 Saam Barati 2015-05-20 15:24:40 PDT
Created attachment 253466 [details]
patch
Comment 6 Filip Pizlo 2015-05-20 16:57:39 PDT
Comment on attachment 253466 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGNode.h:1308
> +    FrozenValue* cellOperand2()

The way I would do this is make cellOperand use opInfo or opInfo2 conditionally on the op(). That way, you'll just have the one method.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5301
> +#ifndef NDEBUG

I wouldn't make this a compile-time thing. We have things like validationEnabled() and you could check that at runtime.

> Source/JavaScriptCore/ftl/FTLOperations.cpp:135
> +        RELEASE_ASSERT(materialization->properties().size() >= table->scopeSize());

Should this be ==?

> Source/JavaScriptCore/ftl/FTLOperations.cpp:144
> +#ifndef NDEBUG

You could just use "if (!ASSERT_DISABLED)" instead of #if.
Comment 7 Saam Barati 2015-05-21 00:16:18 PDT
Comment on attachment 253466 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGNode.h:1308
>> +    FrozenValue* cellOperand2()
> 
> The way I would do this is make cellOperand use opInfo or opInfo2 conditionally on the op(). That way, you'll just have the one method.

Will do.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5301
>> +#ifndef NDEBUG
> 
> I wouldn't make this a compile-time thing. We have things like validationEnabled() and you could check that at runtime.

I'll make this and the other loop execute under validationEnabled()

>> Source/JavaScriptCore/ftl/FTLOperations.cpp:135
>> +        RELEASE_ASSERT(materialization->properties().size() >= table->scopeSize());
> 
> Should this be ==?

No, because the materialization will have the ActivationScopePLoc and ActivationSymbolTablePLoc.
Maybe a nice addition to the validation would be to count the number of ClosureVarPLoc and ensure it's the same as the scopeSize().

Will we prune the properties that we place on the materialization after the sinking phase such that "store" followed by "store" to the
same scope offset will condense into a single "store"? If so, I believe they should always be equal (and also equal in compileMaterializeActivation validation too).
Comment 8 Saam Barati 2015-05-21 00:22:10 PDT
> Will we prune the properties that we place on the materialization after the
> sinking phase such that "store" followed by "store" to the
> same scope offset will condense into a single "store"? If so, I believe they
> should always be equal (and also equal in compileMaterializeActivation
> validation too).
What I mean for "store" here is the original PutHint for the default value.
Comment 9 Saam Barati 2015-05-21 18:12:24 PDT
Created attachment 253566 [details]
patch

Patch with Fil's suggestions.
Comment 10 WebKit Commit Bot 2015-05-21 18:15:23 PDT
Attachment 253566 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGNode.h:1297:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Saam Barati 2015-05-21 18:18:55 PDT
Created attachment 253568 [details]
patch

fix style.
Comment 12 Filip Pizlo 2015-05-21 18:51:27 PDT
Comment on attachment 253568 [details]
patch

Actually, I want to give Basile a chance to comment.
Comment 13 Basile Clement 2015-05-21 19:07:04 PDT
This looks good to me. 

(In reply to comment #12)
> Comment on attachment 253568 [details]
> patch
> 
> Actually, I want to give Basile a chance to comment.
Comment 14 WebKit Commit Bot 2015-05-21 19:39:36 PDT
Comment on attachment 253568 [details]
patch

Clearing flags on attachment: 253568

Committed r184747: <http://trac.webkit.org/changeset/184747>
Comment 15 WebKit Commit Bot 2015-05-21 19:39:42 PDT
All reviewed patches have been landed.  Closing bug.