New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 775896 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

ArrayBufferTracker should only consider committed size

Project Member Reported by mlippautz@chromium.org, Oct 18 2017

Issue description

All heuristics are based on committed sizes and we have to avoid passing reservation sizes to the external memory tracker.

Furthermore, byteLength might be a HeapNumber and we are thus only allowed to read them on live objects after pointer updating.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6488c9e5a62c2640da09a9545c6dca8b127a83de

commit 6488c9e5a62c2640da09a9545c6dca8b127a83de
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Wed Oct 18 18:12:07 2017

[heap] ArrayBufferTracker: Only consider committed size

- Only consider commited size of ABs.
- Compute freed memory from retained sizes byte length might be a
  HeapNumber and thus prohibited from accessing (as it may be already
  collected).

CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Bug:  chromium:775896 
Change-Id: Ia0bed66afac5e4d5ed58194950a55156e19cec72
Reviewed-on: https://chromium-review.googlesource.com/725722
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48699}
[modify] https://crrev.com/6488c9e5a62c2640da09a9545c6dca8b127a83de/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/6488c9e5a62c2640da09a9545c6dca8b127a83de/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/6488c9e5a62c2640da09a9545c6dca8b127a83de/src/heap/mark-compact.cc
[modify] https://crrev.com/6488c9e5a62c2640da09a9545c6dca8b127a83de/src/heap/mark-compact.h

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d10b621895a14491142524fd05430b0480e3cc92

commit d10b621895a14491142524fd05430b0480e3cc92
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Wed Oct 18 18:34:10 2017

Revert "[heap] ArrayBufferTracker: Only consider committed size"

This reverts commit 6488c9e5a62c2640da09a9545c6dca8b127a83de.

Reason for revert: wasm grow memory test fails; requires investigation.

https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20shared/builds/20548/steps/Check/logs/grow-memory

Original change's description:
> [heap] ArrayBufferTracker: Only consider committed size
> 
> - Only consider commited size of ABs.
> - Compute freed memory from retained sizes byte length might be a
>   HeapNumber and thus prohibited from accessing (as it may be already
>   collected).
> 
> CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> 
> Bug:  chromium:775896 
> Change-Id: Ia0bed66afac5e4d5ed58194950a55156e19cec72
> Reviewed-on: https://chromium-review.googlesource.com/725722
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#48699}

TBR=ulan@chromium.org,mlippautz@chromium.org

Change-Id: I0f8b28b876b09f149ff330e532e57cf1871e3961
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:775896 
Cq-Include-Trybots: master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/726440
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48700}
[modify] https://crrev.com/d10b621895a14491142524fd05430b0480e3cc92/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/d10b621895a14491142524fd05430b0480e3cc92/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/d10b621895a14491142524fd05430b0480e3cc92/src/heap/mark-compact.cc
[modify] https://crrev.com/d10b621895a14491142524fd05430b0480e3cc92/src/heap/mark-compact.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/46f9d5a2548a5d239ca1b8c7c233f8892456bede

commit 46f9d5a2548a5d239ca1b8c7c233f8892456bede
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Wed Oct 18 20:04:31 2017

Reland "[heap] ArrayBufferTracker: Only consider committed size"

This is a reland of 6488c9e5a62c2640da09a9545c6dca8b127a83de
Original change's description:
> [heap] ArrayBufferTracker: Only consider committed size
> 
> - Only consider commited size of ABs.
> - Compute freed memory from retained sizes byte length might be a
>   HeapNumber and thus prohibited from accessing (as it may be already
>   collected).
> 
> CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> 
> Bug:  chromium:775896 
> Change-Id: Ia0bed66afac5e4d5ed58194950a55156e19cec72
> Reviewed-on: https://chromium-review.googlesource.com/725722
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#48699}

Tbr: ulan@chromium.org
Bug:  chromium:775896 
Change-Id: Ibbec1ffa8fe90d3668f0fe0c1b8b9997b5fd644e
Cq-Include-Trybots: master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/726579
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48707}
[modify] https://crrev.com/46f9d5a2548a5d239ca1b8c7c233f8892456bede/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/46f9d5a2548a5d239ca1b8c7c233f8892456bede/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/46f9d5a2548a5d239ca1b8c7c233f8892456bede/src/heap/mark-compact.cc
[modify] https://crrev.com/46f9d5a2548a5d239ca1b8c7c233f8892456bede/src/heap/mark-compact.h

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e1e5f6cf97e865f4f3e01f5131dc106564c1f779

commit e1e5f6cf97e865f4f3e01f5131dc106564c1f779
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Wed Oct 18 21:16:27 2017

