Hosting the discardable memory service in the mus process

Hosting the discardable memory service in the mus process.

With mus+ash, we host the discardable memory service in the mus process.
For classic chrome, we will run the discardable memory service in the
browser process.

BUG=654678
Committed: https://crrev.com/d6843e447d292f253b427d04022c4bc562d7aaab
Cr-Commit-Position: refs/heads/master@{#439342}

[Peng, 2016-12-05 21:12:25]

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

[commit-bot: I haz the power, 2016-12-05 21:13:27]

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, 2016-12-05 21:24:07]

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

[commit-bot: I haz the power, 2016-12-05 21:24:08]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Peng, 2016-12-05 21:33:12]

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

[commit-bot: I haz the power, 2016-12-05 21:33:39]

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, 2016-12-05 23:42:11]

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

[commit-bot: I haz the power, 2016-12-05 23:42:11]

Dry run: This issue passed the CQ dry run.

[Peng, 2016-12-06 20:38:45]

Description was changed from

==========
WIP


Use dm in mus

BUG=654678
==========

to

==========
Hosting the discardable memory service in the mus process.

With mus+ash, we host the discardable memory service in the mus process.
For classic chrome, we will run the discardable memory service in the
browser process. 

BUG=654678
==========

[Peng, 2016-12-06 20:47:52]

penghuang@chromium.org changed reviewers:
+ sadrul@chromium.org

[Peng, 2016-12-06 20:47:53]

Hi Sadrul, Could you please take a look changes in ui/views and maybe changes in
services/ui as well? Thanks.

[Fady Samuel, 2016-12-07 14:07:57]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org, rjkroege@chromium.org

[Fady Samuel, 2016-12-07 14:07:58]

Drive-by Question (sadrul? rjkroege?): Should discardable shared memory manager
service live in Mus-Gpu or Mus-Ws? My inclination is Mus-Gpu because these
resources are usually
graphics related, right? If the GPU process goes away then
so does all the discardable memory, right?

If in Mus-Gpu, then it needs to hang off the GpuMain interface.

[Peng, 2016-12-07 16:07:31]

On 2016/12/07 14:07:58, Fady Samuel wrote:
> Drive-by Question (sadrul? rjkroege?): Should discardable shared memory
manager
> service live in Mus-Gpu or Mus-Ws? My inclination is Mus-Gpu because these
> resources are usually
> graphics related, right? If the GPU process goes away then
> so does all the discardable memory, right?
> 
> If in Mus-Gpu, then it needs to hang off the GpuMain interface.

FYI, I think it is better to run this service in a privileged process, because
this
service needs allocate shared memory frequently. If the process does have the
privilege,
we need an extra sync IPC for every memory allocation. Does mus-gpu have hte
privilege?

[sadrul, 2016-12-07 16:11:41]

sadrul@chromium.org changed reviewers:
+ reveman@chromium.org

[sadrul, 2016-12-07 16:11:41]

+reveman@

On 2016/12/07 14:07:58, Fady Samuel wrote:
> Drive-by Question (sadrul? rjkroege?): Should discardable shared memory
manager
> service live in Mus-Gpu or Mus-Ws? My inclination is Mus-Gpu because these
> resources are usually
> graphics related, right? If the GPU process goes away then
> so does all the discardable memory, right?
> 
> If in Mus-Gpu, then it needs to hang off the GpuMain interface.

Considering that this currently lives in chrome-browser, I think mus-ws is a
good equivalent? If it lived in chrome-gpu, then mus-gpu would be the right
place. Of course, if it lives in chrome-browser because that's where the DC
(display compositor) is, then maybe it should move to mus-gpu since that's where
DC lives in mus. Does the DiscardableShmManager need additional privilege?

Gpu memory-buffer requests go through mus-ws before it goes to mus-gpu. Would we
have to do that for this too if it goes in mus-gpu?

[reveman, 2016-12-07 16:34:56]

