Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(591)

Issue 23622016: limit InitialMaxFastElement by MaxRegularSpaceAllocationSize (Closed)

Created:
7 years, 3 months ago by weiliang.lin2
Modified:
7 years, 3 months ago
Reviewers:
Jakob Kummerow
CC:
v8-dev
Base URL:
https://github.com/v8/v8.git@master
Visibility:
Public.

Description

limit InitialMaxFastElement by MaxRegularSpaceAllocationSize BUG=

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -11 lines) Patch
M src/code-stubs-hydrogen.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/elements.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/objects.h View 2 chunks +2 lines, -4 lines 0 comments Download
M src/objects.cc View 1 chunk +9 lines, -0 lines 0 comments Download
M src/runtime.cc View 4 chunks +6 lines, -5 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
weiliang.lin2
To deopt when the allocation size (including elements size and array base size) exceeds MaxRegularSpaceAllocationSize. ...
7 years, 3 months ago (2013-09-01 10:20:10 UTC) #1
Jakob Kummerow
7 years, 3 months ago (2013-09-02 13:06:05 UTC) #2
If I understand correctly, this CL is functionally equivalent to:

#if V8_TARGET_ARCH_X64
  static const int kInitialMaxFastElementArray = 98297;
#else
  static const int kInitialMaxFastElementArray = 100000;
#endif

Right?
While a computed value is arguably cleaner (and more robust towards potential
future target architectures) than an #if, I'm still not sure it's the best
solution here. Since fast vs. dictionary elements can have a significant impact
on performance, it would be quite surprising to have different behavior on
different platforms.

Since the limit we're hitting is kind of arbitrary anyway, how about simply
bumping that a bit? See https://codereview.chromium.org/23604023 for what I
mean.

Powered by Google App Engine
This is Rietveld 408576698