Support computed properties for ES2015 Function.name

Support computed properties for ES2015 Function.name

Adds a new runtime function, %DefineDataPropertyInLiteral, which
takes a fifth argument specifying whether the property and value
are syntactically such that the value is a function (or class)
literal that should have its name set at runtime.

The new runtime call also allows us to eliminate the now-redundant
%DefineClassMethod runtime function.

This should get much less ugly once we can desugar the "dynamic"
part of object literals in the parser (but that work is currently
blocked on having a performant way of desugaring literals).

BUG=v8:3699, v8:3761
LOG=n

Committed: https://crrev.com/21c045a2fafd171272c53b17a5d495f5be100662
Cr-Commit-Position: refs/heads/master@{#33756}

[adamk, 2016-01-26 19:32:05]

Description was changed from

==========
WIP: Function.name for computed properties

BUG=v8:3699
LOG=n
==========

to

==========
Work in progress: Function.name for computed properties

BUG=v8:3699
LOG=n
==========

[adamk, 2016-01-26 19:34:34]

adamk@chromium.org changed reviewers:
+ verwaest@chromium.org

[adamk, 2016-01-26 19:34:34]

Hi Toon,

This is still a work-in-progress, but I had a couple of questions about the
proper implementation strategy I was hoping you could answer.

1. What version of DefineProperty should I be calling in JSFunction::SetName()?
The main requirement is that we need to replace the built-in "name" accessor on
the JSFunction with a data property. We also know that no one has a reference to
this JSFunction yet, if that happens to be useful information. What I have works
now, but that name looks scary.

2. What's the right contract for a function like JSFunction::SetName? Right now
I have it returning void and CHECKing that allocations succeed. Is that
acceptable? It seems weird to ever throw an exception here, but maybe that's the
right approach in case the exception is due to, say, worker shutdown?

[Toon Verwaest, 2016-01-27 13:40:37]

Since I landed https://codereview.chromium.org/1632603002/ you can just set the
property (with SetProperty), assuming that it's writable. That will keep the
attributes as they are. If you want custom attributes (assuming that it's
writable / configurable), you can also call the function you are currently
calling.

Doing ToHandleChecked is fine for code that shouldn't fail.

https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-classes...
src/runtime/runtime-classes.cc:222:
String::cast(function->shared()->name())->length() == 0)) {
In which case can it be a symbol but not length==0 on the SFI?

https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-object.cc
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-object....
src/runtime/runtime-object.cc:1016:
RUNTIME_FUNCTION(Runtime_DefineGetterPropertyUnchecked) {
I'm a bit confused why this is called Unchecked? Unlike
DefineDataPropertyUnchecked, it seems to perform all necessary checks ...

[adamk, 2016-01-27 17:04:51]

On 2016/01/27 13:40:37, Toon Verwaest wrote:
> Since I landed https://codereview.chromium.org/1632603002/ you can just set
the
> property (with SetProperty), assuming that it's writable. That will keep the
> attributes as they are. If you want custom attributes (assuming that it's
> writable / configurable), you can also call the function you are currently
> calling.

Not writable, unfortunately, so SetProperty won't work. There's also the new
DefineOwnProperty stuff, should I just use that?

> Doing ToHandleChecked is fine for code that shouldn't fail.
> 
>
https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-classes.cc
> File src/runtime/runtime-classes.cc (right):
> 
>
https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-classes...
> src/runtime/runtime-classes.cc:222:
> String::cast(function->shared()->name())->length() == 0)) {
> In which case can it be a symbol but not length==0 on the SFI?

As my TODO says, this really doesn't need to be a runtime check: the compiler
could know when generating code whether the name should be set here.

I'm actually considering adding a new function Map that doesn't include the
'name' getter/setter for such cases, which would make my first question moot. Is
there any drawback to doing so?

>
https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-object.cc
> File src/runtime/runtime-object.cc (right):
> 
>
https://codereview.chromium.org/1626423003/diff/1/src/runtime/runtime-object....
> src/runtime/runtime-object.cc:1016:
> RUNTIME_FUNCTION(Runtime_DefineGetterPropertyUnchecked) {
> I'm a bit confused why this is called Unchecked? Unlike
> DefineDataPropertyUnchecked, it seems to perform all necessary checks ...

Probably cargo-culted from the above?

[Toon Verwaest, 2016-01-27 17:26:33]

verwaest@chromium.org changed reviewers:
+ ishell@chromium.org

[Toon Verwaest, 2016-01-27 17:26:33]

I'm all in favor of adding a new map that has inobject space for the name field.
Igor did all the prework for Function subclassing anyway, so that should work.
You'll need multiple maps I presume for the multiple language modes.

+ishell

[adamk, 2016-01-27 20:17:45]

On 2016/01/27 17:26:33, Toon Verwaest wrote:
> I'm all in favor of adding a new map that has inobject space for the name
field.
> Igor did all the prework for Function subclassing anyway, so that should work.
> You'll need multiple maps I presume for the multiple language modes.
> 
> +ishell

Thinking about this a bit more, such an optimization might be more appropriate
as a follow-up to getting this spec-correct.

In the meantime, is JSReceiver::DefineOwnProperty() designed to be called
internally? Or am I better off leaving this with the really long
...IgnoreAttributs version?

[Toon Verwaest, 2016-01-29 12:18:49]

lgtm; the current approach is ok for a first version.

The right approach is indeed directly using the correct map (with space in the
object) and only setting up the field when needed from generated code rather
than dynamically checking in the runtime.

[adamk, 2016-01-29 23:43:18]

Will upload a new version of this after

[adamk, 2016-01-29 23:43:27]

On 2016/01/29 23:43:18, adamk wrote:
> Will upload a new version of this after

er...after https://codereview.chromium.org/1647213002/ lands

[adamk, 2016-02-02 23:55:42]

Description was changed from

==========
Work in progress: Function.name for computed properties

BUG=v8:3699
LOG=n
==========

to

==========
Support computed properties for ES2015 Function.name

Adds a new runtime function, %DefineDataPropertyInLiteral, which
takes a fifth argument specifying whether the property and value
are syntactically such that the value is a function (or class)
literal that should have its name set at runtime.

The new runtime call also allows us to eliminate the now-redundant
%DefineClassMethod runtime function.

This should get much less ugly once we can desugar the "dynamic"
part of object literals in the parser (but that work is currently
blocked on having a performant way of desugaring literals).

BUG=v8:3699, v8:3761
LOG=n
==========

[adamk, 2016-02-02 23:57:30]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org, rossberg@chromium.org
- ishell@chromium.org

[adamk, 2016-02-02 23:57:30]

https://codereview.chromium.org/1626423003/diff/20001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1626423003/diff/20001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:1652: __ mov(r0,
Operand(Smi::FromInt(NONE)));
Let me know if you'd like me to change these to the shorter "Push(Smi*)"
version; I left them as-is for now to reduce the diff size.

[adamk, 2016-02-02 23:57:54]

This is now ready for review, PTAL

[Benedikt Meurer, 2016-02-03 05:14:51]

LGTM on compiler parts, modulo comment about frame state.

https://codereview.chromium.org/1626423003/diff/20001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1626423003/diff/20001/src/compiler/linkage.cc...
src/compiler/linkage.cc:151: case Runtime::kDefineGetterPropertyUnchecked:  //
TODO(jarin): Is it safe?
Maybe include kDefineDataPropertyInLiteral here instead of creating a frame
state with BailoutId::None in the graph builder?

[Toon Verwaest, 2016-02-03 10:47:23]

still lgtm

https://codereview.chromium.org/1626423003/diff/20001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1626423003/diff/20001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:1652: __ mov(r0,
Operand(Smi::FromInt(NONE)));
On 2016/02/02 23:57:30, adamk wrote:
> Let me know if you'd like me to change these to the shorter "Push(Smi*)"
> version; I left them as-is for now to reduce the diff size.

