memory-infra: Finish moving to Mojo (3nd attempt)

memory-infra: Finish moving to Mojo (3nd attempt)

This was originally landed in https://codereview.chromium.org/2734193002 and,
before that, in https://codereview.chromium.org/2694083005. But it broke chrome
when component build is on. The issue was that
//services/resource_coordinator:lib is a static library with static local
variables. This CL fixes it.

Tested on a component build.

An example interaction diagram (to see the large image, click on it):
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4/edit#heading=h.1ku5wcoxqifr

BUG=679830, 697773, 697384, 697062
Review-Url: https://codereview.chromium.org/2741203002
Cr-Commit-Position: refs/heads/master@{#457712}
Committed: https://chromium.googlesource.com/chromium/src/+/6e4c507dc612077518d242eabcfae9622601d7e6

[chiniforooshan, 2017-03-10 22:17:33]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 22:17:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chiniforooshan, 2017-03-10 23:11:47]

chiniforooshan@chromium.org changed reviewers:
+ primiano@chromium.org

[chiniforooshan, 2017-03-10 23:11:48]

Hi Primiano, I removed global/static variables. CoordinatorImpl is now owned by
BrowserMainLoop, the same thing that initializes and shuts down Mojo. And,
MemoryDumpManagerDelegateImpl is owned by MemoryDumpManager, the only class that
actually uses it. Could you please take a look? Thanks.

[commit-bot: I haz the power, 2017-03-10 23:50:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 23:50:38]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-03-13 21:31:11]

The new code layout of the memory classes looks good to me.
A bit doubtful about that static method in BrowserMainLoop, also can't see where
this is used.

I don't see any change in the BUILD.gn files though (am I diffing the wrong
patchsets?) so not sure how the ODR violation is going away.
I understand that you might not be affected by that anymore as now you got rid
of the global static that was making it evident, but I think we might be still
in the case where a source set is pulled into two different components (or maybe
I missed a part of this CL when looking at incremental diffs)

Thanks for the patience here

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:221: delegate_.swap(delegate);
delegate_ = std::move(delegate) is the new, sexy, C++11 way of doing this. For
more context see
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OT7H_joutY8

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:290:
MemoryDumpManagerDelegateForTesting* delegate_ptr_;
we do we need both here, unique_ptr has an operator* which does what you want
right?

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:846:
base::MakeUnique<memory_instrumentation::CoordinatorImpl>(true);
maybe add a comment to: true /* is_coordinator*/

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:116: static
memory_instrumentation::CoordinatorImpl*
hmm this seems a bit of an unprecedented pattern. Make sure you check this with
content/browser owners.
Why can't we use BML::GetInsance()->memory_instrumentation() ?

[DmitrySkiba, 2017-03-14 00:00:42]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2017-03-14 00:00:43]

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:143: uint64_t GetTracingProcessId() const
{ return tracing_process_id_; }
I think this should be get_tracing_process_id(), since it's trivial.

[chiniforooshan, 2017-03-14 16:06:35]

chiniforooshan@chromium.org changed reviewers:
+ jam@chromium.org, mkwst@chromium.org

[chiniforooshan, 2017-03-14 16:06:37]

