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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks ago by erikchen
Modified:
1 week, 1 day 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 83 (51 generated)
erikchen
primiano: Please review.
1 week, 6 days 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 week, 6 days 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 week, 6 days 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 week, 5 days ago (2017-05-14 04:49:17 UTC) #16
Primiano Tucci
removing that NOTREACHED(); smells quite bad. Also, see comments below, I think you have a ...
1 week, 5 days 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 week, 4 days ago (2017-05-15 06:30:55 UTC) #29
Primiano Tucci
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 week, 4 days ago (2017-05-15 07:12:36 UTC) #30
Primiano Tucci
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 week, 4 days 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 week, 4 days ago (2017-05-15 16:35:40 UTC) #32
Primiano Tucci
On 2017/05/15 16:35:40, DmitrySkiba wrote: > On 2017/05/15 07:12:36, Primiano Tucci wrote: > > On ...
1 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 4 days ago (2017-05-15 17:50:29 UTC) #44
Avi (ping after 24h)
lgtm content stampity stamp
1 week, 4 days ago (2017-05-15 17:52:43 UTC) #45
Primiano Tucci
LGTM
1 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 4 days 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 week, 3 days ago (2017-05-16 04:58:22 UTC) #62
rkaplow
lgtm
1 week, 2 days 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 week, 2 days 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 week, 2 days 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 week, 2 days 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 week, 1 day ago (2017-05-18 04:22:01 UTC) #79
Marc Treib (OOO till June 7)
On 2017/05/18 04:22:01, commit-bot: I haz the power wrote: > Committed patchset #13 (id:240001) as ...
1 week, 1 day ago (2017-05-18 13:54:25 UTC) #80
Marc Treib (OOO till June 7)
On 2017/05/18 13:54:25, Marc Treib wrote: > On 2017/05/18 04:22:01, commit-bot: I haz the power ...
1 week, 1 day ago (2017-05-18 13:59:18 UTC) #81
Marc Treib (OOO till June 7)
On 2017/05/18 13:59:18, Marc Treib wrote: > On 2017/05/18 13:54:25, Marc Treib wrote: > > ...
1 week, 1 day ago (2017-05-18 14:01:58 UTC) #82
erikchen
1 week, 1 day 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06