Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1383733002:
[tracing] Add graphics memory dump provider for Android (Closed)
|
Created:
5 years, 2 months ago by Primiano Tucci (use gerrit) Modified:
5 years, 2 months ago CC:
ericrk, chromium-reviews, darin-cc_chromium.org, jam, perezju, piman+watch_chromium.org, reveman, ssid, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Add graphics memory dump provider for Android
This CL introduces a memory dump provider able to gather GPU/Graphics
data on Android coming from libmemtrack.
libmemtrack is an API which allows different GPU vendors to expose
memory statistics about graphical memory usage. Due to the technical
design of this API (libmemtrack requires root) the architecture of this
dump provider is decoupled in a helper service (crrev.com/1383583002)
which needs to be sideloaded manually on the device (will be automated
in telemetry scripts).
The helper service exposes the memory stats via a UNIX IPC socket.
This memory dump provider connects to the socket and receives the
memory numbers for the current process.
Design-doc: https://goo.gl/4Y30p9
Screenshot: https://goo.gl/Z27djX
BUG=529451
Committed: https://crrev.com/b3fb641552ace46ce637f4345ac8809cbfbe0059
Cr-Commit-Position: refs/heads/master@{#354032}
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : CHECK -> return false + nits #Patch Set 4 : Rebase #
Total comments: 16
Patch Set 5 : Re rsesek@ #
Total comments: 2
Patch Set 6 : Remove test friends lines #
Total comments: 1
Patch Set 7 : Add memtrack to column name #
Messages
Total messages: 37 (12 generated)
Patchset #2 (id:20001) has been deleted
primiano@chromium.org changed reviewers: + petrcermak@chromium.org
+petrcermak for a first round. +ericrk/reveman FYI (see screenshot for the final effect)
ericrk@chromium.org changed reviewers: + ericrk@chromium.org
Looks good! Excited this came together so fast. https://codereview.chromium.org/1383733002/diff/40001/base/trace_event/proces... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1383733002/diff/40001/base/trace_event/proces... base/trace_event/process_memory_dump.h:67: MemoryAllocatorDump* GetOrCreateAllocatorDump( Can we remove the similar function from SkTraceMemoryDump_chrome.h? As a separate CL is fine https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... File components/tracing/graphics_memory_dump_provider_android.cc (right): https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:57: // not an (untrusted) user process. Hard crash otherwise. Doesn't this let a user process masquerading as our memory tracker hard crash the browser/gpu processes reliably on trace? Probably not a concern if trace is dev only, as a dev would like to know there's something weird on their machine, but if we ever run memory traces in an automated way on regular users chrome instances, this seems like it could allow an evil non-root app to crash us? https://codereview.chromium.org/1383733002/diff/40001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1383733002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:1177: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( Do we need to register this in both Browser and GPU processes to handle the in-proc GPU case? why is this needed at all in the browser proc?
Looks good overall. Just a few comments. One description nit: Drop "that" in "The helper service that exposes the memory stats via a UNIX IPC socket." (i.e. it should be just "The helper service exposes the memory stats via a UNIX IPC socket."). Thanks, Petr https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... File components/tracing/graphics_memory_dump_provider_android.cc (right): https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:57: // not an (untrusted) user process. Hard crash otherwise. On 2015/10/02 18:30:11, ericrk wrote: > Doesn't this let a user process masquerading as our memory tracker hard crash > the browser/gpu processes reliably on trace? Probably not a concern if trace is > dev only, as a dev would like to know there's something weird on their machine, > but if we ever run memory traces in an automated way on regular users chrome > instances, this seems like it could allow an evil non-root app to crash us? +1. Why don't you just return false like in the rest of the cases? https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:104: return true; Strictly speaking, shouldn't you check that at least one value was added? It seems that a response " " would cause this method to return true. https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... File components/tracing/graphics_memory_dump_provider_android.h (right): https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.h:11: #include "base/memory/scoped_ptr.h" I don't think you need this include.
https://codereview.chromium.org/1383733002/diff/40001/base/trace_event/proces... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1383733002/diff/40001/base/trace_event/proces... base/trace_event/process_memory_dump.h:67: MemoryAllocatorDump* GetOrCreateAllocatorDump( On 2015/10/02 18:30:11, ericrk wrote: > Can we remove the similar function from SkTraceMemoryDump_chrome.h? As a > separate CL is fine OK I'll do that in a separate CL (at this point before this, so this becomes smaller) https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... File components/tracing/graphics_memory_dump_provider_android.cc (right): https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:57: // not an (untrusted) user process. Hard crash otherwise. On 2015/10/05 08:29:27, petrcermak wrote: > On 2015/10/02 18:30:11, ericrk wrote: > > Doesn't this let a user process masquerading as our memory tracker hard crash > > the browser/gpu processes reliably on trace? Probably not a concern if trace > is > > dev only, as a dev would like to know there's something weird on their > machine, > > but if we ever run memory traces in an automated way on regular users chrome > > instances, this seems like it could allow an evil non-root app to crash us? > > +1. Why don't you just return false like in the rest of the cases? Makes sense. Done. https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:104: return true; On 2015/10/05 08:29:27, petrcermak wrote: > Strictly speaking, shouldn't you check that at least one value was added? It > seems that a response " " would cause this method to return true. Yeah but this seems a problem with the helper daemon. In that case it would be more useful to see an empty dump in the trace (Which suggests that the data source is bad) than hiding that completely. In essence: once I establish a socket connection I expect the helper daemon to behave. https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... File components/tracing/graphics_memory_dump_provider_android.h (right): https://codereview.chromium.org/1383733002/diff/40001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.h:11: #include "base/memory/scoped_ptr.h" On 2015/10/05 08:29:27, petrcermak wrote: > I don't think you need this include. Done. https://codereview.chromium.org/1383733002/diff/40001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1383733002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:1177: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( On 2015/10/02 18:30:11, ericrk wrote: > Do we need to register this in both Browser and GPU processes to handle the > in-proc GPU case? why is this needed at all in the browser proc? In the standard (non-in-proc) case I see that both browser and gpu process provide data to meminfo. Are you suggesting that we only need one dumper at max, in the process that hosts the gpu process?
lgtm https://codereview.chromium.org/1383733002/diff/40001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1383733002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:1177: base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider( On 2015/10/05 at 14:24:25, Primiano Tucci wrote: > On 2015/10/02 18:30:11, ericrk wrote: > > Do we need to register this in both Browser and GPU processes to handle the > > in-proc GPU case? why is this needed at all in the browser proc? > In the standard (non-in-proc) case I see that both browser and gpu process provide data to meminfo. Are you suggesting that we only need one dumper at max, in the process that hosts the gpu process? That's what I was thinking, but now that I think of it, we may need something in the browser proc to catch allocations the Android system makes on our behalf? Not 100% sure - I don't think this can hurt much, so please don't let this hold up the CL. I'd like to see how these two numbers differ once we get this in.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... File components/tracing/graphics_memory_dump_provider_android.cc (right): https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:34: sock = socket(AF_UNIX, SOCK_SEQPACKET, 0); Consider using base/sync_socket.h, since it has RAII semantics. |sock| is leaked in several early-return conditions. At minimum, create a local ScopedGeneric for this. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:38: // Set recv timeout to 250ms nit: punctuation. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:69: snprintf(buf, sizeof(buf) - 1, "%d", getpid()); base::IntToString() and then c_str() and size() + 1 for the send()? https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:70: if (send(sock, buf, strlen(buf) + 1, 0) <= 0) What about EINTR? https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:78: if (recv(sock, buf, sizeof(buf), 0) <= 0) EINTR again. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:84: std::stringstream ss(std::string(buf, sizeof(buf))); The char buf -> std::string copy can be avoided by declaring |buf| to be a std::string and using ::resize() to allocate the space. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:87: uint64 value; Use the stdint.h types (uint64_t) rather than the deprecated ones in basictypes.h. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:93: if (key_split_point > 0 && key_split_point < row_name.size() - 1) { This will always be >0 since it's size_t (unsigned). Check against std::string::npos instead.
Super thanks Robert for the thorough review! See answers inline. Note: I'm adding a unittest to this as part of a separate CL (crrev.com/1396313002) because, as usual, that requires some gyp/gn dance to add a single unittest :-/. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... File components/tracing/graphics_memory_dump_provider_android.cc (right): https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:34: sock = socket(AF_UNIX, SOCK_SEQPACKET, 0); On 2015/10/09 17:22:21, Robert Sesek wrote: > Consider using base/sync_socket.h, since it has RAII semantics. |sock| is leaked > in several early-return conditions. Oh, excellent point. Actually turns out I can't just use sync_socket as that cannot adopt a random socket. > At minimum, create a local ScopedGeneric for this. I think what I want here is ScopedFD. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:38: // Set recv timeout to 250ms On 2015/10/09 17:22:21, Robert Sesek wrote: > nit: punctuation. Done. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:69: snprintf(buf, sizeof(buf) - 1, "%d", getpid()); On 2015/10/09 17:22:21, Robert Sesek wrote: > base::IntToString() and then c_str() and size() + 1 for the send()? Hmm but that would make another copy to construct the std::String that I don't really need (note I reuse the |buf| below). https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:70: if (send(sock, buf, strlen(buf) + 1, 0) <= 0) On 2015/10/09 17:22:21, Robert Sesek wrote: > What about EINTR? another excellent point https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:78: if (recv(sock, buf, sizeof(buf), 0) <= 0) On 2015/10/09 17:22:21, Robert Sesek wrote: > EINTR again. Done. https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:84: std::stringstream ss(std::string(buf, sizeof(buf))); On 2015/10/09 17:22:21, Robert Sesek wrote: > The char buf -> std::string copy can be avoided by declaring |buf| to be a > std::string and using ::resize() to allocate the space. Hmm but in theory I am not supposed to write to the .data() of a string (both data and cstr only return a const char*), as that could lead to undefined behavior. In practice I'm pretty sure it will work on most STL implementations, but it looks fragile. Anyways, I switched to use base::StringTokenizer and StringPiece https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:87: uint64 value; On 2015/10/09 17:22:21, Robert Sesek wrote: > Use the stdint.h types (uint64_t) rather than the deprecated ones in > basictypes.h. Oh, right, a piece in my brain is still hardcoded to the old chrome style ones. We should have a presubmit for this. Done https://codereview.chromium.org/1383733002/diff/80001/components/tracing/grap... components/tracing/graphics_memory_dump_provider_android.cc:93: if (key_split_point > 0 && key_split_point < row_name.size() - 1) { On 2015/10/09 17:22:21, Robert Sesek wrote: > This will always be >0 since it's size_t (unsigned). Well this could be 0 if the string starts with _, in which case the substr below would make no sense. That was the original point of the check > Check against std::string::npos instead. If I check for 0 < X < size(), checking against npos should be rendundant, as npos >> size()
primiano@chromium.org changed reviewers: + dsinclair@chromium.org, piman@chromium.org
Missing OWNERS stamps +dsinclair for components/tracing/ +piman for content/
lgtm
rsesek@, mind taking a 2nd look? Thanks
https://codereview.chromium.org/1383733002/diff/100001/components/tracing/gra... File components/tracing/graphics_memory_dump_provider_android.h (right): https://codereview.chromium.org/1383733002/diff/100001/components/tracing/gra... components/tracing/graphics_memory_dump_provider_android.h:33: FRIEND_TEST_ALL_PREFIXES(GraphicsMemoryDumpProviderTest, ParseResponse); Where's the test?
LGTM otherwise
https://codereview.chromium.org/1383733002/diff/100001/components/tracing/gra... File components/tracing/graphics_memory_dump_provider_android.h (right): https://codereview.chromium.org/1383733002/diff/100001/components/tracing/gra... components/tracing/graphics_memory_dump_provider_android.h:33: FRIEND_TEST_ALL_PREFIXES(GraphicsMemoryDumpProviderTest, ParseResponse); On 2015/10/13 22:38:39, Robert Sesek wrote: > Where's the test? Oh, this line should really be moved to crrev.com/1396313002, the cl introducing the test. Why not putting the test here? that CL needs some gyp reshuffling, hence my guts say that it is going to and be reverted because ... gyp is gyp and non trivial gyp changes break stuff. Removed from here.
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org, rsesek@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1383733002/#ps120001 (title: "Remove test friends lines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383733002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383733002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Aaaaa owners are never enough. +dsinclair for /components/tracing
https://codereview.chromium.org/1383733002/diff/120001/components/tracing/gra... File components/tracing/graphics_memory_dump_provider_android.cc (right): https://codereview.chromium.org/1383733002/diff/120001/components/tracing/gra... components/tracing/graphics_memory_dump_provider_android.cc:101: base::CStringTokenizer tokenizer(response, response + length, " \n"); If I have root on the device, could I setup my own daemon to respond and send arbitrarily large amounts of text back? Should we have a max length we look at?
On 2015/10/14 14:44:46, dsinclair wrote: > https://codereview.chromium.org/1383733002/diff/120001/components/tracing/gra... > File components/tracing/graphics_memory_dump_provider_android.cc (right): > > https://codereview.chromium.org/1383733002/diff/120001/components/tracing/gra... > components/tracing/graphics_memory_dump_provider_android.cc:101: > base::CStringTokenizer tokenizer(response, response + length, " \n"); > If I have root on the device, could I setup my own daemon to respond and send > arbitrarily large amounts of text back? Should we have a max length we look at?
On 2015/10/14 14:44:46, dsinclair wrote: > https://codereview.chromium.org/1383733002/diff/120001/components/tracing/gra... > File components/tracing/graphics_memory_dump_provider_android.cc (right): > > https://codereview.chromium.org/1383733002/diff/120001/components/tracing/gra... > components/tracing/graphics_memory_dump_provider_android.cc:101: > base::CStringTokenizer tokenizer(response, response + length, " \n"); > If I have root on the device, could I setup my own daemon to respond and send > arbitrarily large amounts of text back? Should we have a max length we look at? We already do. We recv() only once and never recv() more than 4096 bytes (see above), so this function never gets called with a buffer larger than 4k.
The CQ bit was checked by dsinclair@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383733002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383733002/120001
The CQ bit was unchecked by primiano@chromium.org
(I unticked it, just realized there is a small last-minute rename to the column name I want to do)
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, dsinclair@chromium.org, ericrk@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1383733002/#ps140001 (title: "Add memtrack to column name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1383733002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1383733002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b3fb641552ace46ce637f4345ac8809cbfbe0059 Cr-Commit-Position: refs/heads/master@{#354032} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