The Push cleanup sounds nice. As you prefer.

[adamk, 2016-02-03 19:04:59]

https://codereview.chromium.org/1626423003/diff/20001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1626423003/diff/20001/src/compiler/linkage.cc...
src/compiler/linkage.cc:151: case Runtime::kDefineGetterPropertyUnchecked:  //
TODO(jarin): Is it safe?
On 2016/02/03 05:14:51, Benedikt Meurer wrote:
> Maybe include kDefineDataPropertyInLiteral here instead of creating a frame
> state with BailoutId::None in the graph builder?

Done.

https://codereview.chromium.org/1626423003/diff/20001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1626423003/diff/20001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:1652: __ mov(r0,
Operand(Smi::FromInt(NONE)));
On 2016/02/03 10:47:23, Toon Verwaest wrote:
> On 2016/02/02 23:57:30, adamk wrote:
> > Let me know if you'd like me to change these to the shorter "Push(Smi*)"
> > version; I left them as-is for now to reduce the diff size.
> 
> The Push cleanup sounds nice. As you prefer.

Added the cleanup (within this switch statement, anyway).

[adamk, 2016-02-04 19:17:44]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-02-04 19:17:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1626423003/#ps40001
(title: "Add FrameStateInputCount, use Push everywhere")

[commit-bot: I haz the power, 2016-02-04 19:17:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1626423003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1626423003/40001

[commit-bot: I haz the power, 2016-02-04 19:19:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-04 19:19:07]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/965)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/959)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/10558)

