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

Issue 2694083005: memory-infra: Finish moving memory_infra from TracingController (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by chiniforooshan
Modified:
7 months, 4 weeks ago
CC:
chromium-reviews, fmeawad
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

memory-infra: Finish moving memory_infra from TracingController This finishes moving memory instrumentation code from TracingController to its own location in //services/resource_coordinator. BUG=679830 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2694083005 Cr-Commit-Position: refs/heads/master@{#453243} Committed: https://chromium.googlesource.com/chromium/src/+/e7bee2f02da7fb7afbdb387197ad9461d8d2eba2

Patch Set 1 #

Patch Set 2 : Add back the process metrics memory dump provider #

Patch Set 3 : Rebase #

Patch Set 4 : Correct rebase #

Patch Set 5 : nit #

Patch Set 6 : DCHECK bug #

Patch Set 7 : Expose interface early and then initialize coordinator #

Patch Set 8 : Fix browsertests #

Patch Set 9 : Fix threading when binding locally #

Patch Set 10 : . #

Patch Set 11 : . #

Patch Set 12 : . #

Patch Set 13 : . #

Patch Set 14 : . #

Total comments: 17

Patch Set 15 : review #

Patch Set 16 : review #

Total comments: 16

Patch Set 17 : review #

Patch Set 18 : sync #

Patch Set 19 : nit #

Patch Set 20 : nit #

Patch Set 21 : nit #

Patch Set 22 : Deleted task runner initialization by mistak; add it back #

Total comments: 6

Patch Set 23 : Don't throttle on browser process #

Total comments: 8

Patch Set 24 : review #

Patch Set 25 : style: auto must not deduce to raw pointer #

Unified diffs Side-by-side diffs Delta from patch set Stats (+417 lines, -934 lines) Patch
M base/trace_event/memory_dump_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +15 lines, -11 lines 0 comments Download
M base/trace_event/memory_dump_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +3 lines, -21 lines 0 comments Download
M base/trace_event/memory_dump_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 6 chunks +10 lines, -10 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 3 chunks +17 lines, -0 lines 0 comments Download
M components/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -1 line 0 comments Download
M components/tracing/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -2 lines 0 comments Download
D components/tracing/child/child_memory_dump_manager_delegate_impl.h View 1 chunk +0 lines, -85 lines 0 comments Download
D components/tracing/child/child_memory_dump_manager_delegate_impl.cc View 1 chunk +0 lines, -114 lines 0 comments Download
M components/tracing/child/child_trace_message_filter.h View 4 chunks +0 lines, -17 lines 0 comments Download
M components/tracing/child/child_trace_message_filter.cc View 1 7 chunks +14 lines, -63 lines 0 comments Download
D components/tracing/child/child_trace_message_filter_browsertest.cc View 1 chunk +0 lines, -326 lines 0 comments Download
M content/browser/browser_main.cc View 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/tracing/memory_tracing_browsertest.cc View 1 2 3 4 5 6 7 8 23 2 chunks +4 lines, -0 lines 0 comments Download
M content/browser/tracing/trace_message_filter.h View 3 chunks +0 lines, -8 lines 0 comments Download
M content/browser/tracing/trace_message_filter.cc View 3 chunks +0 lines, -29 lines 0 comments Download
M content/browser/tracing/tracing_controller_impl.h View 6 chunks +0 lines, -33 lines 0 comments Download
M content/browser/tracing/tracing_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +0 lines, -172 lines 0 comments Download
M content/child/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -0 lines 0 comments Download
M content/child/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M content/child/child_thread_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +5 lines, -0 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +4 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -0 lines 0 comments Download
M services/resource_coordinator/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +4 lines, -0 lines 0 comments Download
M services/resource_coordinator/memory/coordinator/coordinator_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +10 lines, -4 lines 0 comments Download
M services/resource_coordinator/memory/coordinator/coordinator_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 5 chunks +33 lines, -12 lines 0 comments Download
M services/resource_coordinator/memory/coordinator/coordinator_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 7 chunks +16 lines, -10 lines 0 comments Download
M services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +34 lines, -5 lines 0 comments Download
M services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +100 lines, -10 lines 0 comments Download
A services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +139 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 90 (60 generated)
chiniforooshan
PTAL
8 months, 1 week ago (2017-02-14 21:53:54 UTC) #11
chiniforooshan
OK, tests pass know. PTAL :)
8 months, 1 week ago (2017-02-16 14:08:30 UTC) #42
oystein (OOO til 10th of July)
https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memory_dump_manager.h File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memory_dump_manager.h#newcode132 base/trace_event/memory_dump_manager.h:132: uint64_t tracing_process_id() const; You can just inline these two. ...
8 months, 1 week ago (2017-02-16 20:50:01 UTC) #43
chiniforooshan
https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memory_dump_manager.h File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memory_dump_manager.h#newcode132 base/trace_event/memory_dump_manager.h:132: uint64_t tracing_process_id() const; On 2017/02/16 20:50:00, oystein wrote: > ...
8 months, 1 week ago (2017-02-16 22:54:15 UTC) #44
oystein (OOO til 10th of July)
lgtm w/ comments but wait for primiano to look at the memory_infra specific bits. https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memory_dump_manager_unittest.cc ...
8 months, 1 week ago (2017-02-16 23:19:49 UTC) #45
Primiano Tucci (use gerrit)
I'll get to this very soon (O(Hours)) sorry for the delay, bad week. in the ...
8 months, 1 week ago (2017-02-17 16:22:46 UTC) #47
chiniforooshan
Thanks. https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memory_dump_manager_unittest.cc File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2694083005/diff/260001/base/trace_event/memory_dump_manager_unittest.cc#newcode227 base/trace_event/memory_dump_manager_unittest.cc:227: delegate_ = delegate_for_mdm_.get(); On 2017/02/16 23:19:49, oystein wrote: ...
8 months, 1 week ago (2017-02-17 17:20:42 UTC) #48
Primiano Tucci (use gerrit)
LGTM with some comments, plz look at them before landing. My only concern is that ...
8 months, 1 week ago (2017-02-17 19:08:44 UTC) #49
chiniforooshan
Thanks. https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memory_dump_manager.cc File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memory_dump_manager.cc#newcode219 base/trace_event/memory_dump_manager.cc:219: delegate_.swap(delegate); On 2017/02/17 19:08:44, Primiano Tucci wrote: > ...
8 months ago (2017-02-22 05:16:34 UTC) #50
chiniforooshan
+kenrb +jochen
8 months ago (2017-02-22 05:17:38 UTC) #52
jochen (gone - plz use gerrit)
which files do you want me to look at?
8 months ago (2017-02-22 09:03:50 UTC) #53
chiniforooshan
On 2017/02/22 09:03:50, jochen wrote: > which files do you want me to look at? ...
8 months ago (2017-02-22 14:34:45 UTC) #54
chiniforooshan
On 2017/02/22 14:34:45, chiniforooshan wrote: > On 2017/02/22 09:03:50, jochen wrote: > > which files ...
8 months ago (2017-02-22 20:25:00 UTC) #59
chiniforooshan
On 2017/02/22 20:25:00, chiniforooshan wrote: > On 2017/02/22 14:34:45, chiniforooshan wrote: > > On 2017/02/22 ...
8 months ago (2017-02-22 22:12:38 UTC) #61
ssid
https://codereview.chromium.org/2694083005/diff/420001/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc File services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc (right): https://codereview.chromium.org/2694083005/diff/420001/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc#newcode98 services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:98: callback.Run(args.dump_guid, false); Sorry if I am mis-remembering something. But, ...
8 months ago (2017-02-23 20:21:50 UTC) #63
ssid
On 2017/02/22 22:12:38, chiniforooshan wrote: > On 2017/02/22 20:25:00, chiniforooshan wrote: > > On 2017/02/22 ...
8 months ago (2017-02-23 20:25:22 UTC) #64
chiniforooshan
On 2017/02/23 20:25:22, ssid wrote: > On 2017/02/22 22:12:38, chiniforooshan wrote: > > On 2017/02/22 ...
8 months ago (2017-02-23 21:14:40 UTC) #65
ssid
On 2017/02/23 21:14:40, chiniforooshan wrote: > On 2017/02/23 20:25:22, ssid wrote: > > On 2017/02/22 ...
8 months ago (2017-02-24 02:17:01 UTC) #66
chiniforooshan
Thanks! https://codereview.chromium.org/2694083005/diff/420001/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc File services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc (right): https://codereview.chromium.org/2694083005/diff/420001/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc#newcode98 services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:98: callback.Run(args.dump_guid, false); On 2017/02/23 20:21:50, ssid wrote: > ...
8 months ago (2017-02-24 15:51:03 UTC) #67
chiniforooshan
Adding jam@ and kenrb@, now that tests pass. jam@ could you please review: - content/child/child_thread_impl.cc ...
8 months ago (2017-02-24 18:00:07 UTC) #73
kenrb
lgtm
8 months ago (2017-02-24 21:31:48 UTC) #74
ssid
Thanks for the TODO. Just have few questions about having task_runner and is_coordinator_ in the ...
8 months ago (2017-02-24 23:19:05 UTC) #75
jam
lgtm
8 months ago (2017-02-24 23:54:37 UTC) #76
chiniforooshan
https://codereview.chromium.org/2694083005/diff/440001/content/browser/tracing/memory_tracing_browsertest.cc File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2694083005/diff/440001/content/browser/tracing/memory_tracing_browsertest.cc#newcode49 content/browser/tracing/memory_tracing_browsertest.cc:49: LOG(ERROR) << "MDM::RequestGlobalDump"; On 2017/02/24 23:19:05, ssid wrote: > ...
8 months ago (2017-02-25 00:04:51 UTC) #77
ssid
Lg https://codereview.chromium.org/2694083005/diff/440001/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc File services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc (right): https://codereview.chromium.org/2694083005/diff/440001/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc#newcode95 services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc:95: // TODO(chiniforooshan): This condition is here to match ...
8 months ago (2017-02-25 16:07:21 UTC) #78
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/2694083005/460001
7 months, 4 weeks ago (2017-02-27 15:40:28 UTC) #81
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/318777)
7 months, 4 weeks ago (2017-02-27 15:54:38 UTC) #83
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/2694083005/470001
7 months, 4 weeks ago (2017-02-27 16:17:08 UTC) #86
commit-bot: I haz the power
Committed patchset #25 (id:470001) as https://chromium.googlesource.com/chromium/src/+/e7bee2f02da7fb7afbdb387197ad9461d8d2eba2
7 months, 4 weeks ago (2017-02-27 17:35:37 UTC) #89
Primiano Tucci (use gerrit)
7 months, 3 weeks ago (2017-03-01 11:33:38 UTC) #90
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:470001) has been created in
https://codereview.chromium.org/2724793002/ by primiano@chromium.org.

The reason for reverting is: Unfortunately this broke lot of memory-infra
metrics and caused flakiness in various bot.
Have to revert because this is causing problems to the sheriff rotation. More
details:
crbug.com/697384 : metrics broken
crbug.com/697062 : memory dumps failing on webview.
Sign in to reply to this message.

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