Partial recover from performance degradation after handlification of ElementsAccessor::CopyElements…

Partial recover from performance degradation after handlification of ElementsAccessor::CopyElements (r20312).

R=yangguo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20464

[Igor Sheludko, 2014-04-03 07:41:11]

PTAL

https://codereview.chromium.org/223413002/diff/1/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/223413002/diff/1/src/factory.h#newcode341
src/factory.h:341: }
This piece of code should be here from the beginning (r20156). I left it in a
general version of NewJSArray() by mistake.

[Yang, 2014-04-03 08:10:42]

LGTM once comments are addressed.

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

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode364
src/builtins.cc:364: AllowHeapAllocation allow_allocation;
Add a comment that the callees return immediately afterwards

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode637
src/builtins.cc:637: DisallowHeapAllocation
no_allocations_before_creating_result;
I would prefer if you still use scopes instead of interchanging Disallow/Allow
whenever possible. That would improve readability a lot imo.

For example, scope this to go from line 637 to 714.

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode730
src/builtins.cc:730: if (IsHoleyElementsKind(kind)) {
Then add a DisallowHeapAllocation scope here.

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode746
src/builtins.cc:746: AllowHeapAllocation allow_allocation_to_create_a_result;
This is now unnecessary.

[Igor Sheludko, 2014-04-03 09:00:59]

PTAL again, there were more changes.

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

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode364
src/builtins.cc:364: AllowHeapAllocation allow_allocation;
On 2014/04/03 08:10:43, Yang wrote:
> Add a comment that the callees return immediately afterwards

As discussed offline, it is safer to put explicit AllowAllocationScope before
each affected call of CallJsBuiltin().

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode637
src/builtins.cc:637: DisallowHeapAllocation
no_allocations_before_creating_result;
On 2014/04/03 08:10:43, Yang wrote:
> I would prefer if you still use scopes instead of interchanging Disallow/Allow
> whenever possible. That would improve readability a lot imo.
> 
> For example, scope this to go from line 637 to 714.

Done.

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode730
src/builtins.cc:730: if (IsHoleyElementsKind(kind)) {
On 2014/04/03 08:10:43, Yang wrote:
> Then add a DisallowHeapAllocation scope here.

Done.

https://codereview.chromium.org/223413002/diff/1/src/builtins.cc#newcode746
src/builtins.cc:746: AllowHeapAllocation allow_allocation_to_create_a_result;
On 2014/04/03 08:10:43, Yang wrote:
> This is now unnecessary.

Done.

[Yang, 2014-04-03 09:11:00]

LGTM.

[Igor Sheludko, 2014-04-03 09:13:10]

Committed patchset #2 manually as r20464 (presubmit successful).