Bug 155437 – Add support for setting Function.name from computed properties.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch and details coming.

[Radar WebKit Bug Importer]

<rdar://problem/25142388>

[Mark Lam]

Created attachment 274157 [details]
proposed patch.

[Mark Lam]

Created attachment 274159 [details]
x86_64 benchmark results.

Perf is neutral with this patch.  The definitely faster results cannot be real because this patch only adds more code to the generated code.

[Saam Barati]

Comment on attachment 274157 [details]
proposed patch.

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

The patch looks good to me, but it might be worth considering hooking SetFunctionName into allocation sinking.
Are there programs now that would have benefited from allocation sinking and now this patch prevents that?

> Source/JavaScriptCore/runtime/JSFunction.cpp:581
> +        auto uid = asSymbol(value)->privateName().uid();

I'm not a big fan of auto here.

[Filip Pizlo]

Comment on attachment 274157 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:9
> +        When we have an assignment to a computed property, we will now inject an
> +        op_set_function_name bytecode if:

It's extremely confusing that "computed property" means "function name" in this patch.  I guess function.name is a kind of computed property.  But this patch makes it seem like they are the same thing.

> Source/JavaScriptCore/ChangeLog:17
> +        The op_set_function_name will result in JSFunction::setFunctionName() being
> +        called on the target function / class before it is assigned to the property.

OK, but why can't that just be a put_by_id trap?

> Source/JavaScriptCore/ChangeLog:22
> +        The choice to not emit the op_set_function_name bytecode all the time is only an
> +        optimization to prevent having to call JSFunction::setFunctionName() when we
> +        already know that it will do nothing (since the target function / class is
> +        proven to already have a name or name property).

Why is this optimization only in the bytecode generator?  It seems like if we want this optimization then it should be in the DFG.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1444
> +            fixEdge<KnownCellUse>(node->child1());

Is this right?  How do you know this will be a cell?

[Mark Lam]

Comment on attachment 274157 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        op_set_function_name bytecode if:
> 
> It's extremely confusing that "computed property" means "function name" in this patch.  I guess function.name is a kind of computed property.  But this patch makes it seem like they are the same thing.

Nope, not the function.name case.  Computed property here means a case like this:

    var x = getSomeValueOfX();
    var o = { [x]: function() {} }
 
... and x is the computed property in this example.  The ES6 spec states that o[x].name must be the stringified value of x at the time o[x] is set to that function (for some definition of stringify that is different than toString()).

>> Source/JavaScriptCore/ChangeLog:17
>> +        called on the target function / class before it is assigned to the property.
> 
> OK, but why can't that just be a put_by_id trap?

Because:
1. It'll be put_by_val (not put_by_id) if we doit  that way.

2. This will make put_by_val have to do above checks of (1) see if the value being put is a function or class, and (2) is anonymous, and (3) if is class, does not have a "name" property.  I think this is unnecessarily penalize all put_by_val for the sake of just a less common case (see (3) below).

3. Consider these case:

     var o = { [x]: function() {} } // this function's name should be set to the "stringified" value of x.
     o[y] = function() {}; // this function's name should NOT be set to the "stringified" value of y.

    Both cases above generates put_by_val, but only the literal object initialization case should result in the function name being set.

4. calling setFunctionName() is a suggestion and not an absolute action.  Consider this case:

        var o = { [x]: class { static [y]() { } }
        var c = o[x]; // c is the class object in o.

   For the above, c.name should be the value of x if and only if the value of y is not "name".

>> Source/JavaScriptCore/ChangeLog:22
>> +        proven to already have a name or name property).
> 
> Why is this optimization only in the bytecode generator?  It seems like if we want this optimization then it should be in the DFG.

Doing it in the bytecode generator benefits the llint and baseline JIT too, and saves the DFG from having to create the SetFunctionName node only to eliminate it (unless you mean doing it in the DFG bytecode parser?).

[Filip Pizlo]

