New issue
Advanced search Search tips

Issue 819217 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Bug for investigating memory regression on context (maybe related to Array Iterator redesign)

Project Member Reported by bmeu...@chromium.org, Mar 6 2018

Issue description

Let's see
 
Let me see

Cc: ca...@igalia.com mythria@chromium.org mstarzinger@chromium.org jgruber@chromium.org machenb...@chromium.org mlippautz@chromium.org leszeks@chromium.org bmeu...@chromium.org mvstan...@chromium.org cbruni@chromium.org adamk@chromium.org gsat...@chromium.org kanghua...@intel.com ishell@chromium.org mathias@chromium.org yangguo@chromium.org sigurds@chromium.org hpayer@chromium.org rmcilroy@chromium.org hablich@chromium.org neis@chromium.org petermarshall@chromium.org marja@chromium.org clemensh@chromium.org jkummerow@chromium.org grt@chromium.org
Status: Assigned (was: Started)
📍 Found significant differences after each of 33 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/14b24e62440000

Roll V8 back to 6.6.346. by grt@chromium.org
https://chromium.googlesource.com/chromium/src/+/1808b6997fe1e50153125f054a6589863733b5bf

[builtins] Remove the ObjectConstructor_ConstructStub builtin. by bmeurer@chromium.org
https://chromium.googlesource.com/v8/v8/+/c8f34835af9168ad75c43d79da070fd541106040

[builtins] Remove BooleanConstructor_ConstructStub builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/df5483caeee99df7df4a84da385a9b70440a19e5

[builtins] Remove BigIntConstructor_ConstructStub builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/306f2fd5a78ba596ab32e8478c2a0f5833a8b295

[builtins] Remove ArrayBufferConstructor_ConstructStub builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/f8c688f4c26c8186788026ff26dfb9dffdf3be6e

[builtins] Remove DateConstructor_ConstructStub builtin. by bmeurer@chromium.org
https://chromium.googlesource.com/v8/v8/+/251f63d82f82f1f67393566e5757a162821b6262

[builtins] Remove SymbolConstructor_ConstructStub builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/0ffaedf8d55861059d47ddcb037b0117369ae321

[builtins] Remove DataViewConstructor_ConstructStub builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/fa82d01ed06c59800f23631487927e17f817b415

Fix buffer-detached check in TypedArray.of/from by jkummerow@chromium.org
https://chromium.googlesource.com/v8/v8/+/c94df3cec425d6fda250a19b3cc75ef924529e3d

[x64] Use XOR instruction to zero register in SpeculationPoison by kanghua.yu@intel.com
https://chromium.googlesource.com/v8/v8/+/a4b615eba1ae2963b0c5659ca22ba42bf16c09bd

[builtins] Refactor the StringConstructor builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/15e207b300bf7f7b70df83e98cbaf8c0d99a7485

[runtime] Move validity cell from PrototypeInfo to Map. by ishell@chromium.org
https://chromium.googlesource.com/v8/v8/+/40a3e6dcb9bdb5cb2a7f6e29fa8e4de75e538ef6

[wasm] Turn {WasmDebugInfo} into a proper {Struct}. by mstarzinger@chromium.org
https://chromium.googlesource.com/v8/v8/+/67fa841bcb1e43977f24917d4b57be2284c6e8e5

Reland "[parser] Remove pretenuring of closures assigned to properties" by adamk@chromium.org
https://chromium.googlesource.com/v8/v8/+/3d7ad2e7e53d3cd6add51512580c054abf7e5805

[parsing] inline ArrayLiteral creation for spread calls by caitp@igalia.com
https://chromium.googlesource.com/v8/v8/+/93fc3841c3da0fc85662e66a57d881555238d634

[runtime] Always store the name in a function's ScopeInfo by cbruni@chromium.org
https://chromium.googlesource.com/v8/v8/+/01488b9c4f1b4c8e2b66494b24d7e7ff8a826860

Revert "[parsing] inline ArrayLiteral creation for spread calls" by sigurds@chromium.org
https://chromium.googlesource.com/v8/v8/+/f48e7349035430ea13dd0ce1ed1e775f6eecabeb

[es2015] Extend the array iterator protector. by bmeurer@chromium.org
https://chromium.googlesource.com/v8/v8/+/1525374ff5a564b55b748ad33e6cd0d0ea684006

Reland "[parsing] inline ArrayLiteral creation for spread calls" by neis@chromium.org
https://chromium.googlesource.com/v8/v8/+/82345e9fbfb4f6b0177c19eb476cc6c6babcef82

[wasm] Turn {WasmSharedModuleData} into a proper {Struct}. by mstarzinger@chromium.org
https://chromium.googlesource.com/v8/v8/+/d623fcae9cad5cb56475b7f1065a0a1adc2cf432

[builtins] Refactor the NumberConstructor builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/279085d852cb0c90c19fab73e9b464f354c7d707

[callbacks] Clean up PropertyCallbackArguments by cbruni@chromium.org
https://chromium.googlesource.com/v8/v8/+/bb862bbc504db5a296ced8e057923cc1d83606f8

[runtime] Add BOILERPLATE_DESCRIPTION_TYPE InstanceType by cbruni@chromium.org
https://chromium.googlesource.com/v8/v8/+/1f307ba52fa3a7f820be554f7212b7b302b49b62

[ic] Remove pointless macros and obsolete comments. by neis@chromium.org
https://chromium.googlesource.com/v8/v8/+/97b3a968a518baf98e93a0f399a579f9128b8e31

