[builtins] Introduce DeleteProperty builtin

[builtins] Introduce DeleteProperty builtin

So that we can delete object properties without a runtime call.
The builtin implements a few fast paths (for now only deletion
of dictionary properties), and calls the runtime for all other
cases.

Review-Url: https://codereview.chromium.org/2810363003
Cr-Commit-Position: refs/heads/master@{#44740}
Committed: https://chromium.googlesource.com/v8/v8/+/43fd247c52b7c4e2ae279ed56070be9d8bcc9494

[Jakob Kummerow, 2017-04-19 14:41:50]

jkummerow@chromium.org changed reviewers:
+ bmeurer@chromium.org, ishell@chromium.org

[Jakob Kummerow, 2017-04-19 14:41:50]

Benedikt: please review src/compiler/*. The changes there are straightforward;
the language_mode is now a parameter to a single runtime function that handles
both modes.

Igor: please review the rest.

Upcoming follow-up: handle the last-added fast property. Landing separately to
isolate each change's impact (and make reviewing easier).

https://codereview.chromium.org/2810363003/diff/20001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/2810363003/diff/20001/src/js/array.js#newcode405
src/js/array.js:405: delete array[n];
This entire file is in strict mode anyway (see line 7), so this doesn't change
behavior, but ensures that we go through the CSA builtin here.

[Jakob Kummerow, 2017-04-19 14:42:23]

Description was changed from

==========
[builtins] Introduce DeleteProperty builtin

So that we can delete object properties without a runtime call.
The builtin will implement a few fast paths, and call the existing
runtime functions for all other cases.
==========

to

==========
[builtins] Introduce DeleteProperty builtin

So that we can delete object properties without a runtime call.
The builtin implements a few fast paths (for now only deletion
of dictionary properties), and calls the runtime for all other
cases.
==========

[Jakob Kummerow, 2017-04-19 14:45:08]

The CQ bit was checked by jkummerow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 14:45:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 15:13:57]

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

[commit-bot: I haz the power, 2017-04-19 15:13:58]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2017-04-19 16:50:17]

src/compiler LGTM

[Igor Sheludko, 2017-04-20 09:18:43]

lgtm with nits:

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
File src/builtins/builtins-internal-gen.cc (right):

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:188: StoreFixedArrayElement(properties,
key_index, filler);
TheHoleValue is in IMMORTAL_IMMOVABLE_ROOT_LIST so we can skip write barrier
here

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:189:
StoreValueByKeyIndex<NameDictionary>(properties, key_index, filler);
and here.

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:262: GotoIf(SmiEqual(language_mode,
SmiConstant(STRICT)), &slow);
is_strict(lm) is implemented as lm != SLOPPY which should also generate a little
bit better code because SLOPPY is 0.
Maybe also STATIC_ASSERT(LANGUAGE_END == 2).

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:276: { Return(TrueConstant()); }
I think this line will look nicer without {}.

https://codereview.chromium.org/2810363003/diff/20001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/2810363003/diff/20001/src/code-factory.h#newc...
src/code-factory.h:202: static Callable DeleteProperty(Isolate* isolate);
This is not necessary anymore, see Builtins::CallableFor(...).

https://codereview.chromium.org/2810363003/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2810363003/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:22867: +  CheckCorrectThrow("%DeleteProperty(other, 'x',
0)");  // 0 == SLOPPY
Maybe add STATIC_ASSERTs?

[Jakob Kummerow, 2017-04-20 09:52:34]

Thanks for the reviews; comments addressed; landing.

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
File src/builtins/builtins-internal-gen.cc (right):

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:188: StoreFixedArrayElement(properties,
key_index, filler);
On 2017/04/20 09:18:42, Igor Sheludko wrote:
> TheHoleValue is in IMMORTAL_IMMOVABLE_ROOT_LIST so we can skip write barrier
> here

Done.

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:189:
StoreValueByKeyIndex<NameDictionary>(properties, key_index, filler);
On 2017/04/20 09:18:42, Igor Sheludko wrote:
> and here.

Done.

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:262: GotoIf(SmiEqual(language_mode,
SmiConstant(STRICT)), &slow);
On 2017/04/20 09:18:42, Igor Sheludko wrote:
> is_strict(lm) is implemented as lm != SLOPPY which should also generate a
little
> bit better code because SLOPPY is 0.
> Maybe also STATIC_ASSERT(LANGUAGE_END == 2).

Done.

https://codereview.chromium.org/2810363003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-internal-gen.cc:276: { Return(TrueConstant()); }
On 2017/04/20 09:18:42, Igor Sheludko wrote:
> I think this line will look nicer without {}.

Done.

https://codereview.chromium.org/2810363003/diff/20001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/2810363003/diff/20001/src/code-factory.h#newc...
src/code-factory.h:202: static Callable DeleteProperty(Isolate* isolate);
On 2017/04/20 09:18:42, Igor Sheludko wrote:
> This is not necessary anymore, see Builtins::CallableFor(...).

Done.

https://codereview.chromium.org/2810363003/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2810363003/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:22867: +  CheckCorrectThrow("%DeleteProperty(other, 'x',
0)");  // 0 == SLOPPY
On 2017/04/20 09:18:42, Igor Sheludko wrote:
> Maybe add STATIC_ASSERTs?

Done.

[Jakob Kummerow, 2017-04-20 09:52:39]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-04-20 09:52:39]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2810363003/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-04-20 09:52:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 10:13:09]

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

[commit-bot: I haz the power, 2017-04-20 10:13:10]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Jakob Kummerow, 2017-04-20 11:46:47]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-04-20 11:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2810363003/#ps60001
(title: "fix CallableFor not to create handles")

[commit-bot: I haz the power, 2017-04-20 11:46:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 12:11:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492688807159960,
"parent_rev": "64bb6e6c9026862a8f0091d56c8610bdd34a49bd", "commit_rev":
"43fd247c52b7c4e2ae279ed56070be9d8bcc9494"}

[commit-bot: I haz the power, 2017-04-20 12:11:13]

Description was changed from

==========
[builtins] Introduce DeleteProperty builtin

So that we can delete object properties without a runtime call.
The builtin implements a few fast paths (for now only deletion
of dictionary properties), and calls the runtime for all other
cases.
==========

to

==========
[builtins] Introduce DeleteProperty builtin

So that we can delete object properties without a runtime call.
The builtin implements a few fast paths (for now only deletion
of dictionary properties), and calls the runtime for all other
cases.

Review-Url: https://codereview.chromium.org/2810363003
Cr-Commit-Position: refs/heads/master@{#44740}
Committed:
https://chromium.googlesource.com/v8/v8/+/43fd247c52b7c4e2ae279ed56070be9d8bc...
==========

[commit-bot: I haz the power, 2017-04-20 12:11:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/43fd247c52b7c4e2ae279ed56070be9d8bc...

[Jakob Kummerow, 2017-04-21 18:03:23]

This has improved "Group-Runtime" on speedometer/angular by 14ms or 9%:

https://chromeperf.appspot.com/report?sid=7acd0dfb5d4b4274730e8694ac49cc22a8d...

[Benedikt Meurer, 2017-04-23 16:45:00]

On 2017/04/21 18:03:23, Jakob Kummerow wrote:
> This has improved "Group-Runtime" on speedometer/angular by 14ms or 9%:
> 
>
https://chromeperf.appspot.com/report?sid=7acd0dfb5d4b4274730e8694ac49cc22a8d...

Nice! Thanks Jakob.