Bug 145192 – Object allocation sinking phase should explicitly create bottom values for CreateActivation sink candidates and CreateActivation should have SymbolTable as a child node

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 253421 [details]
patch

[Filip Pizlo]

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.

[Saam Barati]

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

[Saam Barati]

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

[Saam Barati]

Created attachment 253466 [details]
patch

[Filip Pizlo]

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.

[Saam Barati]

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

[Saam Barati]

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

[Saam Barati]

Created attachment 253566 [details]
patch

Patch with Fil's suggestions.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 253568 [details]
patch

fix style.

[Filip Pizlo]

Comment on attachment 253568 [details]
patch

Actually, I want to give Basile a chance to comment.

[Basile Clement]

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.

[WebKit Commit Bot]

Comment on attachment 253568 [details]
patch

Clearing flags on attachment: 253568

Committed r184747: <http://trac.webkit.org/changeset/184747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.