Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 2820433005: memory-infra: Start disentangling tracing from memory-infra (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by hjd
Modified:
6 months ago
CC:
chromium-reviews, Dirk Pranke, chrome-grc-reviews_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, jam, darin-cc_chromium.org, agrieve+watch_chromium.org, tracing+reviews_chromium.org, danakj+watch_chromium.org, tfarina
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

memory-infra: Start disentangling tracing from memory-infra Starts refactoring the tracing specific code from MemoryDumpManager into a new class MemoryTracingObserver. This shouldn't change the behaviour of the MemoryDumpManager. BUG=703184 Review-Url: https://codereview.chromium.org/2820433005 Cr-Commit-Position: refs/heads/master@{#466936} Committed: https://chromium.googlesource.com/chromium/src/+/4a4589fc42e386ba9b2ad1c153d17dd8180941f8

Patch Set 1 #

Total comments: 33

Patch Set 2 : update for comments #

Patch Set 3 : pass config in #

Total comments: 37

Patch Set 4 : update for comments #

Total comments: 10

Patch Set 5 : address a few more comments #

Patch Set 6 : rebase mk2 #

Patch Set 7 : format #

Patch Set 8 : add todo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+207 lines, -122 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M base/trace_event/memory_dump_manager.h View 1 2 3 4 5 5 chunks +15 lines, -6 lines 0 comments Download
M base/trace_event/memory_dump_manager.cc View 1 2 3 4 5 6 7 11 chunks +17 lines, -58 lines 0 comments Download
M base/trace_event/memory_dump_manager_unittest.cc View 1 2 3 4 5 2 chunks +31 lines, -58 lines 0 comments Download
A base/trace_event/memory_tracing_observer.h View 1 2 3 4 1 chunk +45 lines, -0 lines 0 comments Download
A base/trace_event/memory_tracing_observer.cc View 1 2 3 4 1 chunk +96 lines, -0 lines 0 comments Download
M tools/gn/bootstrap/bootstrap.py View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 38 (14 generated)
hjd
early preview of disentangling tracing from memory-infra, comments welcome https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dump_manager.cc File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dump_manager.cc#oldcode801 base/trace_event/memory_dump_manager.cc:801: ...
6 months, 1 week ago (2017-04-13 14:17:00 UTC) #2
Primiano Tucci (use gerrit)
Took an initial quick look (sorry sheriffing today) and added some comments. Overall makes lot ...
6 months, 1 week ago (2017-04-13 15:22:37 UTC) #3
Primiano Tucci (use gerrit)
for the TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID I think what you really need is some sort of callback or ...
6 months, 1 week ago (2017-04-13 15:29:56 UTC) #4
hjd
On 2017/04/13 15:29:56, Primiano Tucci wrote: > for the TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID I think what you really ...
6 months, 1 week ago (2017-04-13 16:15:04 UTC) #5
hjd
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dump_manager.h File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dump_manager.h#newcode123 base/trace_event/memory_dump_manager.h:123: virtual void Enable(); On 2017/04/13 15:22:37, Primiano Tucci wrote: ...
6 months, 1 week ago (2017-04-13 16:32:18 UTC) #6
ssid
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dump_manager.cc File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dump_manager.cc#oldcode801 base/trace_event/memory_dump_manager.cc:801: } On 2017/04/13 14:16:59, hjd wrote: > Can we ...
6 months, 1 week ago (2017-04-14 23:33:09 UTC) #7
hjd
I've updated the patch with some changes, ideally we would like TracingMemoryObserver to depend on ...
6 months ago (2017-04-19 09:54:42 UTC) #8
hjd
I've updated the patch with some changes, ideally we would like TracingMemoryObserver to depend on ...
6 months ago (2017-04-19 09:54:56 UTC) #9
Primiano Tucci (use gerrit)
On 2017/04/19 09:54:42, hjd wrote: > I've updated the patch with some changes, ideally we ...
6 months ago (2017-04-19 11:56:09 UTC) #10
hjd
ptal, updated to pass config in and remove unittest
6 months ago (2017-04-20 12:50:07 UTC) #11
Primiano Tucci (use gerrit)
Looks good, some comments, nothing major really. The only major comment is: so far the ...
6 months ago (2017-04-21 10:10:27 UTC) #12
hjd
Thanks! :) https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory_dump_manager.cc File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory_dump_manager.cc#newcode739 base/trace_event/memory_dump_manager.cc:739: MemoryTracingObserver::MaybeAddDumpToTrace(&pmd_async_state->req_args, pid, On 2017/04/21 10:10:26, Primiano Tucci ...
6 months ago (2017-04-21 11:51:58 UTC) #13
Primiano Tucci (use gerrit)
LGTM with some final comments. Also plz expand a little bit the CL description. I'd ...
6 months ago (2017-04-21 13:14:17 UTC) #14
hjd
https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory_dump_manager.cc File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory_dump_manager.cc#newcode743 base/trace_event/memory_dump_manager.cc:743: // A dump only counts as successful if we ...
6 months ago (2017-04-21 13:42:32 UTC) #17
hjd
+thakis for two line change in base/BUILD.gn Thanks!
6 months ago (2017-04-21 13:52:14 UTC) #19
Nico
base/build.gn lgtm
6 months ago (2017-04-21 14:07:50 UTC) #20
ssid
On 2017/04/19 11:56:09, Primiano Tucci wrote: > On 2017/04/19 09:54:42, hjd wrote: > > I've ...
6 months ago (2017-04-21 19:09:11 UTC) #21
hjd
On 2017/04/21 19:09:11, ssid wrote: > On 2017/04/19 11:56:09, Primiano Tucci wrote: > > On ...
6 months ago (2017-04-24 11:55:26 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820433005/100001
6 months ago (2017-04-24 11:56:28 UTC) #25
Primiano Tucci (use gerrit)
On 2017/04/24 11:55:26, hjd wrote: > however I think at the moment there are some ...
6 months ago (2017-04-24 12:19:33 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820433005/160001
6 months ago (2017-04-24 12:22:08 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/437387)
6 months ago (2017-04-24 15:13:15 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2820433005/160001
6 months ago (2017-04-25 09:23:58 UTC) #35
commit-bot: I haz the power
6 months ago (2017-04-25 10:02:41 UTC) #38
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/4a4589fc42e386ba9b2ad1c153d1...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa