Addressed perf regression in Browsermark2.0 array blur test

A performance regression in array literal creation was caused by refactoring that eliminated a special fast case for shallow arrays. At the same time the general case got a bit slower. This CL restores most of the performance without coding the special fast case. The virtual dispatching is unnecessary because we know what we want to do at compile time. A flag was added to Runtime::CreateArrayLiteral. The flags delivers information about shallowness but also whether or not allocation mementos should be created. This is useful for crankshafted code.

BUG=v8:3008
LOG=N
R=mstarzinger@chromium.org

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

[mvstanton, 2013-11-20 14:33:00]

Hi Michael,
Can you have a look? Unfortunately I had to undo much of the refactoring that
consolidated CreateArrayLiteral and CreateArrayLiteralShallow. Sending a
follow-up mail.
thx,
--Michael

[Michael Starzinger, 2013-11-20 21:26:19]

One comment to address. I am fine with this fix (although I don't like it very
much). The general problem seems to be that DeepCopy is just not implemented as
efficiently as it could be.

For example the element kinds FAST_SMI_ELEMENTS and FAST_HOLEY_SMI_ELEMENTS
cannot contain any nested objects, so the iteration shouldn't be necessary. But
I won't block this CL over it (windmill tilting mode deactivated).

https://codereview.chromium.org/77293003/diff/80001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/77293003/diff/80001/src/ast.h#newcode1607
src/ast.h:1607: enum Flags {
These flags are used for an interface between generated code and the runtime,
it's unrelated to the AST. Please move the flags somewhere more appropriate. I
would propose the runtime.h file. You could even use bit-fields to encode the
flags.

[mvstanton, 2013-11-22 14:42:00]

Hi Michael,
Okay I had another run at it, and was able to get to a 3% degrade and keep the
general purpose code. I removed all the virtual dispatching and some unnecessary
method calls. 

I didn't yet move the ArrayLiteral flags to runtime.h, as this change is already
big. I'd probably prefer to do that in a follow-up CL which also moves the
ObjectLiteral flags in a similar fashion. Very simple CL, just touches a lot of
files.

Thanks much,
--Michael

https://codereview.chromium.org/77293003/diff/80001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/77293003/diff/80001/src/ast.h#newcode1607
src/ast.h:1607: enum Flags {
On 2013/11/20 21:26:19, Michael Starzinger wrote:
> These flags are used for an interface between generated code and the runtime,
> it's unrelated to the AST. Please move the flags somewhere more appropriate. I
> would propose the runtime.h file. You could even use bit-fields to encode the
> flags.

In that case I should probably move ObjectLiteral::Flags to runtime.h as well,
because I'm simply making the array literal infrastructure symmetrical with the
object literal infrastructure.

[Michael Starzinger, 2013-11-22 20:07:16]

LGTM. I give up!

https://codereview.chromium.org/77293003/diff/130001/src/allocation-site-scop...
File src/allocation-site-scopes.h (right):

https://codereview.chromium.org/77293003/diff/130001/src/allocation-site-scop...
src/allocation-site-scopes.h:64: current_ = Handle<AllocationSite>(*top_,
isolate());
nit: If this is the only use-site of AllocationSiteContext::isolate then we can
get rid of that field completely (constructor without isolate is usable).

https://codereview.chromium.org/77293003/diff/130001/src/allocation-site-scop...
src/allocation-site-scopes.h:115: 
nit: Only one empty new-line.

https://codereview.chromium.org/77293003/diff/130001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/77293003/diff/130001/src/objects.h#newcode2528
src/objects.h:2528: bool hint_object_is_shallow_array = false);
Yay, more magic boolean parameters. :/

[mvstanton, 2013-11-25 11:51:50]

Thanks for the help and impetus to try harder :)
--Michael

https://codereview.chromium.org/77293003/diff/130001/src/allocation-site-scop...
File src/allocation-site-scopes.h (right):

https://codereview.chromium.org/77293003/diff/130001/src/allocation-site-scop...
src/allocation-site-scopes.h:64: current_ = Handle<AllocationSite>(*top_,
isolate());
On 2013/11/22 20:07:16, Michael Starzinger wrote:
> nit: If this is the only use-site of AllocationSiteContext::isolate then we
can
> get rid of that field completely (constructor without isolate is usable).

Hmm, the isolate does get used to access the factory and create allocation sites
in allocation-site-scopes.cc, so it gets to stay in the boat.

https://codereview.chromium.org/77293003/diff/130001/src/allocation-site-scop...
src/allocation-site-scopes.h:115: 
On 2013/11/22 20:07:16, Michael Starzinger wrote:
> nit: Only one empty new-line.

Done.

https://codereview.chromium.org/77293003/diff/130001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/77293003/diff/130001/src/objects.h#newcode2528
src/objects.h:2528: bool hint_object_is_shallow_array = false);
On 2013/11/22 20:07:16, Michael Starzinger wrote:
> Yay, more magic boolean parameters. :/

Good point, I replaced it with an enum DeepCopyHints, that is passed down into
StructureWalk for consumption.

[mvstanton, 2013-11-25 12:41:42]

Committed patchset #7 manually as r18046 (presubmit successful).