Issue 8968028: Ensure large Smi-only arrays don't transition to FAST_DOUBLE_ARRAY

Issue 8968028: Ensure large Smi-only arrays don't transition to FAST_DOUBLE_ARRAY (Closed)

Created:
8 years, 12 months ago by danno

Modified:
8 years, 11 months ago

Reviewers:
Vyacheslav Egorov (Chromium), Jakob Kummerow

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Ensure large Smi-only arrays don't transition to FAST_DOUBLE_ARRAY BUG=v8:1849 TEST=test/mjsunit/regress/regress-1849.js Committed: http://code.google.com/p/v8/source/detail?r=10309

Patch Set 2 : Transition to Smi only arrays #

Total comments: 8

Patch Set 3 : review feedback #

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+43 lines, -30 lines)			Patch
M	src/objects.h	View	1	2 chunks	+4 lines, -2 lines	0 comments	Download
M	src/objects.cc	View	1	3 chunks	+28 lines, -8 lines	0 comments	Download
M	src/objects-inl.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
A +	test/mjsunit/regress/regress-1849.js	View	1 2	1 chunk	+9 lines, -18 lines	0 comments	Download
M	test/mjsunit/regress/regress-95113.js	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Jakob Kummerow

LGTM with comments addressed. http://codereview.chromium.org/8968028/diff/2001/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/8968028/diff/2001/src/objects.cc#newcode9253 src/objects.cc:9253: if (has_smi_only_elements) { I think ...

8 years, 12 months ago (2011-12-29 15:18:41 UTC) #2

Vyacheslav Egorov (Chromium)

DBC http://codereview.chromium.org/8968028/diff/2001/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/8968028/diff/2001/src/objects-inl.h#newcode1222 src/objects-inl.h:1222: ASSERT(current->IsSmi() || current-IsTheHole()); current->IsHole()

8 years, 12 months ago (2011-12-29 15:54:48 UTC) #3

danno

8 years, 11 months ago (2011-12-30 14:12:02 UTC) #4

http://codereview.chromium.org/8968028/diff/2001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/8968028/diff/2001/src/objects-inl.h#newcode1222
src/objects-inl.h:1222: ASSERT(current->IsSmi() || current-IsTheHole());
On 2011/12/29 15:54:48, Vyacheslav Egorov wrote:
> current->IsHole()

Done.

http://codereview.chromium.org/8968028/diff/2001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8968028/diff/2001/src/objects.cc#newcode9253
src/objects.cc:9253: if (has_smi_only_elements) {
There's no harm in going to smi elements for very large arrays when the flag
isn't on. We already do this for very large array literals. The smi_only_arrays
flag has become a bit of a misnomer, it now means "enable the smi/double
features that are known to still cause performance regressions".

On 2011/12/29 15:18:41, Jakob wrote:
> I think you need an additional "&& FLAG_smi_only_arrays" here, otherwise it'll
> transition to SMI elements even if the flag is off.

http://codereview.chromium.org/8968028/diff/2001/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-1849.js (right):

http://codereview.chromium.org/8968028/diff/2001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-1849.js:1: // Copyright 2009 the V8 project
authors. All rights reserved.
On 2011/12/29 15:18:41, Jakob wrote:
> 2011

Done.

http://codereview.chromium.org/8968028/diff/2001/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-1849.js:30: // Flags: --allow-natives-syntax
See the other comment. 
On 2011/12/29 15:18:41, Jakob wrote:
> --smi-only-arrays ? And then you'll need to put the assertion behind an if()
> that checks if the flag is in effect.

Expand Messages | Collapse Messages