Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
951463002:
[tracing] Add memory maps dumper impl for Linux/Android (Closed)
|
Created:
5 years, 10 months ago by Primiano Tucci (use gerrit) Modified:
5 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, picksi1, Sami, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@mmaps_skeleton Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Add memory maps dumper impl for Linux/Android
This is a follow-up to crrev.com/928723004. It introduces
the code which is able to read and parse the /proc/.../smaps
file into an actual TraceValue and the corresponding unittest.
BUG=460884
Committed: https://crrev.com/452b8c04929b90fc632f08997be9c01ac0a4cdce
Cr-Commit-Position: refs/heads/master@{#318023}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : small refactor (dsinclair review) #
Total comments: 9
Patch Set 3 : dsinclair nits #
Total comments: 4
Patch Set 4 : Add Kb check + malformed regions #
Total comments: 2
Patch Set 5 : Make coutner logic more robust #Patch Set 6 : Fix for iOS #
Messages
Total messages: 27 (10 generated)
Patchset #1 (id:1) has been deleted
primiano@chromium.org changed reviewers: + dsinclair@chromium.org, nduca@chromium.org
Note: this is based on top of crrev.com/928723004 (which is not landed yet). It makes sense to review the 928723004 first and this next.
https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:35: if ((next >= '0' && next <= '9') || (next >= 'a' && next <= 'f')) { What about A-Z? (There is a isxdigit() in <cctype> can we use that?) https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:36: // Parse head line: "00400000-00421000 r-xp 00000000 fc:01 1234 /foo.so" Add the \n to the end since, it looks like, it will be consumed with the getLine below https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:64: region.mapped_file = mapped_file; Can the bodies of the if/else get pulled out into methods easily? https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:66: // Parse counter line: "VmRSS: 0 Kb" If the \n gets consumed below, add it to the example above.
https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:35: if ((next >= '0' && next <= '9') || (next >= 'a' && next <= 'f')) { On 2015/02/23 15:37:12, dsinclair wrote: > What about A-Z? (There is a isxdigit() in <cctype> can we use that?) ![A-F] is deliberate. The kernel always used (so far) lowercase hex in smaps. This parser uses 1 char lookahead (which is easy and fast) to determine whether the current line is a header (a-f0-9) or a counter (A-Z).*. If I just use isxdigit() this will break when encountering "Anonymous", as the "A" will be confused with the start of an address. And using longer lookahead will make the entire code less readable and less performant IMHO without any actual benefit (i.e. I thing that if the kernel starts using uppercase letters in the hex addresses that will be a major ABI break). https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:36: // Parse head line: "00400000-00421000 r-xp 00000000 fc:01 1234 /foo.so" On 2015/02/23 15:37:12, dsinclair wrote: > Add the \n to the end since, it looks like, it will be consumed with the getLine > below Done. https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:64: region.mapped_file = mapped_file; On 2015/02/23 15:37:12, dsinclair wrote: > Can the bodies of the if/else get pulled out into methods easily? Done. https://codereview.chromium.org/951463002/diff/20001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:66: // Parse counter line: "VmRSS: 0 Kb" On 2015/02/23 15:37:12, dsinclair wrote: > If the \n gets consumed below, add it to the example above. Done.
lgtm, couple of nits. https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:31: std::string prot_flags; nit: s/prot/protected/ https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:39: // This is not just paranoia, it can actually happen (See b/17402069). Do we want b/ links in open code? https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:115: counters_parsed_for_current_region += ParseSmapsCounter(smaps, ®ion); is it worth adding an ASSERT(counters_parsed_for_current_region <= kNumExpectedCountersPerRegion)?
also lgtm assuming someone does a careful review
primiano@chromium.org changed reviewers: + skyostil@chromium.org
+skyostil if you have any time, can you have just a final pass? Uber thanks https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:31: std::string prot_flags; On 2015/02/23 17:10:34, dsinclair wrote: > nit: s/prot/protected/ I assume you meant protection not protected, by consistency with below, right? https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:39: // This is not just paranoia, it can actually happen (See b/17402069). On 2015/02/23 17:10:34, dsinclair wrote: > Do we want b/ links in open code? I created a public bug to mirror the internal one. Just FYI, there is anyways a number (123) of other b/ cases in the codebase. https://code.google.com/p/chromium/codesearch#search/&q=%5Cbb/%5Cd%7B5,%7D%5C... https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:115: counters_parsed_for_current_region += ParseSmapsCounter(smaps, ®ion); On 2015/02/23 17:10:34, dsinclair wrote: > is it worth adding an ASSERT(counters_parsed_for_current_region <= > kNumExpectedCountersPerRegion)? Uhm that cannot technically happen, even if the kernel is screwed up, because the counter is reset below when reaches kNumExpected.
https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:91: smaps->ignore(kMaxLineSize, '\n'); I guess the kernel format is set in stone by now so it's not worth DCHECKing that the unit is "Kb"? https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider_unittest.cc:67: } // namespace Could you add a couple of malformed examples (empty file, one with a negative/zero region size)?
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Thanks skyostil. https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:91: smaps->ignore(kMaxLineSize, '\n'); On 2015/02/24 11:11:25, Sami wrote: > I guess the kernel format is set in stone by now so it's not worth DCHECKing > that the unit is "Kb"? I would expect tons of things to break in that case. Anyways, we can always add a debug-only check, like this. https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/951463002/diff/60001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider_unittest.cc:67: } // namespace On 2015/02/24 11:11:25, Sami wrote: > Could you add a couple of malformed examples (empty file, one with a > negative/zero region size)? Done for negative zero. Empty file, instead, is a legitimate case (anonymous memory)
Sweet, lgtm. https://codereview.chromium.org/951463002/diff/120001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/951463002/diff/120001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider_unittest.cc:51: // An invalid region, with zero size and overlapping with the last one nit: extra space here
https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:31: std::string prot_flags; On 2015/02/24 09:16:14, Primiano Tucci wrote: > On 2015/02/23 17:10:34, dsinclair wrote: > > nit: s/prot/protected/ > > I assume you meant protection not protected, by consistency with below, right? Yes (whatever the correct expanded version of prot is, heh) https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:115: counters_parsed_for_current_region += ParseSmapsCounter(smaps, ®ion); On 2015/02/24 09:16:14, Primiano Tucci wrote: > On 2015/02/23 17:10:34, dsinclair wrote: > > is it worth adding an ASSERT(counters_parsed_for_current_region <= > > kNumExpectedCountersPerRegion)? > > Uhm that cannot technically happen, even if the kernel is screwed up, because > the counter is reset below when reaches kNumExpected. I'm more worried about ParseSnmapsCounter getting changed and outputting say += 3 which is > kNumExpectedCountersPerRegion.
https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/951463002/diff/40001/base/trace_event/process... base/trace_event/process_memory_maps_dump_provider.cc:115: counters_parsed_for_current_region += ParseSmapsCounter(smaps, ®ion); On 2015/02/24 14:43:13, dsinclair wrote: > On 2015/02/24 09:16:14, Primiano Tucci wrote: > > On 2015/02/23 17:10:34, dsinclair wrote: > > > is it worth adding an ASSERT(counters_parsed_for_current_region <= > > > kNumExpectedCountersPerRegion)? > > > > Uhm that cannot technically happen, even if the kernel is screwed up, because > > the counter is reset below when reaches kNumExpected. > > > I'm more worried about ParseSnmapsCounter getting changed and outputting say += > 3 which is > kNumExpectedCountersPerRegion. > Ok I see your concern. I think that PS5 should address that. https://codereview.chromium.org/951463002/diff/120001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/951463002/diff/120001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider_unittest.cc:51: // An invalid region, with zero size and overlapping with the last one On 2015/02/24 14:22:43, Sami wrote: > nit: extra space here Done.
Awesome, thanks a lot. lgtm.
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/951463002/#ps140001 (title: "Make coutner logic more robust")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/951463002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org, nduca@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/951463002/#ps160001 (title: "Fix for iOS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/951463002/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/452b8c04929b90fc632f08997be9c01ac0a4cdce Cr-Commit-Position: refs/heads/master@{#318023} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
