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

Issue 639818 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 640524

Blocking:
issue 468240



Sign in to add a comment

GC tracer scopes not working within incremental marking

Project Member Reported by mlippautz@chromium.org, Aug 22 2016

Issue description

Within incremental marking we cannot use GC tracer scopes as all scopes are reset during a start/stop cycle.

This already shows up with all external scopes being 0 within incremental marking finalization. Also, recorded finalization steps are out of sync wrt. incremental steps, i.e., finalization metrics are reset every cycle while regular incremental metrics consider the last GC event.

This blocks properly profiling object grouping in the finalization of incremental marking.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 23 2016

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

commit 300a8f97472b88ff2f94eb977c36b4bf1bedabf1
Author: mlippautz <mlippautz@chromium.org>
Date: Tue Aug 23 13:25:27 2016

[heap] Tracer: Handle incremental marking scopes

Before this patch all tracing scopes in incremental marking would be reset
during a gc tracer start/stop cycle. This patch handles scopes the same way it
does other incremental marking metrics.

Also:
- Align finalization metric with regular marking metric.
- Smaller cleanups

BUG= chromium:639818 
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2264033002
Cr-Commit-Position: refs/heads/master@{#38822}

[modify] https://crrev.com/300a8f97472b88ff2f94eb977c36b4bf1bedabf1/src/heap/gc-tracer.cc
[modify] https://crrev.com/300a8f97472b88ff2f94eb977c36b4bf1bedabf1/src/heap/gc-tracer.h
[modify] https://crrev.com/300a8f97472b88ff2f94eb977c36b4bf1bedabf1/src/heap/incremental-marking.cc
[modify] https://crrev.com/300a8f97472b88ff2f94eb977c36b4bf1bedabf1/src/heap/mark-compact.cc
[modify] https://crrev.com/300a8f97472b88ff2f94eb977c36b4bf1bedabf1/test/cctest/heap/test-heap.cc
[modify] https://crrev.com/300a8f97472b88ff2f94eb977c36b4bf1bedabf1/test/unittests/heap/gc-tracer-unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 23 2016

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

commit 0f4f30a1d2243d73673dab3319b17d0ba421f187
Author: mlippautz <mlippautz@chromium.org>
Date: Tue Aug 23 14:15:07 2016

Revert of [heap] Tracer: Handle incremental marking scopes (patchset #4 id:100001 of https://codereview.chromium.org/2264033002/ )

Reason for revert:
Unittest fails on win32 debug:
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds/4188/steps/Check/logs/GCTracerTest.Incremen..

Original issue's description:
> [heap] Tracer: Handle incremental marking scopes
>
> Before this patch all tracing scopes in incremental marking would be reset
> during a gc tracer start/stop cycle. This patch handles scopes the same way it
> does other incremental marking metrics.
>
> Also:
> - Align finalization metric with regular marking metric.
> - Smaller cleanups
>
> BUG= chromium:639818 
> R=jochen@chromium.org
>
> Committed: https://crrev.com/300a8f97472b88ff2f94eb977c36b4bf1bedabf1
> Cr-Commit-Position: refs/heads/master@{#38822}

TBR=jochen@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:639818 

Review-Url: https://codereview.chromium.org/2275583002
Cr-Commit-Position: refs/heads/master@{#38827}

[modify] https://crrev.com/0f4f30a1d2243d73673dab3319b17d0ba421f187/src/heap/gc-tracer.cc
[modify] https://crrev.com/0f4f30a1d2243d73673dab3319b17d0ba421f187/src/heap/gc-tracer.h
[modify] https://crrev.com/0f4f30a1d2243d73673dab3319b17d0ba421f187/src/heap/incremental-marking.cc
[modify] https://crrev.com/0f4f30a1d2243d73673dab3319b17d0ba421f187/src/heap/mark-compact.cc
[modify] https://crrev.com/0f4f30a1d2243d73673dab3319b17d0ba421f187/test/cctest/heap/test-heap.cc
[modify] https://crrev.com/0f4f30a1d2243d73673dab3319b17d0ba421f187/test/unittests/heap/gc-tracer-unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 23 2016

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

commit 7695642e2c7d893d300920ac0084905288f72859
Author: mlippautz <mlippautz@chromium.org>
Date: Tue Aug 23 15:13:01 2016

[heap] Tracer: Handle incremental marking scopes

Before this patch all tracing scopes in incremental marking would be reset
during a gc tracer start/stop cycle. This patch handles scopes the same way it
does other incremental marking metrics.

Also:
- Align finalization metric with regular marking metric.
- Smaller cleanups

BUG= chromium:639818 

Review-Url: https://codereview.chromium.org/2273673002
Cr-Commit-Position: refs/heads/master@{#38834}

[modify] https://crrev.com/7695642e2c7d893d300920ac0084905288f72859/src/heap/gc-tracer.cc
[modify] https://crrev.com/7695642e2c7d893d300920ac0084905288f72859/src/heap/gc-tracer.h
[modify] https://crrev.com/7695642e2c7d893d300920ac0084905288f72859/src/heap/incremental-marking.cc
[modify] https://crrev.com/7695642e2c7d893d300920ac0084905288f72859/src/heap/mark-compact.cc
[modify] https://crrev.com/7695642e2c7d893d300920ac0084905288f72859/test/cctest/heap/test-heap.cc
[modify] https://crrev.com/7695642e2c7d893d300920ac0084905288f72859/test/unittests/heap/gc-tracer-unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 25 2016

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

commit 3866975f1b159f3703429ce50ca228bcdbf28306
Author: mlippautz <mlippautz@chromium.org>
Date: Thu Aug 25 14:21:59 2016

[heap] GCTracer: Record details for incremental marking

Record details, such as cumulative duration, number of steps, and longest steps
in IncrementalMarkingDetails which get populated at a single callsite
(AddScopeSample). Remove member fields that thus become obsolete (unfortunately
not all of them).

Additional remove some dead code and refactor printing. Printing in a single
statement allows for using logcat on Android.

This should also address the regression in chromium:640524.

BUG= chromium:639818 ,chromium:640524
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2269093002
Cr-Commit-Position: refs/heads/master@{#38912}

[modify] https://crrev.com/3866975f1b159f3703429ce50ca228bcdbf28306/src/heap/gc-tracer.cc
[modify] https://crrev.com/3866975f1b159f3703429ce50ca228bcdbf28306/src/heap/gc-tracer.h
[modify] https://crrev.com/3866975f1b159f3703429ce50ca228bcdbf28306/src/heap/incremental-marking.cc
[modify] https://crrev.com/3866975f1b159f3703429ce50ca228bcdbf28306/test/unittests/heap/gc-tracer-unittest.cc

Status: Fixed (was: Started)
Scopes now work within incremental marking (steps). For those particular scopes we also record the number of steps and their longest step, both of which can be useful for checking using --trace-gc-nvp.
Blockedon: 640524
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 26 2016

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

commit a4a4e7fa97ef6824ea72260201f71e058672433c
Author: mlippautz <mlippautz@chromium.org>
Date: Fri Aug 26 08:10:13 2016

[heap] GCTracer: Properly reset all members for unittests

BUG= chromium:639818 
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2279193002
Cr-Commit-Position: refs/heads/master@{#38930}

[modify] https://crrev.com/a4a4e7fa97ef6824ea72260201f71e058672433c/src/heap/gc-tracer.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 29 2016

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

commit 33ffcc8f819d0cf8f86eabae6cd1860c693a4672
Author: mlippautz <mlippautz@chromium.org>
Date: Mon Aug 29 09:32:30 2016

[heap] GCTracer: Fix UB when iterating incremental scopes

BUG= chromium:639818 
R=ulan@chromium.org

Review-Url: https://codereview.chromium.org/2288693002
Cr-Commit-Position: refs/heads/master@{#38971}

[modify] https://crrev.com/33ffcc8f819d0cf8f86eabae6cd1860c693a4672/src/heap/gc-tracer.cc

Labels: Merge-Request-54

Comment 10 by dimu@chromium.org, Sep 8 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 8 2016

Labels: merge-merged-5.4
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/bb1f4cc14db88b70cae3b317cd26a573439cfd9f

commit bb1f4cc14db88b70cae3b317cd26a573439cfd9f
Author: mlippautz <mlippautz@chromium.org>
Date: Thu Sep 08 11:00:13 2016

Merged: Squashed multiple commits.

Merged: [heap] GCTracer: Record details for incremental marking
Revision: 3866975f1b159f3703429ce50ca228bcdbf28306

Merged: [heap] GCTracer: Properly reset all members for unittests
Revision: a4a4e7fa97ef6824ea72260201f71e058672433c

Merged: [heap] GCTracer: Fix UB when iterating incremental scopes
Revision: 33ffcc8f819d0cf8f86eabae6cd1860c693a4672

BUG= chromium:639818 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=hablich@chromium.org

Review-Url: https://codereview.chromium.org/2324493003
Cr-Commit-Position: refs/branch-heads/5.4@{#35}
Cr-Branched-From: 5ce282769772d94937eb2cb88eb419a6890c8b2d-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49d7b47b40a2d6f74d04d1b76ceae2a0253-refs/heads/master@{#38841}

[modify] https://crrev.com/bb1f4cc14db88b70cae3b317cd26a573439cfd9f/src/heap/gc-tracer.cc
[modify] https://crrev.com/bb1f4cc14db88b70cae3b317cd26a573439cfd9f/src/heap/gc-tracer.h
[modify] https://crrev.com/bb1f4cc14db88b70cae3b317cd26a573439cfd9f/src/heap/incremental-marking.cc
[modify] https://crrev.com/bb1f4cc14db88b70cae3b317cd26a573439cfd9f/test/unittests/heap/gc-tracer-unittest.cc

Labels: -Merge-Approved-54

Sign in to add a comment