[builtins] Refactor the ProxyConstructor builtin by mathias@chromium.org
https://chromium.googlesource.com/v8/v8/+/08e168d0ff38f97f8638d25fdd050dda7ae10ca6

[ic] Introduce new IC for storing into array literals. by neis@chromium.org
https://chromium.googlesource.com/v8/v8/+/2e2860f74f9c610d5ffb3ee1778d937055d25dea

[builtins] Enable embedded builtins and add testing variants by jgruber@chromium.org
https://chromium.googlesource.com/v8/v8/+/abcc28ced071faee637d5fd59d06cfdf80d8ea3f

[ic] Relax a CHECK. by neis@chromium.org
https://chromium.googlesource.com/v8/v8/+/c895a23a996beafcd706e18656c8673338167ebd

[turbofan] Fix bug in Array.p.reduceRight by sigurds@chromium.org
https://chromium.googlesource.com/v8/v8/+/d1df563059107761c21d9c587169302644e906de

[interpreter] Only create spread-related feedback slots when necessary. by neis@chromium.org
https://chromium.googlesource.com/v8/v8/+/cf8cd1c444e7975211915fc77c52fe3c66876f78

[in-place weak refs] Add in-place weak references & migrate one WeakCell to it. by marja@chromium.org
https://chromium.googlesource.com/v8/v8/+/07c1e641d9864cb2de9692cf69bccca4b49a6f6a

Revert "[in-place weak refs] Add in-place weak references & migrate one WeakCell to it." by sigurds@chromium.org
https://chromium.googlesource.com/v8/v8/+/73d6037c2051816fe926ae127aa52e863a800189

[es2015] Refactor the JSArrayIterator. by bmeurer@chromium.org
https://chromium.googlesource.com/v8/v8/+/06ee127b75726f9ee541aab10f6aecfe4d96675a

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Comment 4 by dtu@chromium.org, Mar 7 2018

Cc: perezju@chromium.org dtu@chromium.org
Owner: jgruber@chromium.org
<dtu from Pinpoint team> looks like all of these commits caused an improvement or regression in memory, but most of them were very small. The biggest regressions were at:
[runtime] Move validity cell from PrototypeInfo to Map. by ishell@chromium.org
https://chromium.googlesource.com/v8/v8/+/40a3e6dcb9bdb5cb2a7f6e29fa8e4de75e538ef6

[builtins] Enable embedded builtins and add testing variants by jgruber@chromium.org
https://chromium.googlesource.com/v8/v8/+/abcc28ced071faee637d5fd59d06cfdf80d8ea3f

I hope to have the regression sizes in the bug comment too (go/catabug/3871)
And only CC the authors/reviewers of the biggest changes (go/catabug/4317)

Also +perezju, the benchmark owner.
Owner: bmeu...@chromium.org
[builtins] Enable embedded builtins and add testing variants

From the pinpoint job, it looks like this increases memory usage (memory:chrome:all_processes:reported_by_chrome:v8:heap:allocated_objects_size) by roughly 20K.

AFAIK there's two sources of memory regressions in this CL:

1. It introduces the constants list containing all constants and external references which were previously embedded into builtins. The constants list is a FixedArray on the root list.

2. Builtins now access constants and external refs through an indirection, which produces a longer instruction sequence than directly embedded constants.

An increase by 20K is not unexpected, and once builtins are entirely off-heap, the memory savings should outweigh these regressions by far. Alternatively, we still have the option to unship embedded builtins if they are not ready for 67.

bmeurer@, your CL ([es2015] Refactor the JSArrayIterator) reduces memory consumption by roughly 10k. 
ishell@, your CL ([runtime] Move validity cell from PrototypeInfo to Map.) increases it by <10k.

Assigning back to bmeurer@.
Cc: -petermarshall@chromium.org
FYI: this CL
  [runtime] Move validity cell from PrototypeInfo to Map. by ishell@chromium.org
  https://chromium.googlesource.com/v8/v8/+/40a3e6dcb9bdb5cb2a7f6e29fa8e4de75e538ef6
is expected to regress memory. The regression will be partially recovered once the next CL lands (https://chromium-review.googlesource.com/c/v8/v8/+/931701).
Owner: petermarshall@chromium.org
As determined by jgruber@ my CL improves memory. Assigning to memory sheriff for investigation.
Cc: -perezju@chromium.org -mlippautz@chromium.org -grt@chromium.org -machenb...@chromium.org -mvstan...@chromium.org -jkummerow@chromium.org -yangguo@chromium.org -leszeks@chromium.org -cbruni@chromium.org -hpayer@chromium.org -gsat...@chromium.org -ca...@igalia.com -hablich@chromium.org -dtu@chromium.org -mythria@chromium.org -neis@chromium.org -kanghua...@intel.com -mathias@chromium.org -adamk@chromium.org -rmcilroy@chromium.org -sigurds@chromium.org -mstarzinger@chromium.org -clemensh@chromium.org -u...@chromium.org -marja@chromium.org
Status: WontFix (was: Assigned)
There are two large (for this range) regressions and one large improvement. The two large regressions mentioned by @dtu cover the vast majority of the increase across this range, so as long as jgruber and ishell have a plan to address them, there is no action needed on this bug. See the picture attached for a graphical representation :)
Memory Regress Graph.png
63.4 KB View Download

Sign in to add a comment