Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(774)

Issue 2883693002: [Memory-UMA] Implement basic working prototype. (Closed)

Created:
1 year, 1 month ago by erikchen
Modified:
1 year, 1 month ago
CC:
chromium-reviews, chrome-grc-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, darin (slow to review)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[reland 1] [Memory-UMA] Implement basic working prototype. The first attempt to land this CL had test failures on ChromeOS for the newly added tests. The tests have been updated to also work on ChromeOS. > This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. > > This CL includes the following changes: > * Fix histogram emissions to not be off by a factor of 2 ** 20. > * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the > scope of end-to-end tests to sanity check UMA emissions. > * Change the location where the object that backs the memory instrumentation > interface is created, as it now requires Mojo to be initialized. > * The implementation of the memory instrumentation interface now requires > ServiceManagerListener, so add that requirement to > content_browser_manifest.json. > * Create a ProcessMap class that maps service identifiers to pids. > * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. > > BUG=703184 > TBR=rkaplow@chromium.org > > Review-Url: https://codereview.chromium.org/2883693002 > Cr-Commit-Position: refs/heads/master@{#471964} > Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55898f5df1b4d2 BUG=703184 Review-Url: https://codereview.chromium.org/2883693002 Cr-Commit-Position: refs/heads/master@{#472659} Committed: https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc1c72f683c16d

Patch Set 1 : Cloned from https://codereview.chromium.org/2875673003/#ps80001. #

Patch Set 2 : tests are still wip #

Patch Set 3 : Tests working. #

Total comments: 20

Patch Set 4 : comemnts from dcheng. #

Patch Set 5 : clean up #

Patch Set 6 : disable test on ASAN. #

Patch Set 7 : Respecify dependent CL, which had somehow gotten lost. #

Total comments: 28

Patch Set 8 : Comments from primiano. #

Patch Set 9 : fix macro. #

Patch Set 10 : Disable test on ASAN. #

Patch Set 11 : Fix tests for ChromeOS. #

Patch Set 12 : Fix tests #

