[runtime] Speed up C++ version of ArrayPush

[runtime] Speed up C++ version of ArrayPush

Mostly by avoiding unnecessary Handle/HandleScope creation,
"length" property lookups, and length conversions.
This yields about 60% speedup on the microbenchmark I tested with.

Note that the C++ builtin is the middle performance tier of three,
so not every Array.push use case will be affected by this patch.

Committed: https://crrev.com/2ae50119cbf794b5c67b412b775bc40e132af1c8
Cr-Commit-Position: refs/heads/master@{#34268}

[Jakob Kummerow, 2016-02-19 16:39:07]

jkummerow@chromium.org changed reviewers:
+ ulan@chromium.org

[Jakob Kummerow, 2016-02-19 16:39:08]

Ulan: PTAL, CQ it if you like it.

Toon: FYI.

[Camillo Bruni, 2016-02-22 19:25:54]

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

[Camillo Bruni, 2016-02-22 19:25:55]

nice! indeed a slightly cleaner solution than my mess :P

https://codereview.chromium.org/1716833002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1716833002/diff/1/src/objects.cc#newcode16252
src/objects.cc:16252: // Optimistic fast path: "length" is usually the first
fast property.
right :) makes sense

[ulan, 2016-02-24 09:09:05]

lgtm, nice clean-up

[ulan, 2016-02-24 09:09:11]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2016-02-24 09:09:19]

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

[commit-bot: I haz the power, 2016-02-24 09:10:28]

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

[commit-bot: I haz the power, 2016-02-24 09:10:29]

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_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14226)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/1831)
  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_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Toon Verwaest, 2016-02-24 10:31:04]

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

[Toon Verwaest, 2016-02-24 10:31:05]

DBC, fine to handle in another CL.

https://codereview.chromium.org/1716833002/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1716833002/diff/1/src/builtins.cc#newcode270
src/builtins.cc:270: // Use a short-lived HandleScope to avoid creating several
copies of the
Push shouldn't need to EnsureWritableFastElements since there wouldn't be space
in the COW array to add the value anyway. It would need to copy anyway. It seems
like EnsureWritableFastElement should only be done in the ElementsAccessor in
the cases where it matters; and probably this entire method should be handled by
the ElementsAccessors so it can be compiled away where irrelevant.

[Jakob Kummerow, 2016-02-25 05:18:38]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-02-25 05:18:38]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/1716833002/#ps40001
(title: "add TODO for Toon's comment")

[commit-bot: I haz the power, 2016-02-25 05:18:45]

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

[commit-bot: I haz the power, 2016-02-25 05:40:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-25 05:40:41]

Description was changed from

==========
[runtime] Speed up C++ version of ArrayPush

Mostly by avoiding unnecessary Handle/HandleScope creation,
"length" property lookups, and length conversions.
This yields about 60% speedup on the microbenchmark I tested with.

Note that the C++ builtin is the middle performance tier of three,
so not every Array.push use case will be affected by this patch.
==========

to

==========
[runtime] Speed up C++ version of ArrayPush

Mostly by avoiding unnecessary Handle/HandleScope creation,
"length" property lookups, and length conversions.
This yields about 60% speedup on the microbenchmark I tested with.

Note that the C++ builtin is the middle performance tier of three,
so not every Array.push use case will be affected by this patch.

Committed: https://crrev.com/2ae50119cbf794b5c67b412b775bc40e132af1c8
Cr-Commit-Position: refs/heads/master@{#34268}
==========

[commit-bot: I haz the power, 2016-02-25 05:40:42]

Patchset 3 (id:??) landed as
https://crrev.com/2ae50119cbf794b5c67b412b775bc40e132af1c8
Cr-Commit-Position: refs/heads/master@{#34268}