[adamk, 2016-02-04 21:48:53]

+rmcilroy, FYI: I disabled the new class test in test-bytecode-generator; please
fix and re-enable at your earliest convenience.

[adamk, 2016-02-04 21:48:58]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-02-04 21:48:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1626423003/#ps60001
(title: "Merge to trunk, disable bytecode generator class test")

[commit-bot: I haz the power, 2016-02-04 21:49:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1626423003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1626423003/60001

[commit-bot: I haz the power, 2016-02-04 22:36:21]

Description was changed from

==========
Support computed properties for ES2015 Function.name

Adds a new runtime function, %DefineDataPropertyInLiteral, which
takes a fifth argument specifying whether the property and value
are syntactically such that the value is a function (or class)
literal that should have its name set at runtime.

The new runtime call also allows us to eliminate the now-redundant
%DefineClassMethod runtime function.

This should get much less ugly once we can desugar the "dynamic"
part of object literals in the parser (but that work is currently
blocked on having a performant way of desugaring literals).

BUG=v8:3699, v8:3761
LOG=n
==========

to

==========
Support computed properties for ES2015 Function.name

Adds a new runtime function, %DefineDataPropertyInLiteral, which
takes a fifth argument specifying whether the property and value
are syntactically such that the value is a function (or class)
literal that should have its name set at runtime.

The new runtime call also allows us to eliminate the now-redundant
%DefineClassMethod runtime function.

This should get much less ugly once we can desugar the "dynamic"
part of object literals in the parser (but that work is currently
blocked on having a performant way of desugaring literals).

BUG=v8:3699, v8:3761
LOG=n
==========

[commit-bot: I haz the power, 2016-02-04 22:36:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-04 22:36:57]

Description was changed from

==========
Support computed properties for ES2015 Function.name

Adds a new runtime function, %DefineDataPropertyInLiteral, which
takes a fifth argument specifying whether the property and value
are syntactically such that the value is a function (or class)
literal that should have its name set at runtime.

The new runtime call also allows us to eliminate the now-redundant
%DefineClassMethod runtime function.

This should get much less ugly once we can desugar the "dynamic"
part of object literals in the parser (but that work is currently
blocked on having a performant way of desugaring literals).

BUG=v8:3699, v8:3761
LOG=n
==========

to

==========
Support computed properties for ES2015 Function.name

Adds a new runtime function, %DefineDataPropertyInLiteral, which
takes a fifth argument specifying whether the property and value
are syntactically such that the value is a function (or class)
literal that should have its name set at runtime.

The new runtime call also allows us to eliminate the now-redundant
%DefineClassMethod runtime function.

This should get much less ugly once we can desugar the "dynamic"
part of object literals in the parser (but that work is currently
blocked on having a performant way of desugaring literals).

BUG=v8:3699, v8:3761
LOG=n

Committed: https://crrev.com/21c045a2fafd171272c53b17a5d495f5be100662
Cr-Commit-Position: refs/heads/master@{#33756}
==========

[commit-bot: I haz the power, 2016-02-04 22:36:58]

Patchset 4 (id:??) landed as
https://crrev.com/21c045a2fafd171272c53b17a5d495f5be100662
Cr-Commit-Position: refs/heads/master@{#33756}

[rmcilroy, 2016-02-05 10:48:28]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2016-02-05 10:48:29]

+oth who implemented class literals for Ignition.

Thanks for letting us know Adam.