On 2016/12/07 at 16:11:41, sadrul wrote:
> +reveman@
> 
> On 2016/12/07 14:07:58, Fady Samuel wrote:
> > Drive-by Question (sadrul? rjkroege?): Should discardable shared memory
manager
> > service live in Mus-Gpu or Mus-Ws? My inclination is Mus-Gpu because these
> > resources are usually
> > graphics related, right? If the GPU process goes away then
> > so does all the discardable memory, right?
> > 
> > If in Mus-Gpu, then it needs to hang off the GpuMain interface.
> 
> Considering that this currently lives in chrome-browser, I think mus-ws is a
good equivalent? If it lived in chrome-gpu, then mus-gpu would be the right
place. Of course, if it lives in chrome-browser because that's where the DC
(display compositor) is, then maybe it should move to mus-gpu since that's where
DC lives in mus. Does the DiscardableShmManager need additional privilege?
> 
> Gpu memory-buffer requests go through mus-ws before it goes to mus-gpu. Would
we have to do that for this too if it goes in mus-gpu?

I think it needs to live in mus-Ws. The service can't recover from a mus-gpu
crash. Discardable memory is used for more than gfx related things. E.g. the
resource cache in blink.

[sadrul, 2016-12-07 17:56:40]

https://codereview.chromium.org/2548423002/diff/40001/content/public/app/mojo...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2548423002/diff/40001/content/public/app/mojo...
content/public/app/mojo/content_renderer_manifest.json:24:
"ui:discardable_memory"
sort