(In reply to comment #6)
> Comment on attachment 274157 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274157&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:9
> >> +        op_set_function_name bytecode if:
> > 
> > It's extremely confusing that "computed property" means "function name" in this patch.  I guess function.name is a kind of computed property.  But this patch makes it seem like they are the same thing.
> 
> Nope, not the function.name case.  Computed property here means a case like
> this:
> 
>     var x = getSomeValueOfX();
>     var o = { [x]: function() {} }
>  
> ... and x is the computed property in this example.  The ES6 spec states
> that o[x].name must be the stringified value of x at the time o[x] is set to
> that function (for some definition of stringify that is different than
> toString()).

Can you be clear in the ChangeLog that this is what we're referring to here?

I think that set_function_name is probably not the best name for this.

> 
> >> Source/JavaScriptCore/ChangeLog:17
> >> +        called on the target function / class before it is assigned to the property.
> > 
> > OK, but why can't that just be a put_by_id trap?
> 
> Because:
> 1. It'll be put_by_val (not put_by_id) if we doit  that way.

I don't think so.  If you have

o = {[x]: function(){}}

then the base of the put is the function, not o. And the key to the put is "name", not the stringified form of x.

> 
> 2. This will make put_by_val have to do above checks of (1) see if the value
> being put is a function or class, and (2) is anonymous, and (3) if is class,
> does not have a "name" property.  I think this is unnecessarily penalize all
> put_by_val for the sake of just a less common case (see (3) below).

That's just not how put_by_id/put_by_val work.  They already handle all sort of deep special cases without penalizing everything.  For example the fast path of put_by_id is fast even though a proxy might intercept the whole put().  Same thing applies here.  You can override put() in JSFunction or whatever to intercept this.

> 
> 3. Consider these case:
> 
>      var o = { [x]: function() {} } // this function's name should be set to
> the "stringified" value of x.
>      o[y] = function() {}; // this function's name should NOT be set to the
> "stringified" value of y.
> 
>     Both cases above generates put_by_val, but only the literal object
> initialization case should result in the function name being set.

Ummm ok, but surely you're not suggesting that the semantics of "set_function_name" are to do the store of the function into o in the first case (the literal).  There is a separate opcode for that.  You're just using set_function_name to set the name of the function.

I'm saying that instead of emitting a set_function_name to set the name of the function, you should emit a put_by_id.

> 
> 4. calling setFunctionName() is a suggestion and not an absolute action. 
> Consider this case:
> 
>         var o = { [x]: class { static [y]() { } }
>         var c = o[x]; // c is the class object in o.
> 
>    For the above, c.name should be the value of x if and only if the value
> of y is not "name".

As I said, put_by_id can handle this.

> 
> >> Source/JavaScriptCore/ChangeLog:22
> >> +        proven to already have a name or name property).
> > 
> > Why is this optimization only in the bytecode generator?  It seems like if we want this optimization then it should be in the DFG.
> 
> Doing it in the bytecode generator benefits the llint and baseline JIT too,
> and saves the DFG from having to create the SetFunctionName node only to
> eliminate it (unless you mean doing it in the DFG bytecode parser?).

No, I mean having this optimization inside the DFG optimization pipeline.

I think that bytecode-generation-time optimizations almost always end up being a bad idea.  We tend to get them wrong and they tend not to provide any benefit.

[Filip Pizlo]

I just realized that my suggestion to use put_by_id is wrong because setFunctionName() does logic that wouldn't be triggered if you just did "function.name = blah".

This patch adds a new bytecode operation for this purpose just so that we can "optimize" it, except that all implementations of set_function_name just call a C function.  This implies that instead of making this a new bytecode opcode, we should make it a native function.  It can be a per-global-object native function that is not visible directly to the user but the bytecode generator can emit calls to it.

If we do this then:

- No new bytecode opcodes.  This is a good thing.  We should try to limit the number of these and not add them too often.

- Same performance for LLInt and Baseline JIT.  They would have called a native function anyway.

- Same performnace for the DFG.  You can still recognize this function as an intrinsic.

[Filip Pizlo]

(In reply to comment #8)
> I just realized that my suggestion to use put_by_id is wrong because
> setFunctionName() does logic that wouldn't be triggered if you just did
> "function.name = blah".
> 
> This patch adds a new bytecode operation for this purpose just so that we
> can "optimize" it, except that all implementations of set_function_name just
> call a C function.  This implies that instead of making this a new bytecode
> opcode, we should make it a native function.  It can be a per-global-object
> native function that is not visible directly to the user but the bytecode
> generator can emit calls to it.
> 
> If we do this then:
> 
> - No new bytecode opcodes.  This is a good thing.  We should try to limit
> the number of these and not add them too often.
> 
> - Same performance for LLInt and Baseline JIT.  They would have called a
> native function anyway.
> 
> - Same performnace for the DFG.  You can still recognize this function as an
> intrinsic.

Meh.  I think this is a good future idea, not necessary right now.

[Filip Pizlo]

Comment on attachment 274157 [details]
proposed patch.

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

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1444
>> +            fixEdge<KnownCellUse>(node->child1());
> 
> Is this right?  How do you know this will be a cell?

Right it's a cell because this is only used for when we just allocated the function.  Add a comment?

[Mark Lam]

Created attachment 274199 [details]
patch for landing w/ a more detailed ChangeLog + some requested comments.

[Mark Lam]

Thanks for the review.  Landed in r198288: <http://trac.webkit.org/r198288>.