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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 235043002: TimelinePanel: make GC events synchronous. (Closed)

Created:
6 years, 8 months ago by loislo
Modified:
6 years, 8 months ago
Reviewers:
caseq, pfeldman, yurys
CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org
Visibility:
Public.

Description

we don't do actions that could modify v8 heap so we can report GC events synchronously. Otherwise these GC events would get call stacks of the next event. We will not collect call stacks for gc event at all because it could be triggered even a single small allocation and the call stack will mislead the developer. If someone would like to know what code allocates memory then he needs to use our allocation profiler with allocation stacks. BUG=361045 R=yurys@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171460

Patch Set 1 #

Patch Set 2 : collect callstack for gc event. #

Patch Set 3 : test was added #

Patch Set 4 : and expectations #

Total comments: 2

Patch Set 5 : comments addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -35 lines) Patch
M LayoutTests/http/tests/inspector/timeline-test.js View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/inspector/timeline/timeline-gc-event.html View 1 2 3 4 1 chunk +42 lines, -0 lines 0 comments Download
A LayoutTests/inspector/timeline/timeline-gc-event-expected.txt View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
M Source/core/inspector/InspectorTimelineAgent.h View 3 chunks +0 lines, -5 lines 0 comments Download
M Source/core/inspector/InspectorTimelineAgent.cpp View 2 3 4 6 chunks +7 lines, -30 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
loislo
6 years, 8 months ago (2014-04-11 12:12:55 UTC) #1
yurys
Can you add a test for this?
6 years, 8 months ago (2014-04-11 12:21:38 UTC) #2
loislo
On 2014/04/11 12:21:38, yurys wrote: > Can you add a test for this? done
6 years, 8 months ago (2014-04-11 13:38:00 UTC) #3
yurys
https://codereview.chromium.org/235043002/diff/60001/LayoutTests/inspector/timeline/timeline-gc-event.html File LayoutTests/inspector/timeline/timeline-gc-event.html (right): https://codereview.chromium.org/235043002/diff/60001/LayoutTests/inspector/timeline/timeline-gc-event.html#newcode15 LayoutTests/inspector/timeline/timeline-gc-event.html:15: if (window.testRunner) maybe call testRunner.gc() instead? https://codereview.chromium.org/235043002/diff/60001/LayoutTests/inspector/timeline/timeline-gc-event.html#newcode27 LayoutTests/inspector/timeline/timeline-gc-event.html:27: if ...
6 years, 8 months ago (2014-04-11 13:46:03 UTC) #4
loislo
On 2014/04/11 13:46:03, yurys wrote: > https://codereview.chromium.org/235043002/diff/60001/LayoutTests/inspector/timeline/timeline-gc-event.html > File LayoutTests/inspector/timeline/timeline-gc-event.html (right): > > https://codereview.chromium.org/235043002/diff/60001/LayoutTests/inspector/timeline/timeline-gc-event.html#newcode15 > ...
6 years, 8 months ago (2014-04-14 10:02:49 UTC) #5
yurys
lgtm Please update the description with a notion that now GC event doesn't include call ...
6 years, 8 months ago (2014-04-14 10:22:27 UTC) #6
loislo
6 years, 8 months ago (2014-04-14 13:08:42 UTC) #7
Message was sent while issue was closed. 
Committed patchset #5 manually as r171460 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698