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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by erikchen
Modified:
2 months 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.
2 months, 1 week 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.
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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, ...
2 months, 1 week 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: > > > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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: ...
2 months, 1 week 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, ...
2 months, 1 week 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. > ...
2 months, 1 week ago (2017-05-15 17:50:29 UTC) #44
Avi (ping after 24h)
lgtm content stampity stamp
2 months, 1 week ago (2017-05-15 17:52:43 UTC) #45
Primiano Tucci
LGTM
2 months, 1 week 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
2 months, 1 week 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
2 months, 1 week 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)
2 months, 1 week 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
2 months, 1 week 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
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-05-16 04:58:22 UTC) #62
rkaplow
lgtm
2 months 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
2 months 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)
2 months 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
2 months 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
2 months 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 ...
2 months 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 ...
2 months 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: > > ...
2 months ago (2017-05-18 14:01:58 UTC) #82
erikchen
2 months 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 25c286973