Patch Set 13 : Fix test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+485 lines, -56 lines) Patch
M chrome/browser/metrics/process_memory_metrics_emitter.cc View 1 2 3 4 5 6 7 8 1 chunk +18 lines, -15 lines 0 comments Download
M chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +187 lines, -5 lines 0 comments Download
M content/browser/browser_main_loop.cc View 1 2 3 4 5 6 7 2 chunks +9 lines, -8 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M services/resource_coordinator/BUILD.gn View 1 2 chunks +3 lines, -0 lines 0 comments Download
M services/resource_coordinator/memory/coordinator/coordinator_impl.h View 1 2 3 4 5 6 7 6 chunks +20 lines, -5 lines 0 comments Download
M services/resource_coordinator/memory/coordinator/coordinator_impl.cc View 1 2 3 4 5 6 7 10 chunks +37 lines, -21 lines 0 comments Download
M services/resource_coordinator/memory/coordinator/coordinator_impl_unittest.cc View 1 2 3 4 5 6 7 1 chunk +10 lines, -1 line 0 comments Download
A services/resource_coordinator/memory/coordinator/process_map.h View 1 2 3 4 5 6 7 1 chunk +61 lines, -0 lines 0 comments Download
A services/resource_coordinator/memory/coordinator/process_map.cc View 1 2 3 4 5 6 7 1 chunk +68 lines, -0 lines 0 comments Download
A services/resource_coordinator/memory/coordinator/process_map_unittest.cc View 1 2 3 4 5 6 7 1 chunk +64 lines, -0 lines 0 comments Download
M services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 3 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 83 (51 generated)
erikchen
primiano: Please review.
1 year, 1 month ago (2017-05-13 18:44:26 UTC) #5
erikchen
dcheng: Please review the 1-line change at content/public/app/mojo/content_browser_manifest.json.
1 year, 1 month ago (2017-05-13 18:46:21 UTC) #7
dcheng
LGTM with some random nits https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc File chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc (right): https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc#newcode158 chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:158: scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter( auto emitter ...
1 year, 1 month ago (2017-05-13 20:22:38 UTC) #11
erikchen
https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc File chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc (right): https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc#newcode158 chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:158: scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter( On 2017/05/13 20:22:37, dcheng wrote: > auto ...
1 year, 1 month ago (2017-05-14 04:49:17 UTC) #16
Primiano Tucci (use gerrit)
removing that NOTREACHED(); smells quite bad. Also, see comments below, I think you have a ...
1 year, 1 month ago (2017-05-15 03:48:10 UTC) #27
DmitrySkiba
https://codereview.chromium.org/2883693002/diff/120001/services/resource_coordinator/memory/coordinator/coordinator_impl.cc File services/resource_coordinator/memory/coordinator/coordinator_impl.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coordinator/memory/coordinator/coordinator_impl.cc#newcode154 services/resource_coordinator/memory/coordinator/coordinator_impl.cc:154: process_manager.get(), On 2017/05/15 03:48:09, Primiano Tucci wrote: > This ...
1 year, 1 month ago (2017-05-15 06:30:55 UTC) #29
Primiano Tucci (use gerrit)
On 2017/05/15 06:30:55, DmitrySkiba wrote: > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coordinator/memory/coordinator/coordinator_impl.cc > File services/resource_coordinator/memory/coordinator/coordinator_impl.cc > (right): > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coordinator/memory/coordinator/coordinator_impl.cc#newcode154 ...
1 year, 1 month ago (2017-05-15 07:12:36 UTC) #30
Primiano Tucci (use gerrit)
taking back some comments, will cleanup later anyways https://codereview.chromium.org/2883693002/diff/120001/services/resource_coordinator/memory/coordinator/coordinator_impl.h File services/resource_coordinator/memory/coordinator/coordinator_impl.h (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coordinator/memory/coordinator/coordinator_impl.h#newcode67 services/resource_coordinator/memory/coordinator/coordinator_impl.h:67: std::pair<mojom::ProcessLocalDumpManagerPtr, ...
1 year, 1 month ago (2017-05-15 16:35:05 UTC) #31
DmitrySkiba
On 2017/05/15 07:12:36, Primiano Tucci wrote: > On 2017/05/15 06:30:55, DmitrySkiba wrote: > > > ...
1 year, 1 month ago (2017-05-15 16:35:40 UTC) #32
Primiano Tucci (use gerrit)
On 2017/05/15 16:35:40, DmitrySkiba wrote: > On 2017/05/15 07:12:36, Primiano Tucci wrote: > > On ...
1 year, 1 month ago (2017-05-15 16:39:41 UTC) #33
DmitrySkiba
On 2017/05/15 16:39:41, Primiano Tucci wrote: > On 2017/05/15 16:35:40, DmitrySkiba wrote: > > On ...
1 year, 1 month ago (2017-05-15 16:54:07 UTC) #34
erikchen
primiano: PTAL https://codereview.chromium.org/2883693002/diff/120001/base/trace_event/memory_allocator_dump.cc File base/trace_event/memory_allocator_dump.cc (left): https://codereview.chromium.org/2883693002/diff/120001/base/trace_event/memory_allocator_dump.cc#oldcode89 base/trace_event/memory_allocator_dump.cc:89: NOTREACHED(); On 2017/05/15 03:48:09, Primiano Tucci wrote: ...
1 year, 1 month ago (2017-05-15 17:29:15 UTC) #38
erikchen
TBR avi for moving code around in content/browser/browser_main_loop.cc. TBR rkaplow for tiny changes to chrome/browser/metrics/process_memory_metrics_emitter.cc, ...
1 year, 1 month ago (2017-05-15 17:48:18 UTC) #42
erikchen
On 2017/05/15 17:48:18, erikchen wrote: > TBR avi for moving code around in content/browser/browser_main_loop.cc. > ...
1 year, 1 month ago (2017-05-15 17:50:29 UTC) #44
Avi (use Gerrit)
lgtm content stampity stamp
1 year, 1 month ago (2017-05-15 17:52:43 UTC) #45
Primiano Tucci (use gerrit)
LGTM
1 year, 1 month ago (2017-05-15 18:02:56 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2883693002/160001
1 year, 1 month ago (2017-05-15 18:06:19 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2883693002/180001
1 year, 1 month ago (2017-05-15 20:48:30 UTC) #53
commit-bot: I haz the power
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_ng/builds/444957)
1 year, 1 month ago (2017-05-15 23:08:32 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2883693002/180001
1 year, 1 month ago (2017-05-15 23:34:28 UTC) #57
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55898f5df1b4d2
1 year, 1 month ago (2017-05-16 01:06:30 UTC) #60
findit-for-me
Findit (https://goo.gl/kROfz5) identified this CL at revision 471964 as the culprit for failures in the ...
1 year, 1 month ago (2017-05-16 02:47:36 UTC) #61
tapted
On 2017/05/16 02:47:36, findit-for-me wrote: > Findit (https://goo.gl/kROfz5) identified this CL at revision 471964 as ...
1 year, 1 month ago (2017-05-16 04:58:22 UTC) #62
rkaplow
lgtm
1 year, 1 month ago (2017-05-17 14:13:44 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2883693002/240001
1 year, 1 month ago (2017-05-17 20:04:56 UTC) #72
commit-bot: I haz the power
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_ng/builds/447560)
1 year, 1 month ago (2017-05-17 23:43:25 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2883693002/240001
1 year, 1 month ago (2017-05-18 01:24:05 UTC) #76
commit-bot: I haz the power
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc1c72f683c16d
1 year, 1 month ago (2017-05-18 04:22:01 UTC) #79
Marc Treib
On 2017/05/18 04:22:01, commit-bot: I haz the power wrote: > Committed patchset #13 (id:240001) as ...
1 year, 1 month ago (2017-05-18 13:54:25 UTC) #80
Marc Treib
On 2017/05/18 13:54:25, Marc Treib wrote: > On 2017/05/18 04:22:01, commit-bot: I haz the power ...
1 year, 1 month ago (2017-05-18 13:59:18 UTC) #81
Marc Treib
On 2017/05/18 13:59:18, Marc Treib wrote: > On 2017/05/18 13:54:25, Marc Treib wrote: > > ...
1 year, 1 month ago (2017-05-18 14:01:58 UTC) #82
erikchen
1 year, 1 month ago (2017-05-18 17:03:47 UTC) #83
Message was sent while issue was closed.
On 2017/05/18 14:01:58, Marc Treib wrote:
> On 2017/05/18 13:59:18, Marc Treib wrote:
> > On 2017/05/18 13:54:25, Marc Treib wrote:
> > > On 2017/05/18 04:22:01, commit-bot: I haz the power wrote:
> > > > Committed patchset #13 (id:240001) as
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc...
> > > 
> > > The new tests fail on MSan bots:
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%...
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom...
> > > 
> > > For some reason Rietveld doesn't want to revert ("file too large"?!), so
> I'll
> > > revert manually.
> > 
> > ...or I won't, since the revert already doesn't apply cleanly anymore. I
guess
> > I'll just disable the tests on MSan...
> 
> CL to disable the tests on MSan: https://codereview.chromium.org/2891063002/

Thanks - I took a look at the failures and it looks like disabling the test for
MSAN is the right behavior. The failures are because we're failing to fetch
memory related metrics, which is expected in MSAN.

Powered by Google App Engine
This is Rietveld 408576698