+jam: Sorry that this CL keeps getting reverted :( I hope this is attempt
succeeds. Could you please pay special attention to the
content/browser/browser_main_loop.h and Primiano's comment about it? We are not
sure if adding the static method if the correct way of accessing the coordinator
on the IO thread. I have uploaded the old CL so that you can look at the
differences, only.

+mkwst: Could you please take a look at content_browser_manifest.json and
constants.mojom?

primiano: As for BrowserMainLoop, please refer to my reply to your comment in
the code.

As for the BUILD files, I made the lifetime of the objects more explicit and
removed all global objects. So, know the service, which is still very small, can
be statically linked to other components. This is similar to the file service,
device service, image decoder service, etc. Only content/browser and
content/test link to the resource_coordinator:lib. Everything else links to the
public targets. We can make it a component later if the service gets huge and
size was an issue.

Thanks a lot everyone!

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:221: delegate_.swap(delegate);
On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> delegate_ = std::move(delegate) is the new, sexy, C++11 way of doing this. For
> more context see
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OT7H_joutY8

Done.

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:143: uint64_t GetTracingProcessId() const
{ return tracing_process_id_; }
On 2017/03/14 00:00:43, DmitrySkiba wrote:
> I think this should be get_tracing_process_id(), since it's trivial.

Yes. You are right. But, in my previous versions of this CL I used
tracing_process_id(), but that required too many changes in files that are
really not related to this CL and so making the CL unnecessarily large.
Reviewers suggested to keep this name for now to not make the already-large CL
even larger and change it later.

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:290:
MemoryDumpManagerDelegateForTesting* delegate_ptr_;
On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> we do we need both here, unique_ptr has an operator* which does what you want
> right?

No, actually. Know that the delegate is not a global variable and MDM takes over
the ownership of the delegate, we wouldn't be able to use the * operator after
ownership transfer.

An alternative would be to expose the delegate by MDM in a method like
GetDelegateForTesting(); but, unfortunately, that wouldn't work either, because
then we have to cast GetDelegateForTesting() to the subclass used here in the
tests, i.e. MemoryDumpManagerDelegateForTesting, which is not allowed.

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:846:
base::MakeUnique<memory_instrumentation::CoordinatorImpl>(true);
On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> maybe add a comment to: true /* is_coordinator*/

Done.

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:116: static
memory_instrumentation::CoordinatorImpl*
On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> hmm this seems a bit of an unprecedented pattern. Make sure you check this
with
> content/browser owners.
> Why can't we use BML::GetInsance()->memory_instrumentation() ?

The GPUProcessHost is created in the IO thread, not the UI thread. So, when
registering the coordinator interface, it cannot directly call GetInstance(),
because GetInstance() should be called on the UI thread.

On the other hand, if it does a PostTask to register the coordinator interface
on the UI thread, then the GPU process may try to connect to the interface
before the interface is registered and so fail.

So, I thought I expose only the coordinator in the IO thread because accessing
this special field is not racy. It is created before and parallel read access
should be fine.

I'll ask jam@ to pay special attention to this part.

[Primiano Tucci (use gerrit), 2017-03-15 16:59:40]

*memory* LGTM.
I leave the discussion about browswer_main_loop.cc to more knowledgeable people.
May the force be with you :)

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:143: uint64_t GetTracingProcessId() const
{ return tracing_process_id_; }
On 2017/03/14 16:06:36, chiniforooshan wrote:
> On 2017/03/14 00:00:43, DmitrySkiba wrote:
> > I think this should be get_tracing_process_id(), since it's trivial.
> 
> Yes. You are right. But, in my previous versions of this CL I used
> tracing_process_id(), but that required too many changes in files that are
> really not related to this CL and so making the CL unnecessarily large.
> Reviewers suggested to keep this name for now to not make the already-large CL
> even larger and change it later.

Yup that's was me. Dmitry you are definitly right, but there were too many call
sites (and hence OWNERS) required. I suggested to do separately (or just not do
at all).
Given the size of the change, git-revertability is a nice property to preserve
here :)

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:290:
MemoryDumpManagerDelegateForTesting* delegate_ptr_;
On 2017/03/14 16:06:36, chiniforooshan wrote:
> On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> > we do we need both here, unique_ptr has an operator* which does what you
want
> > right?
> 
> No, actually. Know that the delegate is not a global variable and MDM takes
over
> the ownership of the delegate, we wouldn't be able to use the * operator after
> ownership transfer.
> 
> An alternative would be to expose the delegate by MDM in a method like
> GetDelegateForTesting(); but, unfortunately, that wouldn't work either,
because
> then we have to cast GetDelegateForTesting() to the subclass used here in the
> tests, i.e. MemoryDumpManagerDelegateForTesting, which is not allowed.

Ahh hold on, now I read it correctly.
Yes, you are right, there is ownership transfer. So really it's the other way
round. the |delegate_ptr_| is what you want but... the |delegate_| at this point
becomes useless (or maybe I am still misreading this?) because you steal the
ownership soon after.
In other words my suggestion here is:

- Keep only the delegate_ptr_ (but just call it delegate_, being a raw pointer)
- Get rid of delegate_
In InitializeMemoryDumpManager just do:

delegate_ = new MemDelegateForTesting.
mdm_->Initialize(WrapUnique(delegate_)).

In this way you still move ownership but are left with a cached copy of the
delegate.

[jam, 2017-03-15 17:14:46]

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:116: static
memory_instrumentation::CoordinatorImpl*
On 2017/03/14 16:06:36, chiniforooshan wrote:
> On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> > hmm this seems a bit of an unprecedented pattern. Make sure you check this
> with
> > content/browser owners.
> > Why can't we use BML::GetInsance()->memory_instrumentation() ?
> 
> The GPUProcessHost is created in the IO thread, not the UI thread. So, when
> registering the coordinator interface, it cannot directly call GetInstance(),
> because GetInstance() should be called on the UI thread.
> 
> On the other hand, if it does a PostTask to register the coordinator interface
> on the UI thread, then the GPU process may try to connect to the interface
> before the interface is registered and so fail.
> 
> So, I thought I expose only the coordinator in the IO thread because accessing
> this special field is not racy. It is created before and parallel read access
> should be fine.
> 
> I'll ask jam@ to pay special attention to this part.

BrowserMainLoop shouldn't be accessed on non-UI threads, even internally. Why
not have a getter in
memory_instrumentation::CoordinatorImp?

[chiniforooshan, 2017-03-15 18:27:30]

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2741203002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:290:
MemoryDumpManagerDelegateForTesting* delegate_ptr_;
On 2017/03/15 16:59:40, Primiano Tucci (slow - perf) wrote:
> On 2017/03/14 16:06:36, chiniforooshan wrote:
> > On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> > > we do we need both here, unique_ptr has an operator* which does what you
> want
> > > right?
> > 
> > No, actually. Know that the delegate is not a global variable and MDM takes
> over
> > the ownership of the delegate, we wouldn't be able to use the * operator
after
> > ownership transfer.
> > 
> > An alternative would be to expose the delegate by MDM in a method like
> > GetDelegateForTesting(); but, unfortunately, that wouldn't work either,
> because
> > then we have to cast GetDelegateForTesting() to the subclass used here in
the
> > tests, i.e. MemoryDumpManagerDelegateForTesting, which is not allowed.
> 
> Ahh hold on, now I read it correctly.
> Yes, you are right, there is ownership transfer. So really it's the other way
> round. the |delegate_ptr_| is what you want but... the |delegate_| at this
point
> becomes useless (or maybe I am still misreading this?) because you steal the
> ownership soon after.
> In other words my suggestion here is:
> 
> - Keep only the delegate_ptr_ (but just call it delegate_, being a raw
pointer)
> - Get rid of delegate_
> In InitializeMemoryDumpManager just do:
> 
> delegate_ = new MemDelegateForTesting.
> mdm_->Initialize(WrapUnique(delegate_)).
> 
> In this way you still move ownership but are left with a cached copy of the
> delegate.

Done.

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2741203002/diff/80001/content/browser/browser...
content/browser/browser_main_loop.h:116: static
memory_instrumentation::CoordinatorImpl*
On 2017/03/15 17:14:45, jam wrote:
> On 2017/03/14 16:06:36, chiniforooshan wrote:
> > On 2017/03/13 21:31:10, Primiano Tucci (slow - perf) wrote:
> > > hmm this seems a bit of an unprecedented pattern. Make sure you check this
> > with
> > > content/browser owners.
> > > Why can't we use BML::GetInsance()->memory_instrumentation() ?
> > 
> > The GPUProcessHost is created in the IO thread, not the UI thread. So, when
> > registering the coordinator interface, it cannot directly call
GetInstance(),
> > because GetInstance() should be called on the UI thread.
> > 
> > On the other hand, if it does a PostTask to register the coordinator
interface
> > on the UI thread, then the GPU process may try to connect to the interface
> > before the interface is registered and so fail.
> > 
> > So, I thought I expose only the coordinator in the IO thread because
accessing
> > this special field is not racy. It is created before and parallel read
access
> > should be fine.
> > 
> > I'll ask jam@ to pay special attention to this part.
> 
> BrowserMainLoop shouldn't be accessed on non-UI threads, even internally. Why
> not have a getter in
> memory_instrumentation::CoordinatorImp?

I see. I was hoping to not use global variables in memory_instrumentation; but
looks like, there is no way around it. Done. The creation and deletion of the
CoordinatorImpl is still here, to make its lifetime more explicit (instead of
using leaky lazy instances, e.g.).

[Mike West, 2017-03-16 15:06:00]

(The bits you've asked me to review look reasonable, but I'm holding off
stamping the patch until the design is hammered out as things seem likely to
shift)

[jam, 2017-03-16 15:42:43]

lgtm with comment

https://codereview.chromium.org/2741203002/diff/120001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2741203002/diff/120001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:26: static
void SetInstance(CoordinatorImpl* coordinator_impl);
this seems redundant; just set the global in the constructor and destructor of
this class.

[chiniforooshan, 2017-03-16 17:13:17]

Thanks all. Will wait for Mike's LGTM.

https://codereview.chromium.org/2741203002/diff/120001/services/resource_coor...
File services/resource_coordinator/memory/coordinator/coordinator_impl.h
(right):

https://codereview.chromium.org/2741203002/diff/120001/services/resource_coor...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:26: static
void SetInstance(CoordinatorImpl* coordinator_impl);
On 2017/03/16 15:42:43, jam wrote:
> this seems redundant; just set the global in the constructor and destructor of
> this class.

Done.

[Mike West, 2017-03-16 17:37:19]

LGTM, thanks!

[chiniforooshan, 2017-03-16 21:16:02]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 21:16:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 22:42:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 22:42:39]

Dry run: This issue passed the CQ dry run.

[chiniforooshan, 2017-03-17 03:09:57]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-03-17 03:09:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, mkwst@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2741203002/#ps160001
(title: "nit: comment")

[commit-bot: I haz the power, 2017-03-17 03:10:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 03:18:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 03:18:39]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chiniforooshan, 2017-03-17 04:10:16]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-03-17 04:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, mkwst@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2741203002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-17 04:10:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 06:13:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 06:13:22]

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_...)

[chiniforooshan, 2017-03-17 07:13:19]

On 2017/03/17 06:13:22, commit-bot: I haz the power wrote:
> 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_...)

Looks like flakiness. Errors do not look relevant and linux_chromium_rel_ng
failed 2 times, but each time in a different step:

- 1st time: telemetry_perf_unittests (with patch) failed,
browser_side_navigation_browser_tests (with patch) passed.
- 2nd time: telemetry_perf_unittests (with patch) passed,
browser_side_navigation_browser_tests (with patch) failed.

Going to try another time.

[chiniforooshan, 2017-03-17 07:13:30]

The CQ bit was checked by chiniforooshan@chromium.org

[commit-bot: I haz the power, 2017-03-17 07:13:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 07:57:01]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489734810568640,
"parent_rev": "6c2939ade3de360100011544d5600adc49013a31", "commit_rev":
"6e4c507dc612077518d242eabcfae9622601d7e6"}

[commit-bot: I haz the power, 2017-03-17 07:57:44]

Description was changed from

==========
memory-infra: Finish moving to Mojo (3nd attempt)

This was originally landed in https://codereview.chromium.org/2734193002 and,
before that, in https://codereview.chromium.org/2694083005. But it broke chrome
when component build is on. The issue was that
//services/resource_coordinator:lib is a static library with static local
variables. This CL fixes it.

Tested on a component build.

An example interaction diagram (to see the large image, click on it):
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_...

BUG=679830, 697773, 697384, 697062
==========

to

==========
memory-infra: Finish moving to Mojo (3nd attempt)

This was originally landed in https://codereview.chromium.org/2734193002 and,
before that, in https://codereview.chromium.org/2694083005. But it broke chrome
when component build is on. The issue was that
//services/resource_coordinator:lib is a static library with static local
variables. This CL fixes it.

Tested on a component build.

An example interaction diagram (to see the large image, click on it):
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_...

BUG=679830, 697773, 697384, 697062

Review-Url: https://codereview.chromium.org/2741203002
Cr-Commit-Position: refs/heads/master@{#457712}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e4c507dc612077518d242eabcfa...
==========

[commit-bot: I haz the power, 2017-03-17 07:57:45]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/6e4c507dc612077518d242eabcfa...