https://codereview.chromium.org/2548423002/diff/40001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2548423002/diff/40001/content/renderer/render...
content/renderer/render_thread_impl.cc:896: if (!manager_initialized) {
Do you need |manager_initialized|? Can you check |manager_ptr| instead?

https://codereview.chromium.org/2548423002/diff/40001/services/ui/manifest.json
File services/ui/manifest.json (right):

https://codereview.chromium.org/2548423002/diff/40001/services/ui/manifest.js...
services/ui/manifest.json:56: "ui:discardable_memory": [
Move this up to be sorted

[Peng, 2016-12-07 19:02:05]

https://codereview.chromium.org/2548423002/diff/40001/content/public/app/mojo...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2548423002/diff/40001/content/public/app/mojo...
content/public/app/mojo/content_renderer_manifest.json:24:
"ui:discardable_memory"
On 2016/12/07 17:56:40, sadrul wrote:
> sort

Done.

https://codereview.chromium.org/2548423002/diff/40001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2548423002/diff/40001/content/renderer/render...
content/renderer/render_thread_impl.cc:896: if (!manager_initialized) {
On 2016/12/07 17:56:40, sadrul wrote:
> Do you need |manager_initialized|? Can you check |manager_ptr| instead?

Seems ConnectToInterface() can be failed, so probably it is better remove #if
define(), and always check IsRunningInMash().

https://codereview.chromium.org/2548423002/diff/40001/services/ui/manifest.json
File services/ui/manifest.json (right):

https://codereview.chromium.org/2548423002/diff/40001/services/ui/manifest.js...
services/ui/manifest.json:56: "ui:discardable_memory": [
On 2016/12/07 17:56:40, sadrul wrote:
> Move this up to be sorted

Done.

[Peng, 2016-12-09 16:20:27]

HI Sadrul, I have updated this CL. PTAL. Thanks.

[sadrul, 2016-12-14 05:07:44]

lgtm

[Peng, 2016-12-14 17:15:21]

penghuang@chromium.org changed reviewers:
+ sky@chromium.org

[Peng, 2016-12-14 17:15:21]

+sky@ for browser/* and services/ui/*

Hi Scott, PTAL. Thanks.

[Peng, 2016-12-14 17:20:51]

Hi David, PTAL. Thanks.

[reveman, 2016-12-14 17:43:27]

lgtm

[Peng, 2016-12-14 18:05:33]

penghuang@chromium.org changed reviewers:
+ dcheng@chromium.org

[Peng, 2016-12-14 18:05:34]

+dcheng@ for content/*

Hi Danial, PTAL. Thanks.

[Peng, 2016-12-15 19:13:22]

On 2016/12/14 18:05:34, Peng wrote:
> +dcheng@ for content/*
> 
> Hi Danial, PTAL. Thanks.

Scott & Daniel, kindly ping.

[sky, 2016-12-15 20:40:08]

https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc
File services/ui/service.cc (right):

https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc#...
services/ui/service.cc:202:
discardable_memory::DiscardableSharedMemoryManager::CreateInstance();
Is there a corresponding destroy?

[Peng, 2016-12-15 20:53:23]

https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc
File services/ui/service.cc (right):

https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc#...
services/ui/service.cc:202:
discardable_memory::DiscardableSharedMemoryManager::CreateInstance();
On 2016/12/15 20:40:08, sky wrote:
> Is there a corresponding destroy?

No. When this service in browser process, we do destroy it. Is it necessary to
destroy it in mus?

[sky, 2016-12-15 21:10:34]

Why wouldn't you want to? Not shutting down things can make for hard
to write tests and flake during shutdown.

On Thu, Dec 15, 2016 at 12:53 PM,  <penghuang@chromium.org> wrote:
>
> https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc
> File services/ui/service.cc (right):
>
>
https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc#...
> services/ui/service.cc:202:
> discardable_memory::DiscardableSharedMemoryManager::CreateInstance();
> On 2016/12/15 20:40:08, sky wrote:
>> Is there a corresponding destroy?
>
> No. When this service in browser process, we do destroy it. Is it
> necessary to destroy it in mus?
>
> https://codereview.chromium.org/2548423002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2016-12-15 22:06:32]

https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc
File services/ui/service.cc (right):

https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc#...
services/ui/service.cc:202:
discardable_memory::DiscardableSharedMemoryManager::CreateInstance();
On 2016/12/15 20:40:08, sky wrote:
> Is there a corresponding destroy?

discardable_memory::DiscardableSharedMemoryManager::CreateInstance() creates an
instance by using LazyInstance. It will be deleted during shutdown. Is it OK or
we should explicitly delete it?

[sky, 2016-12-15 22:09:30]

Ok, fair enough. LGTM

[reveman, 2016-12-15 22:16:17]

On 2016/12/15 at 22:06:32, penghuang wrote:
> https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc
> File services/ui/service.cc (right):
> 
>
https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc#...
> services/ui/service.cc:202:
discardable_memory::DiscardableSharedMemoryManager::CreateInstance();
> On 2016/12/15 20:40:08, sky wrote:
> > Is there a corresponding destroy?
> 
> discardable_memory::DiscardableSharedMemoryManager::CreateInstance() creates
an instance by using LazyInstance. It will be deleted during shutdown. Is it OK
or we should explicitly delete it?

Just to add some background; It used to be that we just had a LazyInstance and
that was used to implement DiscardableSharedMemoryManager::current().
CreateInstance was added as mojo refactor required that. I think making both
create and destroy explicit is better and it would be nice to see that in a
follow up patch.

[dcheng, 2016-12-16 09:13:02]

manifest changes LGTM with a random thought thrown in (no action necessary, just
something to consider)

https://codereview.chromium.org/2548423002/diff/80001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2548423002/diff/80001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:181: if (!io_task_runner) {
Random thought: I looked through all the callsites I could find in codesearch...
and no one ever passes an IO task runner. Kind of feels like the argument should
just be removed...

(I'm generally not a fan of optional arguments if we don't need them)

[Peng, 2016-12-16 14:58:47]

On 2016/12/15 22:16:17, reveman wrote:
> On 2016/12/15 at 22:06:32, penghuang wrote:
> > https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc
> > File services/ui/service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2548423002/diff/80001/services/ui/service.cc#...
> > services/ui/service.cc:202:
> discardable_memory::DiscardableSharedMemoryManager::CreateInstance();
> > On 2016/12/15 20:40:08, sky wrote:
> > > Is there a corresponding destroy?
> > 
> > discardable_memory::DiscardableSharedMemoryManager::CreateInstance() creates
> an instance by using LazyInstance. It will be deleted during shutdown. Is it
OK
> or we should explicitly delete it?
> 
> Just to add some background; It used to be that we just had a LazyInstance and
> that was used to implement DiscardableSharedMemoryManager::current().
> CreateInstance was added as mojo refactor required that. I think making both
> create and destroy explicit is better and it would be nice to see that in a
> follow up patch.

Sure. I will make a separate patch for it.

[Peng, 2016-12-16 15:03:10]

https://codereview.chromium.org/2548423002/diff/80001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2548423002/diff/80001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:181: if (!io_task_runner) {
On 2016/12/16 09:13:01, dcheng wrote:
> Random thought: I looked through all the callsites I could find in
codesearch...
> and no one ever passes an IO task runner. Kind of feels like the argument
should
> just be removed...
> 
> (I'm generally not a fan of optional arguments if we don't need them)

Good suggestion. I will make a separate CL for it.

[Peng, 2016-12-16 15:03:47]

penghuang@chromium.org changed reviewers:
+ avi@chromium.org

[Peng, 2016-12-16 15:03:47]

+avi@ for content/*

Hi Avi, PTAL. Thanks.

[Peng, 2016-12-16 15:08:25]

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

[commit-bot: I haz the power, 2016-12-16 15:08: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, 2016-12-16 15:10:25]

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

[commit-bot: I haz the power, 2016-12-16 15:10:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Peng, 2016-12-16 15:37:44]

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

[commit-bot: I haz the power, 2016-12-16 15:37:55]

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

[Avi (use Gerrit), 2016-12-16 15:55:02]

lgtm

stamp

[commit-bot: I haz the power, 2016-12-16 16:25:09]

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

[commit-bot: I haz the power, 2016-12-16 16:25:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peng, 2016-12-16 16:50:21]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-12-16 16:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org, dcheng@chromium.org, reveman@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2548423002/#ps120001
(title: "Fix a test failure")

[commit-bot: I haz the power, 2016-12-16 16:50:47]

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, 2016-12-16 17:06:24]

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

[commit-bot: I haz the power, 2016-12-16 17:06:25]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Peng, 2016-12-16 19:38:23]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-12-16 19:38:23]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
sadrul@chromium.org, sky@chromium.org, dcheng@chromium.org, reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2548423002/#ps140001
(title: "Fix compile issue")

[commit-bot: I haz the power, 2016-12-16 19:39:41]

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, 2016-12-16 22:38:21]

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

[commit-bot: I haz the power, 2016-12-16 22:38:22]

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

[Peng, 2016-12-17 12:10:58]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-12-17 12:10:58]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
sadrul@chromium.org, sky@chromium.org, dcheng@chromium.org, reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2548423002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-17 12:11:04]

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, 2016-12-17 13:57:23]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481976658302570,
"parent_rev": "fdd6b2601f47e66c7d19ae6fba2329ef597ffca5", "commit_rev":
"b46b5b8f8401ab706c06b97517f8e3606bf4f29b"}

[commit-bot: I haz the power, 2016-12-17 13:57:52]

Description was changed from

==========
Hosting the discardable memory service in the mus process.

With mus+ash, we host the discardable memory service in the mus process.
For classic chrome, we will run the discardable memory service in the
browser process. 

BUG=654678
==========

to

==========
Hosting the discardable memory service in the mus process.

With mus+ash, we host the discardable memory service in the mus process.
For classic chrome, we will run the discardable memory service in the
browser process. 

BUG=654678

Review-Url: https://codereview.chromium.org/2548423002
==========

[commit-bot: I haz the power, 2016-12-17 13:57:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-17 14:02:29]

Description was changed from

==========
Hosting the discardable memory service in the mus process.

With mus+ash, we host the discardable memory service in the mus process.
For classic chrome, we will run the discardable memory service in the
browser process. 

BUG=654678

Review-Url: https://codereview.chromium.org/2548423002
==========

to

==========
Hosting the discardable memory service in the mus process.

With mus+ash, we host the discardable memory service in the mus process.
For classic chrome, we will run the discardable memory service in the
browser process.

BUG=654678

Committed: https://crrev.com/d6843e447d292f253b427d04022c4bc562d7aaab
Cr-Commit-Position: refs/heads/master@{#439342}
==========

[commit-bot: I haz the power, 2016-12-17 14:02:31]

Patchset 9 (id:??) landed as
https://crrev.com/d6843e447d292f253b427d04022c4bc562d7aaab
Cr-Commit-Position: refs/heads/master@{#439342}