Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2485623002:
discardable_memory: Using mojo IPC to replace Chrome IPC (Closed)
|
Created:
4 years, 1 month ago by Peng Modified:
4 years ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiondiscardable_memory: Using mojo IPC to replace Chrome IPC
It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl
to RenderThreadImpl and PpapiThread
BUG=654678
Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd
Committed: https://crrev.com/342762b5a769ad9ed321ded9254bde4b44a2450b
Cr-Original-Commit-Position: refs/heads/master@{#435969}
Cr-Commit-Position: refs/heads/master@{#436010}
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : WIP #Patch Set 4 : Fix tests errors #Patch Set 5 : Rebase #Patch Set 6 : Fix build error on win #Patch Set 7 : WIP #Patch Set 8 : Fix build error #Patch Set 9 : Update #
Total comments: 16
Patch Set 10 : Address review issues. #
Total comments: 12
Patch Set 11 : Address review issues #
Total comments: 2
Patch Set 12 : Address review issues #Patch Set 13 : Fix trybot errors #
Total comments: 33
Patch Set 14 : Address review issues #Patch Set 15 : Rebase #
Total comments: 9
Patch Set 16 : Address review issues. #Patch Set 17 : Address review issues #Patch Set 18 : Fix build bots. #
Total comments: 47
Patch Set 19 : Address review issues #Patch Set 20 : Address review issues #
Total comments: 1
Patch Set 21 : rebase #Patch Set 22 : Address review issues #
Total comments: 8
Patch Set 23 : Address review issues #Patch Set 24 : Fix bot issues #Messages
Total messages: 164 (113 generated)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP WIP WIP BUG= ========== to ========== discardable_memory: Using mojo IPC to to replace Chrome IPC BUG=654678 ==========
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
penghuang@chromium.org changed reviewers: + reveman@chromium.org
Hi David, PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== discardable_memory: Using mojo IPC to to replace Chrome IPC BUG=654678 ========== to ========== discardable_memory: Using mojo IPC to replace Chrome IPC BUG=654678 ==========
https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:34: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); s/scoped_refptr<base::SingleThreadTaskRunner>/base::SingleThreadTaskRunner*/ to avoid unnecessary refcount churn https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:75: scoped_refptr<ThreadSafeManagerProxy> manager_; why do we need a separate instance for this? Can the current manager class contain the logic needed for IPC to be threads safe? Having a different manager instance inside the manager class is awkward and makes the code harder to understand imo https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/public/interfaces/discardable_memory.mojom (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/public/interfaces/discardable_memory.mojom:1: // Copyright 2016 The Chromium Authors. All rights reserved. should the name of this file be "discardable_shared_memory_manager.mojom" ? https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/public/interfaces/discardable_memory.mojom:8: // Allocate a locked discardable share memory. s/discardable share memory/discardable shared memory segment/ https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/public/interfaces/discardable_memory.mojom:12: // Notify a discardable shared memory is deleted. "Notify manager that a memory segment has been deleted"? https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:14: int last_client_id = 0; Can this be in the ::discardable_memory::DiscardableSharedMemoryManager instance instead? If we need to keep it here then use: base::StaticAtomicSequenceNumber g_next_client_id https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:21: ::discardable_memory::DiscardableSharedMemoryManager::current() Can this class have a pointer to the manager instance instead of relying on ::current()? https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.h (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.h:25: // mojom::DiscardableSharedMemoryManager overrids: s/overrids/overrides/
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:34: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); On 2016/11/11 20:17:26, reveman wrote: > s/scoped_refptr<base::SingleThreadTaskRunner>/base::SingleThreadTaskRunner*/ to > avoid unnecessary refcount churn Done. https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:75: scoped_refptr<ThreadSafeManagerProxy> manager_; On 2016/11/11 20:17:26, reveman wrote: > why do we need a separate instance for this? Can the current manager class > contain the logic needed for IPC to be threads safe? Having a different manager > instance inside the manager class is awkward and makes the code harder to > understand imo It is because I want to make manager_ a thread safe refcount ptr, So I can use it in base::Bind() for some callbacks which will be called in io thread. I tried using WeakPtrFactory in this class, but this class will be destroyed in main thread, it causes problem (because WeakPtr is not thread safe, dereference and invalidate have to be in the same thread). https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/public/interfaces/discardable_memory.mojom (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/public/interfaces/discardable_memory.mojom:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/11 20:17:26, reveman wrote: > should the name of this file be "discardable_shared_memory_manager.mojom" ? Done. https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/public/interfaces/discardable_memory.mojom:8: // Allocate a locked discardable share memory. On 2016/11/11 20:17:26, reveman wrote: > s/discardable share memory/discardable shared memory segment/ Done. https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/public/interfaces/discardable_memory.mojom:12: // Notify a discardable shared memory is deleted. On 2016/11/11 20:17:26, reveman wrote: > "Notify manager that a memory segment has been deleted"? Done. https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:14: int last_client_id = 0; On 2016/11/11 20:17:27, reveman wrote: > Can this be in the ::discardable_memory::DiscardableSharedMemoryManager instance > instead? If we need to keep it here then use: > base::StaticAtomicSequenceNumber g_next_client_id Done. https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:21: ::discardable_memory::DiscardableSharedMemoryManager::current() On 2016/11/11 20:17:26, reveman wrote: > Can this class have a pointer to the manager instance instead of relying on > ::current()? Done. https://codereview.chromium.org/2485623002/diff/160001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.h (right): https://codereview.chromium.org/2485623002/diff/160001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.h:25: // mojom::DiscardableSharedMemoryManager overrids: On 2016/11/11 20:17:27, reveman wrote: > s/overrids/overrides/ Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Kindly ping. On 2016/11/11 22:50:35, Peng wrote: > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > File > components/discardable_memory/client/client_discardable_shared_memory_manager.h > (right): > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/client/client_discardable_shared_memory_manager.h:34: > scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); > On 2016/11/11 20:17:26, reveman wrote: > > s/scoped_refptr<base::SingleThreadTaskRunner>/base::SingleThreadTaskRunner*/ > to > > avoid unnecessary refcount churn > > Done. > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/client/client_discardable_shared_memory_manager.h:75: > scoped_refptr<ThreadSafeManagerProxy> manager_; > On 2016/11/11 20:17:26, reveman wrote: > > why do we need a separate instance for this? Can the current manager class > > contain the logic needed for IPC to be threads safe? Having a different > manager > > instance inside the manager class is awkward and makes the code harder to > > understand imo > > It is because I want to make manager_ a thread safe refcount ptr, So I can use > it in base::Bind() for some callbacks which will be called in io thread. I tried > using WeakPtrFactory in this class, but this class will be destroyed in main > thread, it causes problem (because WeakPtr is not thread safe, dereference and > invalidate have to be in the same thread). > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > File components/discardable_memory/public/interfaces/discardable_memory.mojom > (right): > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/public/interfaces/discardable_memory.mojom:1: // > Copyright 2016 The Chromium Authors. All rights reserved. > On 2016/11/11 20:17:26, reveman wrote: > > should the name of this file be "discardable_shared_memory_manager.mojom" ? > > Done. > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/public/interfaces/discardable_memory.mojom:8: // > Allocate a locked discardable share memory. > On 2016/11/11 20:17:26, reveman wrote: > > s/discardable share memory/discardable shared memory segment/ > > Done. > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/public/interfaces/discardable_memory.mojom:12: // > Notify a discardable shared memory is deleted. > On 2016/11/11 20:17:26, reveman wrote: > > "Notify manager that a memory segment has been deleted"? > > Done. > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > File > components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc > (right): > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:14: > int last_client_id = 0; > On 2016/11/11 20:17:27, reveman wrote: > > Can this be in the ::discardable_memory::DiscardableSharedMemoryManager > instance > > instead? If we need to keep it here then use: > > base::StaticAtomicSequenceNumber g_next_client_id > > Done. > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:21: > ::discardable_memory::DiscardableSharedMemoryManager::current() > On 2016/11/11 20:17:26, reveman wrote: > > Can this class have a pointer to the manager instance instead of relying on > > ::current()? > > Done. > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > File > components/discardable_memory/service/discardable_shared_memory_manager_proxy.h > (right): > > https://codereview.chromium.org/2485623002/diff/160001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager_proxy.h:25: > // mojom::DiscardableSharedMemoryManager overrids: > On 2016/11/11 20:17:27, reveman wrote: > > s/overrids/overrides/ > > Done.
https://codereview.chromium.org/2485623002/diff/180001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc (right): https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:13: int DiscardableSharedMemoryManagerProxy::last_client_id_ = 0; I'd still prefer to see this as a member of the DiscardableSharedMemoryManager class and the client id passed as a ctor argument to the proxy instance. Is that possible? https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:18: ::discardable_memory::DiscardableSharedMemoryManager::current()) {} why can't the manager be passed as an argument instead of having to use DiscardableSharedMemoryManager::current()? https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:21: CHECK_EQ(manager_, what's the purpose of all these CHECKs? https://codereview.chromium.org/2485623002/diff/180001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.h (right): https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.h:25: static void Create(mojom::DiscardableSharedMemoryManagerRequest request); Can you replace this with a DiscardableSharedMemoryManager::Bind function and keep this proxy an implementation detail of DiscardableSharedMemoryManager? ie. move all of it to discardable_shared_memory_manager.cc https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.h:38: ::discardable_memory::DiscardableSharedMemoryManager* manager_; nit: ..Manager* const manager_; https://codereview.chromium.org/2485623002/diff/180001/content/child/child_th... File content/child/child_thread_impl.h (right): https://codereview.chromium.org/2485623002/diff/180001/content/child/child_th... content/child/child_thread_impl.h:354: bool init_discardable_memory; why do we need a flag for this now instead of creating a manager unconditionally as before?
Patchset #11 (id:200001) has been deleted
https://codereview.chromium.org/2485623002/diff/180001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc (right): https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:13: int DiscardableSharedMemoryManagerProxy::last_client_id_ = 0; On 2016/11/15 16:13:20, reveman wrote: > I'd still prefer to see this as a member of the DiscardableSharedMemoryManager > class and the client id passed as a ctor argument to the proxy instance. Is that > possible? Done. https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:18: ::discardable_memory::DiscardableSharedMemoryManager::current()) {} On 2016/11/15 16:13:20, reveman wrote: > why can't the manager be passed as an argument instead of having to use > DiscardableSharedMemoryManager::current()? Done. https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.cc:21: CHECK_EQ(manager_, On 2016/11/15 16:13:20, reveman wrote: > what's the purpose of all these CHECKs? Removed them. https://codereview.chromium.org/2485623002/diff/180001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager_proxy.h (right): https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.h:25: static void Create(mojom::DiscardableSharedMemoryManagerRequest request); On 2016/11/15 16:13:20, reveman wrote: > Can you replace this with a DiscardableSharedMemoryManager::Bind function and > keep this proxy an implementation detail of DiscardableSharedMemoryManager? ie. > move all of it to discardable_shared_memory_manager.cc Done. https://codereview.chromium.org/2485623002/diff/180001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager_proxy.h:38: ::discardable_memory::DiscardableSharedMemoryManager* manager_; On 2016/11/15 16:13:20, reveman wrote: > nit: ..Manager* const manager_; Done. https://codereview.chromium.org/2485623002/diff/180001/content/child/child_th... File content/child/child_thread_impl.h (right): https://codereview.chromium.org/2485623002/diff/180001/content/child/child_th... content/child/child_thread_impl.h:354: bool init_discardable_memory; On 2016/11/15 16:13:20, reveman wrote: > why do we need a flag for this now instead of creating a manager unconditionally > as before? Because we currently only expose the discardable memory service to pepper plugin and renderer(currently the discardable memory is only available to browser, renderer and plugin processes), so we have to only init it for right processes. Otherwise, some processes (GPU, utility processes) will crash.
https://codereview.chromium.org/2485623002/diff/220001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/220001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:162: event.Wait(); Why is this proxy needed when we're using a waitable event? I'm failing to see how ClientDiscardableSharedMemoryManager could be destroyed before IO thread has finished running this task. Can you explain how that can happen?
https://codereview.chromium.org/2485623002/diff/220001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/220001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:162: event.Wait(); On 2016/11/15 22:57:15, reveman wrote: > Why is this proxy needed when we're using a waitable event? I'm failing to see > how ClientDiscardableSharedMemoryManager could be destroyed before IO thread has > finished running this task. Can you explain how that can happen? This function is fine, but the ClientDiscardableSharedMemoryManager::DeletedDiscardableSharedMemory() also need post a task to IO thread. It causes the problem.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi David, I updated CL by following your suggestion. PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
Patchset #13 (id:260001) has been deleted
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by penghuang@chromium.org
The CQ bit was unchecked by penghuang@chromium.org
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lg, mostly nits https://codereview.chromium.org/2485623002/diff/280001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:92: void InitManagerOnIOThread(mojom::DiscardableSharedMemoryManagerPtr* manager, s/IOThread/IO/ and align this function name with the choice of member variable name. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:99: void DeletedDiscardableSharedMemoryOnIOThread( nit: s/OnIOThread/OnIO/ https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:102: if (!manager->is_bound()) when will this happen? can we DCHECK instead? https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:135: if (!io_task_runner_->DeleteSoon(FROM_HERE, manager)) Can this fail in practice? In that case a wrapper function like https://cs.chromium.org/chromium/src/storage/browser/fileapi/sandbox_file_sys... would make it a bit cleaner. Otherwise, bool posted = io_task_runner_->DeleteSoon(FROM_HERE, manager_.release()); DCHECK(posted); would be preferred. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:216: auto shared_memory = nit: please keep the explicit type instead of using auto. It makes it more clear what type of object is returned and as a result makes the code easier to understand imo. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:368: // Waiting until IPC is finished in the IO thread. nit: "...IPC has finished on the IO thread." https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:378: if (!manager_->is_bound()) can this happen? DCHECK instead? https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:391: do { Why this loop? I think the code would be easier to read if you just early out with a return statement. Maybe use a ScopedClosure to ensure event->Signal() is called. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:394: auto handle = base::SharedMemory::NULLHandle(); Can you avoid using auto here an below so it's more clear what the types we're dealing with here are? https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:404: base::TerminateBecauseOutOfMemory(memory_size); nit: base::TerminateBecauseOutOfMemory never returns so the break below is not needed https://codereview.chromium.org/2485623002/diff/280001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:67: void AllocateOnIOThread( nit: s/AllocateOnIOThread/AllocateOnIO/ as it's common practice in chromium code to just suffix functions that are supposed to execute on IO or UI threads simply with OnIO/OnUI. See content/browser/gpu/browser_gpu_memory_buffer_manager.h for an example. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:72: void OnAllocateCompletedOnIOThread( nit: too many "On" :) I don't think we need the On prefix here. I'd prefer AllocatedOnIO or AllocateCompletedOnIO https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:82: std::unique_ptr<mojom::DiscardableSharedMemoryManagerPtr> manager_; Can we rename this variable to have it be less confusing to have manager instance with a manager member? manager_ipc_, manager_mojo_, just ipc_ or mojo_? Something that allows language referring to this member to be less confusing. Currently you have comments mentioning "manager" and it's not perfectly clear if it refers to the instance of this class or this manager_ member. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:85: std::unique_ptr<DiscardableSharedMemoryHeap> heap_; why std::unique_ptr instead of allocated as part of the class as before?
Patchset #14 (id:300001) has been deleted
https://codereview.chromium.org/2485623002/diff/280001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:92: void InitManagerOnIOThread(mojom::DiscardableSharedMemoryManagerPtr* manager, On 2016/11/18 05:26:50, reveman wrote: > s/IOThread/IO/ and align this function name with the choice of member variable > name. Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:92: void InitManagerOnIOThread(mojom::DiscardableSharedMemoryManagerPtr* manager, On 2016/11/18 05:26:50, reveman wrote: > s/IOThread/IO/ and align this function name with the choice of member variable > name. Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:99: void DeletedDiscardableSharedMemoryOnIOThread( On 2016/11/18 05:26:50, reveman wrote: > nit: s/OnIOThread/OnIO/ Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:99: void DeletedDiscardableSharedMemoryOnIOThread( On 2016/11/18 05:26:50, reveman wrote: > nit: s/OnIOThread/OnIO/ Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:102: if (!manager->is_bound()) On 2016/11/18 05:26:50, reveman wrote: > when will this happen? can we DCHECK instead? Probably when the browser is being terminated. The mojo connection is broken, the OnManagerConnectionError is called and the manager will be unbound. In that case, I think we just want to return here instead of crash. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:102: if (!manager->is_bound()) On 2016/11/18 05:26:50, reveman wrote: > when will this happen? can we DCHECK instead? Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:135: if (!io_task_runner_->DeleteSoon(FROM_HERE, manager)) On 2016/11/18 05:26:50, reveman wrote: > Can this fail in practice? In that case a wrapper function like > https://cs.chromium.org/chromium/src/storage/browser/fileapi/sandbox_file_sys... > would make it a bit cleaner. Otherwise, > > bool posted = io_task_runner_->DeleteSoon(FROM_HERE, manager_.release()); > DCHECK(posted); > > would be preferred. Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:216: auto shared_memory = On 2016/11/18 05:26:50, reveman wrote: > nit: please keep the explicit type instead of using auto. It makes it more clear > what type of object is returned and as a result makes the code easier to > understand imo. Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:368: // Waiting until IPC is finished in the IO thread. On 2016/11/18 05:26:50, reveman wrote: > nit: "...IPC has finished on the IO thread." Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:378: if (!manager_->is_bound()) On 2016/11/18 05:26:50, reveman wrote: > can this happen? DCHECK instead? When the mojo connection is broken, the |manager_| will be unbound by OnManagerConnectionError() callback. In that case, we don't want to continue. so we can return or CHECK(instead of DCHECK). https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:391: do { On 2016/11/18 05:26:50, reveman wrote: > Why this loop? I think the code would be easier to read if you just early out > with a return statement. Maybe use a ScopedClosure to ensure event->Signal() is > called. Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:394: auto handle = base::SharedMemory::NULLHandle(); On 2016/11/18 05:26:50, reveman wrote: > Can you avoid using auto here an below so it's more clear what the types we're > dealing with here are? Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:404: base::TerminateBecauseOutOfMemory(memory_size); On 2016/11/18 05:26:49, reveman wrote: > nit: base::TerminateBecauseOutOfMemory never returns so the break below is not > needed Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:67: void AllocateOnIOThread( On 2016/11/18 05:26:50, reveman wrote: > nit: s/AllocateOnIOThread/AllocateOnIO/ as it's common practice in chromium code > to just suffix functions that are supposed to execute on IO or UI threads simply > with OnIO/OnUI. See content/browser/gpu/browser_gpu_memory_buffer_manager.h for > an example. Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:72: void OnAllocateCompletedOnIOThread( On 2016/11/18 05:26:50, reveman wrote: > nit: too many "On" :) I don't think we need the On prefix here. I'd prefer > AllocatedOnIO or AllocateCompletedOnIO Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:82: std::unique_ptr<mojom::DiscardableSharedMemoryManagerPtr> manager_; On 2016/11/18 05:26:50, reveman wrote: > Can we rename this variable to have it be less confusing to have manager > instance with a manager member? manager_ipc_, manager_mojo_, just ipc_ or mojo_? > Something that allows language referring to this member to be less confusing. > Currently you have comments mentioning "manager" and it's not perfectly clear if > it refers to the instance of this class or this manager_ member. Done. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:85: std::unique_ptr<DiscardableSharedMemoryHeap> heap_; On 2016/11/18 05:26:50, reveman wrote: > why std::unique_ptr instead of allocated as part of the class as before? The destructor of |heap_| may call DeletedDiscardableSharedMemory (it posts task to IO thread) of this class, and in destructor of CDSMM we will post a task to delete |manager_| on IO thread. So we want to delete |heap_| before posting the task for deleting |manager_|.
penghuang@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: Please review changes in //content Hi John, PTAL. Thanks.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
penghuang@chromium.org changed reviewers: + avi@chromium.org - jam@chromium.org
Because jam is OOO, -jam@ and +avi@ for content// Hi Avi, PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
avi@chromium.org changed reviewers: + dcheng@chromium.org
I don't know about mojo conversion. Daniel does, and you'd need a security review anyway, so tossing it his way.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2485623002/diff/280001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:102: if (!manager->is_bound()) On 2016/11/18 at 16:28:27, Peng wrote: > On 2016/11/18 05:26:50, reveman wrote: > > when will this happen? can we DCHECK instead? > Probably when the browser is being terminated. The mojo connection is broken, the OnManagerConnectionError is called and the manager will be unbound. In that case, I think we just want to return here instead of crash. Makes sense. Thanks for explaining. https://codereview.chromium.org/2485623002/diff/280001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/280001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:85: std::unique_ptr<DiscardableSharedMemoryHeap> heap_; On 2016/11/18 at 16:28:28, Peng wrote: > On 2016/11/18 05:26:50, reveman wrote: > > why std::unique_ptr instead of allocated as part of the class as before? > > > The destructor of |heap_| may call DeletedDiscardableSharedMemory (it posts task to IO thread) of this class, and in destructor of CDSMM we will post a task to delete |manager_| on IO thread. > So we want to delete |heap_| before posting the task for deleting |manager_|. Got it. Can you add a comment where you reset this to make it clear that it needs to be done before the mojo part is deleted? https://codereview.chromium.org/2485623002/diff/340001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/340001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:131: heap_.reset(); nit: please add a short comment here explaining why this needs to be done before manager_mojo_ is deleted https://codereview.chromium.org/2485623002/diff/340001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:133: // Delete the manager on IO thread, so any pinding tasks on IO thread will be s/the manager on IO/the manager mojo on IO/? s/pinding/pending/ https://codereview.chromium.org/2485623002/diff/340001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2485623002/diff/340001/content/browser/browse... content/browser/browser_main_loop.cc:638: discardable_memory::DiscardableSharedMemoryManager::current(); Why would it be initialize on the IO thread? Ideally we'd remove DiscardableSharedMemoryManager::current() and instead pass a pointer to an instance allocated here to where it's used. That might be complicated so if we need explicit allocation to happen here then adding a CreateInstance() function is OK I think. https://codereview.chromium.org/2485623002/diff/340001/content/ppapi_plugin/p... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2485623002/diff/340001/content/ppapi_plugin/p... content/ppapi_plugin/ppapi_thread.cc:112: !command_line.HasSwitch(switches::kSingleProcess)) sorry if I asked this already but would it be possible to just init this even in the single process case? and in that way remove the need for a builder option..
https://codereview.chromium.org/2485623002/diff/340001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/340001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:131: heap_.reset(); On 2016/11/18 21:55:25, reveman wrote: > nit: please add a short comment here explaining why this needs to be done before > manager_mojo_ is deleted Done. https://codereview.chromium.org/2485623002/diff/340001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:133: // Delete the manager on IO thread, so any pinding tasks on IO thread will be On 2016/11/18 21:55:25, reveman wrote: > s/the manager on IO/the manager mojo on IO/? > s/pinding/pending/ Done. https://codereview.chromium.org/2485623002/diff/340001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2485623002/diff/340001/content/browser/browse... content/browser/browser_main_loop.cc:638: discardable_memory::DiscardableSharedMemoryManager::current(); On 2016/11/18 21:55:25, reveman wrote: > Why would it be initialize on the IO thread? Ideally we'd remove > DiscardableSharedMemoryManager::current() and instead pass a pointer to an > instance allocated here to where it's used. That might be complicated so if we > need explicit allocation to happen here then adding a CreateInstance() function > is OK I think. I just tried your suggestion. The content_browsertest crashes. Because the content_browsertest uses the kSingleProcess flag, and the content_browsertest needs the child discardable shared memory manager in childthreadimpl. https://codereview.chromium.org/2485623002/diff/340001/content/ppapi_plugin/p... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2485623002/diff/340001/content/ppapi_plugin/p... content/ppapi_plugin/ppapi_thread.cc:112: !command_line.HasSwitch(switches::kSingleProcess)) On 2016/11/18 21:55:26, reveman wrote: > sorry if I asked this already but would it be possible to just init this even in > the single process case? and in that way remove the need for a builder option.. Done
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2485623002/diff/340001/content/ppapi_plugin/p... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2485623002/diff/340001/content/ppapi_plugin/p... content/ppapi_plugin/ppapi_thread.cc:112: !command_line.HasSwitch(switches::kSingleProcess)) On 2016/11/21 15:56:39, Peng wrote: > On 2016/11/18 21:55:26, reveman wrote: > > sorry if I asked this already but would it be possible to just init this even > in > > the single process case? and in that way remove the need for a builder > option.. > > Done Sorry. I just tried it. But the content_browsertest crashes. It is because the content browsertest will set this flag, but we still need initial the client discardable shared memory manager for it. So we cannot test this flags in ChildThreadImpl.
I'd prefer if single process and multi process modes were as close as possible. ie. both used a CDSMM instance. I'd also hate to see your testing code cause unnecessary complexity. What would it take to make these tests work without this special case? It might be OK to disable these test temporarily if we know what it takes to make them work with the new code.
I'd prefer if single process and multi process modes were as close as possible. ie. both used a CDSMM instance. I'd also hate to see your testing code cause unnecessary complexity. What would it take to make these tests work without this special case? It might be OK to disable these test temporarily if we know what it takes to make them work with the new code.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi David, I have updated the CL by following your suggestion. PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/21 21:55:41, Peng wrote: > Hi David, I have updated the CL by following your suggestion. PTAL. Thanks. Hi David, Kindly ping.
Latest patch looks great. lgtm after addressing this final set of comments. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:62: class DiscardableSharedMemoryManagerProxy Please add a comment above this class describing what it's used for? The "Proxy" suffix has confused me a couple of times when reviewing this code. It's typically used on the client side while here it's used on the service side instead. What do you think about calling this DiscardableSharedMemoryManagerHost or HostDiscardableSharedMemoryManager instead? https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:253: DCHECK(g_discardable_shared_memory_manager == nullptr); nit: DCHECK(!g_discardable_shared_memory_manager) https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:259: DCHECK(!(g_discardable_shared_memory_manager == nullptr)); nit: DCHECK(g_discardable_shared_memory_manager) https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:51: static DiscardableSharedMemoryManager* current(); Please rename this to GetInstance() now that we have CreateInstance? https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:54: static void Bind(mojom::DiscardableSharedMemoryManagerRequest request); Please make this a member function instead of static. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:134: int32_t last_client_id_; nit (optional): it's more common in chromium to use next_client_id_ and post increment. I don't feel strongly about it but I guess it makes more sense as initially there's not really a "last" id..
https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:62: class DiscardableSharedMemoryManagerProxy On 2016/11/23 17:07:44, reveman wrote: > Please add a comment above this class describing what it's used for? The "Proxy" > suffix has confused me a couple of times when reviewing this code. It's > typically used on the client side while here it's used on the service side > instead. What do you think about calling this DiscardableSharedMemoryManagerHost > or HostDiscardableSharedMemoryManager instead? How about MojoDiscardableSharedMemoryManagerImpl? Done https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:253: DCHECK(g_discardable_shared_memory_manager == nullptr); On 2016/11/23 17:07:44, reveman wrote: > nit: DCHECK(!g_discardable_shared_memory_manager) It doesn't work. Because base::LazyInstance only overrides operator==. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:259: DCHECK(!(g_discardable_shared_memory_manager == nullptr)); On 2016/11/23 17:07:44, reveman wrote: > nit: DCHECK(g_discardable_shared_memory_manager) It doesn't work. Because base::LazyInstance only overrides operator==. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:51: static DiscardableSharedMemoryManager* current(); On 2016/11/23 17:07:44, reveman wrote: > Please rename this to GetInstance() now that we have CreateInstance? Done. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:54: static void Bind(mojom::DiscardableSharedMemoryManagerRequest request); On 2016/11/23 17:07:44, reveman wrote: > Please make this a member function instead of static. Done. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:134: int32_t last_client_id_; On 2016/11/23 17:07:44, reveman wrote: > nit (optional): it's more common in chromium to use next_client_id_ and post > increment. I don't feel strongly about it but I guess it makes more sense as > initially there's not really a "last" id.. Done.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/24 15:04:37, Peng wrote: > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > File components/discardable_memory/service/discardable_shared_memory_manager.cc > (right): > > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager.cc:62: > class DiscardableSharedMemoryManagerProxy > On 2016/11/23 17:07:44, reveman wrote: > > Please add a comment above this class describing what it's used for? The > "Proxy" > > suffix has confused me a couple of times when reviewing this code. It's > > typically used on the client side while here it's used on the service side > > instead. What do you think about calling this > DiscardableSharedMemoryManagerHost > > or HostDiscardableSharedMemoryManager instead? > > How about MojoDiscardableSharedMemoryManagerImpl? > > Done > > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager.cc:253: > DCHECK(g_discardable_shared_memory_manager == nullptr); > On 2016/11/23 17:07:44, reveman wrote: > > nit: DCHECK(!g_discardable_shared_memory_manager) > > It doesn't work. Because base::LazyInstance only overrides operator==. > > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager.cc:259: > DCHECK(!(g_discardable_shared_memory_manager == nullptr)); > On 2016/11/23 17:07:44, reveman wrote: > > nit: DCHECK(g_discardable_shared_memory_manager) > > It doesn't work. Because base::LazyInstance only overrides operator==. > > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > File components/discardable_memory/service/discardable_shared_memory_manager.h > (right): > > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager.h:51: > static DiscardableSharedMemoryManager* current(); > On 2016/11/23 17:07:44, reveman wrote: > > Please rename this to GetInstance() now that we have CreateInstance? > > Done. > > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager.h:54: > static void Bind(mojom::DiscardableSharedMemoryManagerRequest request); > On 2016/11/23 17:07:44, reveman wrote: > > Please make this a member function instead of static. > > Done. > > https://codereview.chromium.org/2485623002/diff/390001/components/discardable... > components/discardable_memory/service/discardable_shared_memory_manager.h:134: > int32_t last_client_id_; > On 2016/11/23 17:07:44, reveman wrote: > > nit (optional): it's more common in chromium to use next_client_id_ and post > > increment. I don't feel strongly about it but I guess it makes more sense as > > initially there's not really a "last" id.. > > Done. Hi Daniel, PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sorry for the review delay. One thing I'm wondering about... can we expand the CL description to better describe the changes we're making? I don't really understand why we need all these changes in ClientDiscardableSharedMemoryManager. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:90: manager_mojo->reset(); Why do we do this? https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:104: return; Should this be a NOTREACHED()? https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:359: size_t size, Let's use uint32_t consistently throughout, if that's possible? https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:372: event.Wait(); I wonder... should we just be using a sync mojo IPC? Also, why do we need to bounce to the IO thread for this? https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:34: base::SingleThreadTaskRunner* io_task_runner); Nit: pass this as a scoped_refptr and std::move() in the constructor. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom:7: interface DiscardableSharedMemoryManager { Where does this interface live? Which process is it running in? I guess it must be the browser process, but it would be nice to have some more comments about what this interface is for. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom:11: int32 id) => (handle<shared_buffer> memory); I know this is just porting an existing API, but I wonder if it would make sense to: - represent the handle with an opaque handle interface (similar to https://codereview.chromium.org/2503813002/) - have this return a tuple of the opaque interface + a handle to the shared_buffer - when the opaque handle is closed, free the shared memory segment. (My understanding is that with mojo, it's nice to use message pipes and objects themselves for identity, rather than integer IDs) https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:83: if (handle != base::SharedMemory::NULLHandle()) { Is it necessary to check this condition? I would expect WrapSharedMemoryHandle() to Just Work. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:475: *shared_memory_handle = base::SharedMemory::DuplicateHandle(memory->handle()); Nit: I would just DuplicateHandle up in the Proxy (then we don't need to be very careful about our handle management here) https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:137: typedef base::hash_map<int32_t, scoped_refptr<MemorySegment>> Nit: prefer using A = B; to typedef B A; https://codereview.chromium.org/2485623002/diff/390001/content/common/in_proc... File content/common/in_process_child_thread_params.cc (right): https://codereview.chromium.org/2485623002/diff/390001/content/common/in_proc... content/common/in_process_child_thread_params.cc:12: : io_runner_(io_runner), service_request_token_(service_request_token) {} Nit: I know this isn't otherwise changed, but std::move(io_runner) here https://codereview.chromium.org/2485623002/diff/390001/content/ppapi_plugin/p... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2485623002/diff/390001/content/ppapi_plugin/p... content/ppapi_plugin/ppapi_thread.cc:139: manager_ptr.PassInterface(), GetIOTaskRunner().get()); Nit: the fact that we end up rebinding this interface pointer on a different thread is an implementation detail. I would just std::move() here, and use PassInterface() inside ClientDiscardableSharedMemoryManager. https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... content/renderer/render_thread_impl.h:270: discardable_shared_memory_manager() { It looks like this is only used for testing? It might be nice to add ForTest() or something to the name. (Maybe there's no convention of something like that here, but I find it useful personally to distinguish) https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... File content/renderer/render_thread_impl_browsertest.cc (right): https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... content/renderer/render_thread_impl_browsertest.cc:472: // Busy wait for host memory usage to be reduced. Is there a way to do this? This is generally a testing anti-pattern
https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom:11: int32 id) => (handle<shared_buffer> memory); On 2016/11/25 00:07:08, dcheng wrote: > I know this is just porting an existing API, but I wonder if it would make sense > to: > > - represent the handle with an opaque handle interface (similar to > https://codereview.chromium.org/2503813002/) > - have this return a tuple of the opaque interface + a handle to the > shared_buffer > - when the opaque handle is closed, free the shared memory segment. > > (My understanding is that with mojo, it's nice to use message pipes and objects > themselves for identity, rather than integer IDs) (To be clear, it's not necessary to do that here. I'm just wondering if it would make sense to do this change)
Description was changed from ========== discardable_memory: Using mojo IPC to replace Chrome IPC BUG=654678 ========== to ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 ==========
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:90: manager_mojo->reset(); On 2016/11/25 00:07:08, dcheng wrote: > Why do we do this? When the mojo connection is broken for some reason, there could be some pending tasks which will access this mojo interface. So I think it is better to reset the interface here, and test it in pending tasks later. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:104: return; On 2016/11/25 00:07:08, dcheng wrote: > Should this be a NOTREACHED()? I think it can be reached. For example: manager_mojo could be reset when still some DeletedDiscardableSharedMemoryOnIO tasks are pending. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:359: size_t size, On 2016/11/25 00:07:08, dcheng wrote: > Let's use uint32_t consistently throughout, if that's possible? This function overrides from subclass. So can not change it to uint32_t. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:372: event.Wait(); On 2016/11/25 00:07:08, dcheng wrote: > I wonder... should we just be using a sync mojo IPC? > > Also, why do we need to bounce to the IO thread for this? This function could be call by any threads. But the mojo interface can only be used on IO thread. SO I have to use event.Wait() here. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:34: base::SingleThreadTaskRunner* io_task_runner); On 2016/11/25 00:07:08, dcheng wrote: > Nit: pass this as a scoped_refptr and std::move() in the constructor. Done. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom:7: interface DiscardableSharedMemoryManager { On 2016/11/25 00:07:08, dcheng wrote: > Where does this interface live? Which process is it running in? > > I guess it must be the browser process, but it would be nice to have some more > comments about what this interface is for. Right now, it will live in browser process. But in future, for mus+ash, it will live in the mus process. Add comments about it. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom:11: int32 id) => (handle<shared_buffer> memory); On 2016/11/25 00:08:05, dcheng wrote: > On 2016/11/25 00:07:08, dcheng wrote: > > I know this is just porting an existing API, but I wonder if it would make > sense > > to: > > > > - represent the handle with an opaque handle interface (similar to > > https://codereview.chromium.org/2503813002/) > > - have this return a tuple of the opaque interface + a handle to the > > shared_buffer > > - when the opaque handle is closed, free the shared memory segment. > > > > (My understanding is that with mojo, it's nice to use message pipes and > objects > > themselves for identity, rather than integer IDs) > > (To be clear, it's not necessary to do that here. I'm just wondering if it would > make sense to do this change) Right now, I don't know implementation detail of discardable memory. So I think it is better no do it in this CL, maybe we could revisit it later. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:83: if (handle != base::SharedMemory::NULLHandle()) { On 2016/11/25 00:07:08, dcheng wrote: > Is it necessary to check this condition? I would expect WrapSharedMemoryHandle() > to Just Work. Done. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:475: *shared_memory_handle = base::SharedMemory::DuplicateHandle(memory->handle()); On 2016/11/25 00:07:08, dcheng wrote: > Nit: I would just DuplicateHandle up in the Proxy (then we don't need to be very > careful about our handle management here) Because the handle inside of memory will be closed (line478) before returning from this function. So we have to dup it here. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:137: typedef base::hash_map<int32_t, scoped_refptr<MemorySegment>> On 2016/11/25 00:07:08, dcheng wrote: > Nit: prefer using A = B; to typedef B A; Done. https://codereview.chromium.org/2485623002/diff/390001/content/common/in_proc... File content/common/in_process_child_thread_params.cc (right): https://codereview.chromium.org/2485623002/diff/390001/content/common/in_proc... content/common/in_process_child_thread_params.cc:12: : io_runner_(io_runner), service_request_token_(service_request_token) {} On 2016/11/25 00:07:08, dcheng wrote: > Nit: I know this isn't otherwise changed, but std::move(io_runner) here Done. https://codereview.chromium.org/2485623002/diff/390001/content/ppapi_plugin/p... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/2485623002/diff/390001/content/ppapi_plugin/p... content/ppapi_plugin/ppapi_thread.cc:139: manager_ptr.PassInterface(), GetIOTaskRunner().get()); On 2016/11/25 00:07:08, dcheng wrote: > Nit: the fact that we end up rebinding this interface pointer on a different > thread is an implementation detail. I would just std::move() here, and use > PassInterface() inside ClientDiscardableSharedMemoryManager. Done. https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... content/renderer/render_thread_impl.h:270: discardable_shared_memory_manager() { On 2016/11/25 00:07:08, dcheng wrote: > It looks like this is only used for testing? It might be nice to add ForTest() > or something to the name. > > (Maybe there's no convention of something like that here, but I find it useful > personally to distinguish) Done. https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... File content/renderer/render_thread_impl_browsertest.cc (right): https://codereview.chromium.org/2485623002/diff/390001/content/renderer/rende... content/renderer/render_thread_impl_browsertest.cc:472: // Busy wait for host memory usage to be reduced. On 2016/11/25 00:07:08, dcheng wrote: > Is there a way to do this? This is generally a testing anti-pattern I don't see a easy way to avoid this busy waiting.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@chromium.org changed reviewers: + yzshen@chromium.org
+yzshen to confirm that transforming a sync IPC to an async mojo one + WaitableEvent is a safe pattern. Also, what happens if a child process tries to allocate a shared memory block on the IO thread? I think WaitableEvent won't spin the message loop, right? https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:475: *shared_memory_handle = base::SharedMemory::DuplicateHandle(memory->handle()); On 2016/11/25 16:41:53, Peng wrote: > On 2016/11/25 00:07:08, dcheng wrote: > > Nit: I would just DuplicateHandle up in the Proxy (then we don't need to be > very > > careful about our handle management here) > > Because the handle inside of memory will be closed (line478) before returning > from this function. So we have to dup it here. Hmm... I was hoping we could wrap this in sort of scoper to be a bit safer about closing the handle when it's needed... Can we just move the DuplicateHandle() to line 483 so we don't need the extra CloseHandle() call at least? https://codereview.chromium.org/2485623002/diff/430001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2485623002/diff/430001/content/browser/browse... content/browser/browser_main_loop.cc:34: #include "base/threading/sequenced_worker_pool.h" Please try to separate rebases from addressing review comments, this is already a pretty large CL.
https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:90: manager_mojo->reset(); On 2016/11/25 16:41:52, Peng wrote: > On 2016/11/25 00:07:08, dcheng wrote: > > Why do we do this? > > When the mojo connection is broken for some reason, there could be some pending > tasks which will access this mojo interface. So I think it is better to reset > the interface here, and test it in pending tasks later. Any pending tasks will just become no-ops when they try to use the mojo interface, so it's not actually a problem. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:81: std::unique_ptr<mojom::DiscardableSharedMemoryManagerPtr> manager_mojo_; Does this actually need to be in a unique_ptr?
On Mon, Nov 28, 2016 at 10:26 AM, <dcheng@chromium.org> wrote: > +yzshen to confirm that transforming a sync IPC to an async mojo one + > WaitableEvent is a safe pattern. > Jay is currently developing thread_safe_interface_ptr.h to achieve what ThreadSafeSender could do. However, sync calls are not supported yet. Please stay tuned. I am fine with using event signaling as a temporary solution, although I personally would like to avoid this pattern if at all possible. It is easy to forget signaling the event in some corner cases, for example, in ClientDiscardableSharedMemoryManager::AllocateOnIO() the code doesn't signal the event if manager_mojo_ is not bound. > > Also, what happens if a child process tries to allocate a shared memory > block on > the IO thread? I think WaitableEvent won't spin the message loop, right? > > > https://codereview.chromium.org/2485623002/diff/390001/ > components/discardable_memory/service/discardable_shared_memory_manager.cc > File > components/discardable_memory/service/discardable_shared_memory_manager.cc > (right): > > https://codereview.chromium.org/2485623002/diff/390001/ > components/discardable_memory/service/discardable_shared_ > memory_manager.cc#newcode475 > components/discardable_memory/service/discardable_shared_ > memory_manager.cc:475: > *shared_memory_handle = > base::SharedMemory::DuplicateHandle(memory->handle()); > On 2016/11/25 16:41:53, Peng wrote: > > On 2016/11/25 00:07:08, dcheng wrote: > > > Nit: I would just DuplicateHandle up in the Proxy (then we don't > need to be > > very > > > careful about our handle management here) > > > > Because the handle inside of memory will be closed (line478) before > returning > > from this function. So we have to dup it here. > > Hmm... I was hoping we could wrap this in sort of scoper to be a bit > safer about closing the handle when it's needed... > > Can we just move the DuplicateHandle() to line 483 so we don't need the > extra CloseHandle() call at least? > > https://codereview.chromium.org/2485623002/diff/430001/ > content/browser/browser_main_loop.cc > File content/browser/browser_main_loop.cc (right): > > https://codereview.chromium.org/2485623002/diff/430001/ > content/browser/browser_main_loop.cc#newcode34 > content/browser/browser_main_loop.cc:34: #include > "base/threading/sequenced_worker_pool.h" > Please try to separate rebases from addressing review comments, this is > already a pretty large CL. > > https://codereview.chromium.org/2485623002/ > -- 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.
https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:90: manager_mojo->reset(); On 2016/11/28 18:29:14, dcheng wrote: > On 2016/11/25 16:41:52, Peng wrote: > > On 2016/11/25 00:07:08, dcheng wrote: > > > Why do we do this? > > > > When the mojo connection is broken for some reason, there could be some > pending > > tasks which will access this mojo interface. So I think it is better to reset > > the interface here, and test it in pending tasks later. > > Any pending tasks will just become no-ops when they try to use the mojo > interface, so it's not actually a problem. If calling a broken or unbound interface does cause any issue. It should be fine. Remove unnecessary code. Done https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.h:81: std::unique_ptr<mojom::DiscardableSharedMemoryManagerPtr> manager_mojo_; On 2016/11/28 18:29:14, dcheng wrote: > Does this actually need to be in a unique_ptr? It has to be a std::unique_ptr<mojom::DiscardableSharedMemoryManagerPtr> or mojom::DiscardableSharedMemoryManagerPtr*, because we need access this pointer when this class has been destroyed. https://codereview.chromium.org/2485623002/diff/390001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/390001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:475: *shared_memory_handle = base::SharedMemory::DuplicateHandle(memory->handle()); On 2016/11/28 18:26:54, dcheng wrote: > On 2016/11/25 16:41:53, Peng wrote: > > On 2016/11/25 00:07:08, dcheng wrote: > > > Nit: I would just DuplicateHandle up in the Proxy (then we don't need to be > > very > > > careful about our handle management here) > > > > Because the handle inside of memory will be closed (line478) before returning > > from this function. So we have to dup it here. > > Hmm... I was hoping we could wrap this in sort of scoper to be a bit safer about > closing the handle when it's needed... > > Can we just move the DuplicateHandle() to line 483 so we don't need the extra > CloseHandle() call at least? Done.
On 2016/11/28 19:23:08, yzshen1 wrote: > On Mon, Nov 28, 2016 at 10:26 AM, <mailto:dcheng@chromium.org> wrote: > > > +yzshen to confirm that transforming a sync IPC to an async mojo one + > > WaitableEvent is a safe pattern. > > > > Jay is currently developing thread_safe_interface_ptr.h to achieve what > ThreadSafeSender could do. > However, sync calls are not supported yet. Please stay tuned. > > I am fine with using event signaling as a temporary solution, although I > personally would like to avoid this pattern if at all possible. > It is easy to forget signaling the event in some corner cases, for example, > in ClientDiscardableSharedMemoryManager::AllocateOnIO() the code doesn't > signal the event if manager_mojo_ is not bound. Thanks for the explanation. To avoid the corner case, I changed the CL to pass ClosureRunner to AllocateOnIO instead of the WaitableEvent*. > > > > > > > Also, what happens if a child process tries to allocate a shared memory > > block on > > the IO thread? I think WaitableEvent won't spin the message loop, right? > > > > > > https://codereview.chromium.org/2485623002/diff/390001/ > > components/discardable_memory/service/discardable_shared_memory_manager.cc > > File > > components/discardable_memory/service/discardable_shared_memory_manager.cc > > (right): > > > > https://codereview.chromium.org/2485623002/diff/390001/ > > components/discardable_memory/service/discardable_shared_ > > memory_manager.cc#newcode475 > > components/discardable_memory/service/discardable_shared_ > > memory_manager.cc:475: > > *shared_memory_handle = > > base::SharedMemory::DuplicateHandle(memory->handle()); > > On 2016/11/25 16:41:53, Peng wrote: > > > On 2016/11/25 00:07:08, dcheng wrote: > > > > Nit: I would just DuplicateHandle up in the Proxy (then we don't > > need to be > > > very > > > > careful about our handle management here) > > > > > > Because the handle inside of memory will be closed (line478) before > > returning > > > from this function. So we have to dup it here. > > > > Hmm... I was hoping we could wrap this in sort of scoper to be a bit > > safer about closing the handle when it's needed... > > > > Can we just move the DuplicateHandle() to line 483 so we don't need the > > extra CloseHandle() call at least? > > > > https://codereview.chromium.org/2485623002/diff/430001/ > > content/browser/browser_main_loop.cc > > File content/browser/browser_main_loop.cc (right): > > > > https://codereview.chromium.org/2485623002/diff/430001/ > > content/browser/browser_main_loop.cc#newcode34 > > content/browser/browser_main_loop.cc:34: #include > > "base/threading/sequenced_worker_pool.h" > > Please try to separate rebases from addressing review comments, this is > > already a pretty large CL. > > > > https://codereview.chromium.org/2485623002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/11/28 19:23:08, yzshen1 wrote: > On Mon, Nov 28, 2016 at 10:26 AM, <mailto:dcheng@chromium.org> wrote: > > > +yzshen to confirm that transforming a sync IPC to an async mojo one + > > WaitableEvent is a safe pattern. > > > > Jay is currently developing thread_safe_interface_ptr.h to achieve what > ThreadSafeSender could do. > However, sync calls are not supported yet. Please stay tuned. > > I am fine with using event signaling as a temporary solution, although I > personally would like to avoid this pattern if at all possible. > It is easy to forget signaling the event in some corner cases, for example, > in ClientDiscardableSharedMemoryManager::AllocateOnIO() the code doesn't > signal the event if manager_mojo_ is not bound. Thanks for the explanation. To avoid the corner case, I changed the CL to pass ClosureRunner to AllocateOnIO instead of the WaitableEvent*. > > > > > > > Also, what happens if a child process tries to allocate a shared memory > > block on > > the IO thread? I think WaitableEvent won't spin the message loop, right? > > > > > > https://codereview.chromium.org/2485623002/diff/390001/ > > components/discardable_memory/service/discardable_shared_memory_manager.cc > > File > > components/discardable_memory/service/discardable_shared_memory_manager.cc > > (right): > > > > https://codereview.chromium.org/2485623002/diff/390001/ > > components/discardable_memory/service/discardable_shared_ > > memory_manager.cc#newcode475 > > components/discardable_memory/service/discardable_shared_ > > memory_manager.cc:475: > > *shared_memory_handle = > > base::SharedMemory::DuplicateHandle(memory->handle()); > > On 2016/11/25 16:41:53, Peng wrote: > > > On 2016/11/25 00:07:08, dcheng wrote: > > > > Nit: I would just DuplicateHandle up in the Proxy (then we don't > > need to be > > > very > > > > careful about our handle management here) > > > > > > Because the handle inside of memory will be closed (line478) before > > returning > > > from this function. So we have to dup it here. > > > > Hmm... I was hoping we could wrap this in sort of scoper to be a bit > > safer about closing the handle when it's needed... > > > > Can we just move the DuplicateHandle() to line 483 so we don't need the > > extra CloseHandle() call at least? > > > > https://codereview.chromium.org/2485623002/diff/430001/ > > content/browser/browser_main_loop.cc > > File content/browser/browser_main_loop.cc (right): > > > > https://codereview.chromium.org/2485623002/diff/430001/ > > content/browser/browser_main_loop.cc#newcode34 > > content/browser/browser_main_loop.cc:34: #include > > "base/threading/sequenced_worker_pool.h" > > Please try to separate rebases from addressing review comments, this is > > already a pretty large CL. > > > > https://codereview.chromium.org/2485623002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can we use ThreadSafeInterfacePtr now that that's landed?
On 2016/11/30 06:08:15, dcheng wrote: > Can we use ThreadSafeInterfacePtr now that that's landed? As yzshen maintained, ThreadSafeInterfacePtr doesn't support sync method call, so we have to use normal InterfacePtr for this CL.
lgtm with nits https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:219: base::Unretained(this), new_id))); Nit: please document why the span is guaranteed not to outlive the manager https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:62: class MojoDiscardableSharedMemoryManagerImpl Nit: it's still worth adding a brief comment about why we indirect like this: basically, this impl class is so we can associate allocations with a given mojo instance, so if the instance is closed, we can release the allocations associated with that instance. https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:51: static DiscardableSharedMemoryManager* GetInstance(); Nit: let's just combine GetInstance() and CreateInstance(). It doesn't add much, if it's not instantiated by setting it as the DiscardableMemoryAllocator, it's going to be instantiated when we register the interface. (Otherwise, it requires a somewhat careful reading of the code to determine that the CreateInstance() call in browser_main_loop.cc doesn't leak a raw pointer. And I don't think enforcing an initialization order guarantee between browser_main_loop.cc and render_process_host_impl.cc is desirable either)
https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/client/client_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/client/client_discardable_shared_memory_manager.cc:219: base::Unretained(this), new_id))); On 2016/12/02 08:48:54, dcheng wrote: > Nit: please document why the span is guaranteed not to outlive the manager Done. https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.cc:62: class MojoDiscardableSharedMemoryManagerImpl On 2016/12/02 08:48:54, dcheng wrote: > Nit: it's still worth adding a brief comment about why we indirect like this: > basically, this impl class is so we can associate allocations with a given mojo > instance, so if the instance is closed, we can release the allocations > associated with that instance. Done. https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:51: static DiscardableSharedMemoryManager* GetInstance(); We added this CreateInstance(), because we need create DiscardableSharedMemoryManager in main thread explicitly. Otherwise, the instance could be created in IO thread, and some initializing code will use blocking IO which is not allowed in IO thread. So reveman suggested adding this CreateInstance() method. On 2016/12/02 08:48:54, dcheng wrote: > Nit: let's just combine GetInstance() and CreateInstance(). It doesn't add much, > if it's not instantiated by setting it as the DiscardableMemoryAllocator, it's > going to be instantiated when we register the interface. > > (Otherwise, it requires a somewhat careful reading of the code to determine that > the CreateInstance() call in browser_main_loop.cc doesn't leak a raw pointer. > And I don't think enforcing an initialization order guarantee between > browser_main_loop.cc and render_process_host_impl.cc is desirable either)
penghuang@chromium.org changed reviewers: + sky@chromium.org
+sky for content/ppapi_plugin/DEPS Hi sky, PTAL, Thanks.
On 2016/11/18 19:11:20, Avi wrote: > I don't know about mojo conversion. Daniel does, and you'd need a security > review anyway, so tossing it his way. Hi Avi, Daniel just LGTMed. Could you please take a look? Thanks.
On 2016/11/18 19:11:20, Avi wrote: > I don't know about mojo conversion. Daniel does, and you'd need a security > review anyway, so tossing it his way. Hi Avi, Daniel just LGTMed. Could you please take a look? Thanks.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
stamping Daniel's lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2485623002/#ps490001 (title: "Address review issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 490001, "attempt_start_ts": 1480700766318980,
"parent_rev": "ab1d442c8295011ba163fc4a98ddd22b14ef2adb", "commit_rev":
"e8945319ded00d5a1c9857d2310c43d76cfc93c3"}
(Also, sorry I forgot to include this previously, but should we include a TODO to switch to ThreadSafeInterfacePtr if it starts supporting sync calls?) https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:51: static DiscardableSharedMemoryManager* GetInstance(); On 2016/12/02 15:27:36, Peng wrote: > We added this CreateInstance(), because we need create > DiscardableSharedMemoryManager in main thread explicitly. Otherwise, the > instance could be created in IO thread, and some initializing code will use > blocking IO which is not allowed in IO thread. > > So reveman suggested adding this CreateInstance() method. > > On 2016/12/02 08:48:54, dcheng wrote: > > Nit: let's just combine GetInstance() and CreateInstance(). It doesn't add > much, > > if it's not instantiated by setting it as the DiscardableMemoryAllocator, it's > > going to be instantiated when we register the interface. > > > > (Otherwise, it requires a somewhat careful reading of the code to determine > that > > the CreateInstance() call in browser_main_loop.cc doesn't leak a raw pointer. > > And I don't think enforcing an initialization order guarantee between > > browser_main_loop.cc and render_process_host_impl.cc is desirable either) > Where is the blocking IO happening?
Message was sent while issue was closed.
Description was changed from ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 ========== to ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:490001)
Message was sent while issue was closed.
Description was changed from ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 ========== to ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Cr-Commit-Position: refs/heads/master@{#435969} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Cr-Commit-Position: refs/heads/master@{#435969}
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:490001) has been created in https://codereview.chromium.org/2545523007/ by rockot@chromium.org. The reason for reverting is: Causing some failures. Example: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac.
Message was sent while issue was closed.
Description was changed from ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Cr-Commit-Position: refs/heads/master@{#435969} ========== to ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Cr-Commit-Position: refs/heads/master@{#435969} ==========
https://codereview.chromium.org/2485623002/diff/470001/components/discardable... File components/discardable_memory/service/discardable_shared_memory_manager.h (right): https://codereview.chromium.org/2485623002/diff/470001/components/discardable... components/discardable_memory/service/discardable_shared_memory_manager.h:51: static DiscardableSharedMemoryManager* GetInstance(); On 2016/12/02 17:51:15, dcheng wrote: > On 2016/12/02 15:27:36, Peng wrote: > > We added this CreateInstance(), because we need create > > DiscardableSharedMemoryManager in main thread explicitly. Otherwise, the > > instance could be created in IO thread, and some initializing code will use > > blocking IO which is not allowed in IO thread. > > > > So reveman suggested adding this CreateInstance() method. > > > > On 2016/12/02 08:48:54, dcheng wrote: > > > Nit: let's just combine GetInstance() and CreateInstance(). It doesn't add > > much, > > > if it's not instantiated by setting it as the DiscardableMemoryAllocator, > it's > > > going to be instantiated when we register the interface. > > > > > > (Otherwise, it requires a somewhat careful reading of the code to determine > > that > > > the CreateInstance() call in browser_main_loop.cc doesn't leak a raw > pointer. > > > And I don't think enforcing an initialization order guarantee between > > > browser_main_loop.cc and render_process_host_impl.cc is desirable either) > > > > Where is the blocking IO happening? https://cs.chromium.org/chromium/src/components/discardable_memory/service/di... I remember the GetDefaultMemoryLimit() caused the problem.
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, avi@chromium.org, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2485623002/#ps510001 (title: "Fix bot issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 510001, "attempt_start_ts": 1480704692429240,
"parent_rev": "57f5491f820e1708db8f621aade3400ac2980ba0", "commit_rev":
"0896159fd14b1d4f23187a4462558532eb637ba7"}
Message was sent while issue was closed.
Description was changed from ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Cr-Commit-Position: refs/heads/master@{#435969} ========== to ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Cr-Commit-Position: refs/heads/master@{#435969} ==========
Message was sent while issue was closed.
Committed patchset #24 (id:510001)
Message was sent while issue was closed.
Description was changed from ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Cr-Commit-Position: refs/heads/master@{#435969} ========== to ========== discardable_memory: Using mojo IPC to replace Chrome IPC It also moves ClientDiscardableSharedMemoryManager from ChildThreadImpl to RenderThreadImpl and PpapiThread BUG=654678 Committed: https://crrev.com/a083f971261bcccc53b64e40c911435857ce3bfd Committed: https://crrev.com/342762b5a769ad9ed321ded9254bde4b44a2450b Cr-Original-Commit-Position: refs/heads/master@{#435969} Cr-Commit-Position: refs/heads/master@{#436010} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/342762b5a769ad9ed321ded9254bde4b44a2450b Cr-Commit-Position: refs/heads/master@{#436010} |
