Adding ElementsAccessor::Pop

Adding ElementsAccessor::Pop
Moving FastElements path to ElementsAccessor.

BUG=

Committed: https://crrev.com/ca8134c834e96eacb3c7756683fdf8220ba5054a
Cr-Commit-Position: refs/heads/master@{#30477}

[Camillo Bruni, 2015-08-28 12:12:52]

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

[commit-bot: I haz the power, 2015-08-28 12:13:04]

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

[commit-bot: I haz the power, 2015-08-28 12:26:17]

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

[commit-bot: I haz the power, 2015-08-28 12:26:18]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/9095)

[Camillo Bruni, 2015-08-28 14:08:11]

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

[Camillo Bruni, 2015-08-28 14:08:11]

PTAL

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc#newcode394
src/builtins.cc:394: result = isolate->factory()->undefined_value();
I am not fully convinced about this branch myself. But I think this is the only
value that has to be converted for a FastElements Array.

[Camillo Bruni, 2015-08-31 07:45:50]

cbruni@chromium.org changed reviewers:
+ ishell@chromium.org
- yangguo@chromium.org

[Camillo Bruni, 2015-08-31 07:45:50]

PTAL

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc#newcode390
src/builtins.cc:390: if (IsJSArrayFastElementMovingAllowed(isolate,
JSArray::cast(*receiver))) {
Using this separate branch I gain roughly 50% speedup on a tight pop loop.

[Igor Sheludko, 2015-08-31 14:08:03]

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc#newcode393
src/builtins.cc:393: if (result->IsTheHole()) {
I think hole handling should be in elements.cc and it's only necessary for holy
arrays (see usages of IsHoleyElementsKind(kind()) ).

https://codereview.chromium.org/1325483002/diff/20001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1325483002/diff/20001/src/elements.cc#newcode647
src/elements.cc:647: static Handle<Object> PopImpl(Handle<JSArray> receiver,
I think FastElementsAccessor is a better place for this implementation. And
ElementsAccessorBase::PopImpl should be unreachable.

[Camillo Bruni, 2015-08-31 14:40:26]

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

[commit-bot: I haz the power, 2015-08-31 14:40:30]

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

[Camillo Bruni, 2015-08-31 14:41:35]

Addressed comments

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1325483002/diff/20001/src/builtins.cc#newcode393
src/builtins.cc:393: if (result->IsTheHole()) {
On 2015/08/31 at 14:08:03, Igor Sheludko wrote:
> I think hole handling should be in elements.cc and it's only necessary for
holy arrays (see usages of IsHoleyElementsKind(kind()) ).

Makes sense.

https://codereview.chromium.org/1325483002/diff/20001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1325483002/diff/20001/src/elements.cc#newcode647
src/elements.cc:647: static Handle<Object> PopImpl(Handle<JSArray> receiver,
On 2015/08/31 at 14:08:03, Igor Sheludko wrote:
> I think FastElementsAccessor is a better place for this implementation. And
ElementsAccessorBase::PopImpl should be unreachable.

moved PopImpl down to FastElementsAccessor

[Igor Sheludko, 2015-08-31 14:54:10]

lgtm

[commit-bot: I haz the power, 2015-08-31 15:05:28]

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

[commit-bot: I haz the power, 2015-08-31 15:05:28]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-08-31 15:16:16]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2015-08-31 15:16:24]

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

[commit-bot: I haz the power, 2015-08-31 15:18:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-31 15:18:20]

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