Revert "Reland "[heap] ArrayBufferTracker: Only consider committed size""

This reverts commit 46f9d5a2548a5d239ca1b8c7c233f8892456bede.

Reason for revert: Aborted compaction pages require separate handling now that we consider byteLength which is a Number.

Original change's description:
> Reland "[heap] ArrayBufferTracker: Only consider committed size"
> 
> This is a reland of 6488c9e5a62c2640da09a9545c6dca8b127a83de
> Original change's description:
> > [heap] ArrayBufferTracker: Only consider committed size
> > 
> > - Only consider commited size of ABs.
> > - Compute freed memory from retained sizes byte length might be a
> >   HeapNumber and thus prohibited from accessing (as it may be already
> >   collected).
> > 
> > CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> > 
> > Bug:  chromium:775896 
> > Change-Id: Ia0bed66afac5e4d5ed58194950a55156e19cec72
> > Reviewed-on: https://chromium-review.googlesource.com/725722
> > Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> > Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#48699}
> 
> Tbr: ulan@chromium.org
> Bug:  chromium:775896 
> Change-Id: Ibbec1ffa8fe90d3668f0fe0c1b8b9997b5fd644e
> Cq-Include-Trybots: master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> Reviewed-on: https://chromium-review.googlesource.com/726579
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#48707}

TBR=ulan@chromium.org,mlippautz@chromium.org

Change-Id: If678ad73326ceb24e85f3a7bf6350df05991005f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:775896 
Cq-Include-Trybots: master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/726799
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48708}
[modify] https://crrev.com/e1e5f6cf97e865f4f3e01f5131dc106564c1f779/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/e1e5f6cf97e865f4f3e01f5131dc106564c1f779/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/e1e5f6cf97e865f4f3e01f5131dc106564c1f779/src/heap/mark-compact.cc
[modify] https://crrev.com/e1e5f6cf97e865f4f3e01f5131dc106564c1f779/src/heap/mark-compact.h

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/da9691deeafd19a2b6769c4e608dfcc1c02883b4

commit da9691deeafd19a2b6769c4e608dfcc1c02883b4
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Thu Oct 19 15:03:34 2017

Reland "Reland "[heap] ArrayBufferTracker: Only consider committed size""

Includes the fix for aborted compaction pages that now require processing 
with all other ArrayBufferTrackers because the considered length (byteLength)
may be a HeapNumber allocated on a compacted page.

This is a reland of 46f9d5a2548a5d239ca1b8c7c233f8892456bede
Original change's description:
> Reland "[heap] ArrayBufferTracker: Only consider committed size"
> 
> This is a reland of 6488c9e5a62c2640da09a9545c6dca8b127a83de
> Original change's description:
> > [heap] ArrayBufferTracker: Only consider committed size
> > 
> > - Only consider commited size of ABs.
> > - Compute freed memory from retained sizes byte length might be a
> >   HeapNumber and thus prohibited from accessing (as it may be already
> >   collected).
> > 
> > CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> > 
> > Bug:  chromium:775896 
> > Change-Id: Ia0bed66afac5e4d5ed58194950a55156e19cec72
> > Reviewed-on: https://chromium-review.googlesource.com/725722
> > Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> > Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#48699}
> 
> Bug:  chromium:775896 
> Change-Id: Ibbec1ffa8fe90d3668f0fe0c1b8b9997b5fd644e
> Cq-Include-Trybots: master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
> Reviewed-on: https://chromium-review.googlesource.com/726579
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#48707}

Bug:  chromium:775896 
Change-Id: I9b7b2ae865ef6cdb25692abb65108df5c2ecc157
Cq-Include-Trybots: master.tryserver.v8:v8_linux64_tsan_rel;master.tryserver.v8:v8_linux64_tsan_concurrent_marking_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/726800
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48753}
[modify] https://crrev.com/da9691deeafd19a2b6769c4e608dfcc1c02883b4/src/heap/array-buffer-tracker-inl.h
[modify] https://crrev.com/da9691deeafd19a2b6769c4e608dfcc1c02883b4/src/heap/array-buffer-tracker.cc
[modify] https://crrev.com/da9691deeafd19a2b6769c4e608dfcc1c02883b4/src/heap/mark-compact.cc
[modify] https://crrev.com/da9691deeafd19a2b6769c4e608dfcc1c02883b4/src/heap/mark-compact.h

Status: Fixed (was: Started)
This is done. There is no need for a backmerge as the feature that would make the sizes diverge (trap handlers) is currently disabled.

Sign in to add a comment