New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 733165 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 732927



Sign in to add a comment

memory-infra service: Remove special shortcut for the registration of the browser process

Project Member Reported by primiano@chromium.org, Jun 14 2017

Issue description

Today the browser process registers to the memory-infra service in a special way [1].
It leverages the knowledge that the browser process is the same that hosts the memory-infra service, and bypasses the Mojo IPCs.
This causes the code in coordinator_impl.cc to behave in weird ways, because the messages received from the browser have no identity (see  Issue 732927 ).
There doesn't seem to be any strong need of this (seems a premature optimization) and as such I'm going to ditch it soon to make everything easier to reason about.


[1] https://cs.chromium.org/chromium/src/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc?rcl=9b2b6a840fc7c88f5c46b5a32d8d844d778f35e4&l=80

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31ac3b9be966893fc9b385d86ab61456ec1f9b74

commit 31ac3b9be966893fc9b385d86ab61456ec1f9b74
Author: Primiano Tucci <primiano@chromium.org>
Date: Thu Jun 22 06:07:50 2017

Memory-infra: introduce helper class to interact with service

This CL:
1) Introduces a helper memory_instrumentation.h/.cc to
   interact with the memory-infra service.
   See design discussion on services-dev
   (https://goo.gl/ywieN0).
2) Switches the tracing-related clients of memory-infra
   (devtools and slow reports) to use 1. 
3) Remove the special shortcut that bypasses mojo when
   registering the memory-infra client for the browser
   process itself ( crbug.com/733165 ).
4) Remove a useless and hard to maintain test 
   client_process_impl_unittest.cc

Upcoming CLs will get rid of the
base::MemoryDumpManager::RequestGlobalDump() method and cleanup
both the mojo interface and the ClientProcess library.

Why this CL doesn't switch also chrome_metrics_emitter?
Some small changes are coming next to the .mojom interface to
have a clear split between the API to request a memory dump and
add to the trace, and the API to request a memory dump and
return back the results (e.g., for UMA). An API for the
latter will be introduced in this new helper class after
that refactoring has happened.

Why removing the client_process_impl_unittest.cc?
Right now that test covers a special test-only code-path
of client_process_impl.cc against a mocked coordinator_impl.cc.
The actual production code being tested is nearly zero.
In addition, this class is more realistically covered by the
process_memory_metrics_emitter_browsertest.cc.
A more sensible end-to-end test to cover the memory-infra
service will follow in upcoming CLs.

BUG= 733165 
TBR=asvitkine

Change-Id: I8b989e7fe17b6643ae5e2d733d4d6484b18ff507
Reviewed-on: https://chromium-review.googlesource.com/536894
Commit-Queue: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Hector Dearman <hjd@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481459}
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/chrome/test/base/tracing_browsertest.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/content/browser/browser_main_loop.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/content/browser/devtools/protocol/tracing_handler.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/content/browser/devtools/protocol/tracing_handler.h
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/content/browser/tracing/background_memory_tracing_observer.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/BUILD.gn
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/memory_instrumentation/coordinator_impl.h
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/public/cpp/BUILD.gn
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc
[modify] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h
[delete] https://crrev.com/4d77c849b825d8520ec86ec053d0962607d7fab9/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl_unittest.cc
[add] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
[add] https://crrev.com/31ac3b9be966893fc9b385d86ab61456ec1f9b74/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/60345b63eb89379e689347ff01d7c8b52cfaa18a

commit 60345b63eb89379e689347ff01d7c8b52cfaa18a
Author: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Date: Wed Sep 13 17:10:15 2017

move memory_infra and tracing to the GRC service

memory_instrumentation::Coordinator is exposed by the browser service
right now, which was a temporary solution before the
resource_coordinator service was created. This CL moves it to the
resource_coordinator service.

Also, this CL exposes the tracing service, too. Later it will be used
instead of the tracing controller that lives in the browser.

Moving memory instrumentation and tracing to the resource_coordinator
service will let the service manager to register all "service"
processes to report memory usage and trace data. This is not possible
if memory instrumentation and tracing live in content_browser since it
will be a layering violation to have a dependency from
//services/service_manager to content.

BUG= 687020 , 640235 , 733165 

Change-Id: I7ea9e61430f4406fd115abd747b35a806a392469
Reviewed-on: https://chromium-review.googlesource.com/628658
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501668}
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/chrome/browser/metrics/process_memory_metrics_emitter.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/content/browser/browser_main_loop.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/content/browser/browser_main_loop.h
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/content/browser/service_manager/common_browser_interfaces.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/content/child/child_thread_impl.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/content/public/app/mojo/content_packaged_services_manifest.json
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/content/public/app/mojo/content_renderer_manifest.json
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/BUILD.gn
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/coordination_unit_introspector_impl.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/coordination_unit_introspector_impl.h
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/coordination_unit_manager.h
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/coordination_unit_provider_impl.h
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/tab_signal_generator_impl.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/coordination_unit/tab_signal_generator_impl.h
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/manifest.json
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/memory_instrumentation/coordinator_impl.h
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/resource_coordinator_service.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/resource_coordinator_service.h
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/service_callbacks_impl.cc
[modify] https://crrev.com/60345b63eb89379e689347ff01d7c8b52cfaa18a/services/resource_coordinator/service_callbacks_impl.h

Cc: chiniforooshan@chromium.org
Didn't Primiano's CL, referenced in comment #1, fix this?
Status: Fixed (was: Started)
> Didn't Primiano's CL, referenced in comment #1, fix this?
Yes, thanks for the reminder :)

Sign in to add a comment