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

Issue 2694083005: memory-infra: Finish moving memory_infra from TracingController

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 days, 7 hours ago by chiniforooshan
Modified:
2 days, 6 hours ago
Reviewers:
Primiano Tucci, oystein, hjd
CC:
chromium-reviews, ssid
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

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: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+180 lines, -941 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 5 chunks +10 lines, -13 lines 3 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 5 chunks +6 lines, -24 lines 1 comment 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 7 chunks +17 lines, -11 lines 0 comments Download
M cc/raster/staging_buffer_pool.cc View 1 chunk +1 line, -2 lines 0 comments Download
M cc/resources/resource_provider.cc View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 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 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 3 chunks +17 lines, -0 lines 1 comment 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/discardable_memory/common/discardable_shared_memory_heap.cc View 1 chunk +1 line, -2 lines 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 1 comment Download
D components/tracing/child/child_trace_message_filter_browsertest.cc View 1 chunk +0 lines, -326 lines 1 comment 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 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 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 2 chunks +7 lines, -0 lines 0 comments Download
M content/common/child_process_host_impl.h View 1 chunk +1 line, -2 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 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 1 chunk +1 line, -0 lines 0 comments Download
M gpu/command_buffer/client/cmd_buffer_helper.cc View 1 chunk +1 line, -2 lines 0 comments Download
M gpu/command_buffer/client/gles2_implementation.cc View 1 chunk +1 line, -2 lines 0 comments Download
M gpu/command_buffer/client/mapped_memory.cc View 1 chunk +1 line, -2 lines 0 comments Download
M media/video/gpu_memory_buffer_video_frame_pool.cc View 1 chunk +1 line, -2 lines 0 comments Download
M services/resource_coordinator/BUILD.gn View 1 2 1 chunk +1 line, -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 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 5 chunks +44 lines, -12 lines 1 comment 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 3 chunks +7 lines, -3 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 3 chunks +5 lines, -2 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 3 chunks +16 lines, -4 lines 0 comments Download
M services/ui/public/cpp/gpu/context_provider_command_buffer.cc View 1 chunk +1 line, -1 line 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 49 (41 generated)
chiniforooshan
PTAL
5 days, 3 hours ago (2017-02-14 21:53:54 UTC) #11
chiniforooshan
OK, tests pass know. PTAL :)
3 days, 11 hours ago (2017-02-16 14:08:30 UTC) #42
oystein
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. ...
3 days, 4 hours 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: > ...
3 days, 2 hours ago (2017-02-16 22:54:15 UTC) #44
oystein
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 ...
3 days, 2 hours ago (2017-02-16 23:19:49 UTC) #45
Primiano Tucci
I'll get to this very soon (O(Hours)) sorry for the delay, bad week. in the ...
2 days, 9 hours 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: ...
2 days, 8 hours ago (2017-02-17 17:20:42 UTC) #48
Primiano Tucci
2 days, 6 hours ago (2017-02-17 19:08:44 UTC) #49
LGTM with some comments, plz look at them before landing.
My only concern is that some test coverage seems to be  disappearing.
I don't care about the complex test that check the general interaction at the
IPC level, but I'd like the throttling and queueing mechanism to still be
covered, as that might regress without noticing and have very subtle side
effects on telemetry.

Also +ssid as he also has some patches

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:219: delegate_.swap(delegate);
As commented above I think this doesn't need to be  a unique_ptr, but in the
case it does, AFAIK the pattern here is delegate_ = std::move(delegate)

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
note this cl will very likely need to be rebased on top of ssid's
https://codereview.chromium.org/2582453002/

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:60: void
Initialize(std::unique_ptr<MemoryDumpManagerDelegate> delegate);
the MDM is a leaky singleton, passing ownership of an object to a leaky
singleton is IMHO a bit an obscure way to make the original object singleton in
the first place. can you just make the delegate leaky singleton?

https://codereview.chromium.org/2694083005/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:135: uint64_t tracing_process_id() const
{ return tracing_process_id_; }
Renaming this getter will require you a lot more owner dances and increase the
likeliness of merge conflicts along the road.
I'd honestly keep this called GetProcessId(), even if becomes an inliner getter,
and eventually renaming in a separate CL.

https://codereview.chromium.org/2694083005/diff/300001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3200: 
just checking: will all this work still fine in the case of webview, which is
--single-process and has browser and renderer in the same process?

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
File components/tracing/child/child_trace_message_filter.cc (right):

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
components/tracing/child/child_trace_message_filter.cc:36: #if
!defined(OS_LINUX) && !defined(OS_NACL)
don't we have an equivalent notification in the new service thingy that says
when a client connects to our service ? (not a huge deal, I'm perfectly fine if
this one stays here for the moment, mostly curious)

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
File components/tracing/child/child_trace_message_filter_browsertest.cc (left):

https://codereview.chromium.org/2694083005/diff/300001/components/tracing/chi...
components/tracing/child/child_trace_message_filter_browsertest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
it's ok to lose this coverage I guess. these days tracing is dependen on by so
many places (e.g. telemetry) that everything will go read if we break it :)
I guess the only thing I still care to keep tested is the throttling logic of
the memmory dumps. Is this test going to be moved somewhere else?

https://codereview.chromium.org/2694083005/diff/300001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2694083005/diff/300001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:43: void
CoordinatorImpl::Initialize(base::SingleThreadTaskRunner* task_runner) {
Hmm you should't keep pointers to the TaskRunner themselves as they are proxy
objects and can be short lived. Instead you should have your own
scoped_refptr<STTR>.
That wil NOT keep the taskrunner alive, but will give you a long lived proxy
(and PostTask will fail if the taskrunner goes away)
Sign in to reply to this message.

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