Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by Michael Lippautz Modified:
4 years, 4 months ago Reviewers:
Hannes Payer (out of office), jochen (gone - plz use gerrit) CC:
Hannes Payer (out of office), ulan, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Improve size profiling for ArrayBuffer tracking
Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this,
we can now do Scavenges if the amount of memory retained from new space is too large.
BUG=chromium:621829
R=jochen@chromium.org,hpayer@chromium.org
Committed: https://crrev.com/28e13bd6a75c9467dae43043e7b741a1387d5252
Cr-Commit-Position: refs/heads/master@{#38731}
Patch Set 1 #Patch Set 2 : Added tests and some more fixes #
Total comments: 7
Patch Set 3 : Addressed comments #Patch Set 4 : Minor cleanups #Patch Set 5 : More small improvements #Patch Set 6 : rebase #
Created: 4 years, 4 months ago
Messages
Total messages: 42 (32 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [heap] Improve size profiling for ArrayBuffer tracking BUG= ========== to ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. BUG=chromium:621829 ==========
Description was changed from ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. BUG=chromium:621829 ========== to ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. BUG=chromium:621829 R=jochen@chromium.org, hpayer@chromium.org ==========
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org, jochen@chromium.org
Description was changed from ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. BUG=chromium:621829 R=jochen@chromium.org, hpayer@chromium.org ========== to ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ==========
PTAL We can then base our decisions on what to do in case of too much external memory on these counters (in case it's dominated by array buffers). If this approach turns out to be too slow and regress performance, we could have a retained counter on LocalArrayBufferTracker and iterate *all* pages of a space when we need a global picture. https://codereview.chromium.org/2210263002/diff/40001/src/heap/array-buffer-t... File src/heap/array-buffer-tracker.cc (right): https://codereview.chromium.org/2210263002/diff/40001/src/heap/array-buffer-t... src/heap/array-buffer-tracker.cc:151: retained_from_new_space_.Increment( General: When (and if) https://codereview.chromium.org/2215693002/ lands, we can write this much nicer, staying completely in size_t land.
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
How are array buffer heavy workloads effected by this change? Can you check beforehand? https://codereview.chromium.org/2210263002/diff/40001/src/heap/array-buffer-t... File src/heap/array-buffer-tracker.cc (right): https://codereview.chromium.org/2210263002/diff/40001/src/heap/array-buffer-t... src/heap/array-buffer-tracker.cc:151: retained_from_new_space_.Increment( On 2016/08/04 11:50:02, Michael Lippautz wrote: > General: When (and if) https://codereview.chromium.org/2215693002/ lands, we can > write this much nicer, staying completely in size_t land. Nice, please do that. https://codereview.chromium.org/2210263002/diff/40001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/2210263002/diff/40001/src/heap/heap.h#newcode... src/heap/heap.h:1418: ArrayBufferTracker* array_buffer_tracker() { return array_buffer_tracker_; } Move this one to the ArrayBuffer tracking section. https://codereview.chromium.org/2210263002/diff/40001/src/heap/heap.h#newcode... src/heap/heap.h:2272: ArrayBufferTracker* array_buffer_tracker_; We usually add comments to all variables in heap.h
https://codereview.chromium.org/2210263002/diff/40001/src/heap/array-buffer-t... File src/heap/array-buffer-tracker.cc (right): https://codereview.chromium.org/2210263002/diff/40001/src/heap/array-buffer-t... src/heap/array-buffer-tracker.cc:151: retained_from_new_space_.Increment( On 2016/08/05 12:14:13, Hannes Payer wrote: > On 2016/08/04 11:50:02, Michael Lippautz wrote: > > General: When (and if) https://codereview.chromium.org/2215693002/ lands, we > can > > write this much nicer, staying completely in size_t land. > > Nice, please do that. Done. https://codereview.chromium.org/2210263002/diff/40001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/2210263002/diff/40001/src/heap/heap.h#newcode... src/heap/heap.h:1418: ArrayBufferTracker* array_buffer_tracker() { return array_buffer_tracker_; } On 2016/08/05 12:14:13, Hannes Payer wrote: > Move this one to the ArrayBuffer tracking section. Done. https://codereview.chromium.org/2210263002/diff/40001/src/heap/heap.h#newcode... src/heap/heap.h:2272: ArrayBufferTracker* array_buffer_tracker_; On 2016/08/05 12:14:14, Hannes Payer wrote: > We usually add comments to all variables in heap.h That's not really true, but done ;)
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6504)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM if performance is fine
Performance is within ~3% on the considered perf.bindings benchmarks which are micro benchmarks only allocating JSArrayBuffers. A followup CL will make use of the information gathered here.
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ========== to ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this, we can now do Scavenges if the amount of memory retaind from new space is too large. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ==========
Description was changed from ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this, we can now do Scavenges if the amount of memory retaind from new space is too large. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ========== to ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this, we can now do Scavenges if the amount of memory retained from new space is too large. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ==========
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/12126) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, hpayer@chromium.org Link to the patchset: https://codereview.chromium.org/2210263002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this, we can now do Scavenges if the amount of memory retained from new space is too large. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ========== to ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this, we can now do Scavenges if the amount of memory retained from new space is too large. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this, we can now do Scavenges if the amount of memory retained from new space is too large. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org ========== to ========== [heap] Improve size profiling for ArrayBuffer tracking Eagerly account for retained sizes during ArrayBuffer tracking. Following up on this, we can now do Scavenges if the amount of memory retained from new space is too large. BUG=chromium:621829 R=jochen@chromium.org,hpayer@chromium.org Committed: https://crrev.com/28e13bd6a75c9467dae43043e7b741a1387d5252 Cr-Commit-Position: refs/heads/master@{#38731} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/28e13bd6a75c9467dae43043e7b741a1387d5252 Cr-Commit-Position: refs/heads/master@{#38731}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2261513003/ by mlippautz@chromium.org. The reason for reverting is: Tanks octane. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
