Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1332943002:
[tracing] Smaps provider uses fscanf instead of istream (Closed)
|
Created:
5 years, 3 months ago by ssid Modified:
5 years, 2 months ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, 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] Smaps provider uses fscanf instead of istream
The smaps file descriptor received from browser process needs to be read
into a string for creating an istream. This could consume lot of memory.
So, this CL switches the dump provider to use fscanf instead of istream
to read the file.
BUG=461788
Committed: https://crrev.com/48ce0ef9537a41150fa0d59edd47c318661aeee4
Cr-Commit-Position: refs/heads/master@{#352675}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fixes #Patch Set 3 : Nits. #Patch Set 4 : Nits. #
Total comments: 16
Patch Set 5 : Using FILE*. #
Total comments: 8
Patch Set 6 : Fixes. #Patch Set 7 : Fixes. #Patch Set 8 : Fixes. #Patch Set 9 : Nit. #Patch Set 10 : Moving to readin by lines. #
Total comments: 14
Patch Set 11 : Fixes. #
Dependent Patchsets: Messages
Total messages: 19 (7 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL
I think this can be simplified a lot as scanf is much more magic (I wonder why I didn't use it in the first place) THanks for fixing this https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:8: #include <inttypes.h> inttypes is not well supported on windows. use base/format_macros.h https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:44: char protection_flags[4]; char protection_flags[5] = {0}; so you get also a NUL terminator, which is definitely safer https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:45: CHECK_EQ(1, fscanf(smaps_file, "%4s", protection_flags)); use %4c, is safer https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:65: if (fscanf(smaps_file, "%s %s %s", ignore, ignore, ignore) != 3) you don't need the ignore vars, nor kMaxAddressLength. Just use %*s to skip the assignment, i.e.: fscanf(smaps_file, "%*s %*s %*s"). If you fail here the next line parsing will be borked. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:69: char next; you don't need all this as scanf handles multiple spaces fine. Actually I think you can solve everything with one scanf in one go, without any fgetc/fputc https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:118: std::string unit(2, 0); I think you can just do a single scanf like scanf("Counter name: %s kB"), and simplify all this code. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:135: FILE* smaps_file = fdopen(smaps_fd.get(), "r"); Hmm I think you should cache also the FILE* struct, together with the scopedfd. Rationale: you can't close the FILE without closing the underlying file, but then you lose the fd, and you don't want that. If you don't close you'll leak FILE* structs, which is similarly bad. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:146: next = fgetc(smaps_file); not sure what all this is for? aren't you trying to do a feof(f) here? https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps_dump_provider.h (right): https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.h:30: static ScopedFD* proc_smaps_for_testing; I think you don't want the * here (just scopedfd) otherwise you lose the scoped effect
PTAL, thanks https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:8: #include <inttypes.h> On 2015/09/10 16:52:36, Primiano Tucci wrote: > inttypes is not well supported on windows. > use base/format_macros.h Done. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:44: char protection_flags[4]; On 2015/09/10 16:52:36, Primiano Tucci wrote: > char protection_flags[5] = {0}; so you get also a NUL terminator, which is > definitely safer Done. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:45: CHECK_EQ(1, fscanf(smaps_file, "%4s", protection_flags)); On 2015/09/10 16:52:36, Primiano Tucci wrote: > use %4c, is safer Actually %4c does not add a '\0' at the end as far as i tested. But still it is not required for our case. So made it 4c https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:65: if (fscanf(smaps_file, "%s %s %s", ignore, ignore, ignore) != 3) On 2015/09/10 16:52:36, Primiano Tucci wrote: > you don't need the ignore vars, nor kMaxAddressLength. > Just use %*s to skip the assignment, i.e.: > > fscanf(smaps_file, "%*s %*s %*s"). > > If you fail here the next line parsing will be borked. Moved everything to single line fscanf. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:69: char next; On 2015/09/10 16:52:36, Primiano Tucci wrote: > you don't need all this as scanf handles multiple spaces fine. > Actually I think you can solve everything with one scanf in one go, without any > fgetc/fputc Hm, it was tricky case if file name does not exist. From discussion, i am using strip white spaces on both sides of the file. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:118: std::string unit(2, 0); On 2015/09/10 16:52:36, Primiano Tucci wrote: > I think you can just do a single scanf like > scanf("Counter name: %s kB"), and simplify all this code. Done. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:135: FILE* smaps_file = fdopen(smaps_fd.get(), "r"); On 2015/09/10 16:52:36, Primiano Tucci wrote: > Hmm I think you should cache also the FILE* struct, together with the scopedfd. > Rationale: you can't close the FILE without closing the underlying file, but > then you lose the fd, and you don't want that. > If you don't close you'll leak FILE* structs, which is similarly bad. > Yes, Made necessary changes. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.cc:146: next = fgetc(smaps_file); On 2015/09/10 16:52:36, Primiano Tucci wrote: > not sure what all this is for? aren't you trying to do a feof(f) here? hm, the next is also used to check if it is a header or segment. Though I could shorten the seek. https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... File base/trace_event/process_memory_maps_dump_provider.h (right): https://codereview.chromium.org/1332943002/diff/1/base/trace_event/process_me... base/trace_event/process_memory_maps_dump_provider.h:30: static ScopedFD* proc_smaps_for_testing; On 2015/09/10 16:52:36, Primiano Tucci wrote: > I think you don't want the * here (just scopedfd) otherwise you lose the scoped > effect Hm it is static variable and scoped. It needs exit time destructor. I should make it lazy instance and non-leaky since it should close the file. This would cause initialization and destruction at exit time in production code. Instead I thought pointer which just gets assigned for tests.
https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:67: use TrimWhitespaceASCII ? https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:72: uint64 counter_value = 0; Is there any need of this function at this point? Can you handle everything in PArseSMaps counter below with one single scanf? https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:83: DCHECK_EQ(1, did_read); I think at this point you can do a single scanf here doing "%s: %SCNu64 kB" https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:86: // whether Rss isn't already counting swapped and fix below if that is Since you are here, plz remove this TODO, we're dealing with this in the UI https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:105: char ignore[kMaxLineSize]; At this point this should be uneeded (provided that the previous scanf swallows the \n) https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:125: next = fgetc(smaps_file); how is this fgetc + ungetc (brrr) different from feof()? https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider.h (right): https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.h:34: #endif Not sure I understand why a pointer to a ScopedFILE? Either a ScopedFILE (this takes ownership) or a FILE* plz. Probably the former
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:67: On 2015/09/30 09:37:02, Primiano Tucci wrote: > use TrimWhitespaceASCII ? Done. https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:72: uint64 counter_value = 0; On 2015/09/30 09:37:02, Primiano Tucci wrote: > Is there any need of this function at this point? Can you handle everything in > PArseSMaps counter below with one single scanf? Yes, This function will only be called when the counter_name is one of "pss:" or "private_dirty:"... In case of vmflags it is not in format to read a counter. https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:83: DCHECK_EQ(1, did_read); On 2015/09/30 09:37:02, Primiano Tucci wrote: > I think at this point you can do a single scanf here doing "%s: %SCNu64 kB" No, see my other comment. https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:86: // whether Rss isn't already counting swapped and fix below if that is On 2015/09/30 09:37:02, Primiano Tucci wrote: > Since you are here, plz remove this TODO, we're dealing with this in the UI Done. https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:105: char ignore[kMaxLineSize]; On 2015/09/30 09:37:02, Primiano Tucci wrote: > At this point this should be uneeded (provided that the previous scanf swallows > the \n) That scanf cannot be used to swallow everything since the format can be different in different lines. like in case of vmflags. https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:125: next = fgetc(smaps_file); On 2015/09/30 09:37:02, Primiano Tucci wrote: > how is this fgetc + ungetc (brrr) different from feof()? feof only returns a boolean. Result of fgetc + ungetc (brrr) is a character, which is used later. Can't find a better way to implement peek. https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider.h (right): https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.h:34: #endif On 2015/09/30 09:37:02, Primiano Tucci wrote: > Not sure I understand why a pointer to a ScopedFILE? > Either a ScopedFILE (this takes ownership) or a FILE* plz. > Probably the former I can't have static ScopedFILE since it needs exit time destructor, as I mentioned in previous comment. I have to use some kind of lazy init and it just becomes harder to differentiate from test and normal mode, also this adds extra variable in production code. Making it FILE*
https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.cc:125: next = fgetc(smaps_file); On 2015/09/30 11:15:13, ssid wrote: > On 2015/09/30 09:37:02, Primiano Tucci wrote: > > how is this fgetc + ungetc (brrr) different from feof()? > > feof only returns a boolean. Result of fgetc + ungetc (brrr) is a character, > which is used later. > > Can't find a better way to implement peek. At this point change this code to be line buffered: read one line per time, then pass the line to the functions, instead of the file* https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider.h (right): https://codereview.chromium.org/1332943002/diff/60001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.h:34: #endif On 2015/09/30 11:15:13, ssid wrote: > On 2015/09/30 09:37:02, Primiano Tucci wrote: > > Not sure I understand why a pointer to a ScopedFILE? > > Either a ScopedFILE (this takes ownership) or a FILE* plz. > > Probably the former > > I can't have static ScopedFILE since it needs exit time destructor, as I > mentioned in previous comment. I have to use some kind of lazy init and it just > becomes harder to differentiate from test and normal mode, also this adds extra > variable in production code. > Making it FILE* Acknowledged. https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:28: const uint32 kMaxCounterNameSize = 20; move this constant down in the only place where it's used. https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:73: CHECK_EQ(1, fscanf(smaps_file, "%" SCNu64 " kB", &counter_value)); Please don't put operations with side-effects in CHECKs. See crbug.com/535470, if somebody turns this into a DCHECK the code will fail. do res = fscanf; CHECK_EQ(1, res) https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:103: fgets(ignore, sizeof(ignore), smaps_file); Just fscanf with %*4096[^\n]\n or similar will be faster as won't do any copy. https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider_unittest.cc:7: #include <fcntl.h> this can only be included on linux/android. move down and add ifdef guard
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Addressing comments and fixing all the tests. PTAL. https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:28: const uint32 kMaxCounterNameSize = 20; On 2015/09/30 15:36:35, Primiano Tucci wrote: > move this constant down in the only place where it's used. Done. https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:73: CHECK_EQ(1, fscanf(smaps_file, "%" SCNu64 " kB", &counter_value)); On 2015/09/30 15:36:35, Primiano Tucci wrote: > Please don't put operations with side-effects in CHECKs. > See crbug.com/535470, if somebody turns this into a DCHECK the code will fail. > do res = fscanf; > CHECK_EQ(1, res) Done. Thanks. https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:103: fgets(ignore, sizeof(ignore), smaps_file); On 2015/09/30 15:36:35, Primiano Tucci wrote: > Just fscanf with %*4096[^\n]\n or similar will be faster as won't do any copy. Done. https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/1332943002/diff/100001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider_unittest.cc:7: #include <fcntl.h> On 2015/09/30 15:36:35, Primiano Tucci wrote: > this can only be included on linux/android. > > move down and add ifdef guard Done.
Patchset #9 (id:220001) has been deleted
LGTM with some comments https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:10: #if defined(OS_LINUX) || defined(OS_ANDROID) Don't really need to hide these. Either you mask the entire file, or just leave the ifdef guards where needed https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:27: bool ParseSmapsHeader(char* header_line, ProcessMemoryMaps::VMRegion* region) { +const -> const char* https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:70: CHECK_EQ(1, res); You can probably relax this to be a DCHECK, no need to hard crash if you can't parse something here. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:78: const uint32 kMaxCounterNameSize = 20; Actually doesn't seem you use this anywhere else, just put the 20 below https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:80: int did_read = sscanf(counter_line, "%s", counter_name); replace %s with %19[^\n]s, so this won't overflow if you get a name larger than 20 chars https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:115: if (fgets(line, kMaxLineSize, smaps_file) == nullptr) maybe set line[0] = '\0' before the fgets so if you happen to read a blank line the if below won't do random things in production code https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider_unittest.cc:14: #include <fcntl.h> either you don't need fcntl and remove it, or just move the ifdef guard up
Made changes suggested. Thanks. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider.cc (right): https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:10: #if defined(OS_LINUX) || defined(OS_ANDROID) On 2015/10/06 16:37:31, Primiano Tucci wrote: > Don't really need to hide these. Either you mask the entire file, or just leave > the ifdef guards where needed Done. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:27: bool ParseSmapsHeader(char* header_line, ProcessMemoryMaps::VMRegion* region) { On 2015/10/06 16:37:31, Primiano Tucci wrote: > +const -> const char* Done. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:70: CHECK_EQ(1, res); On 2015/10/06 16:37:31, Primiano Tucci wrote: > You can probably relax this to be a DCHECK, no need to hard crash if you can't > parse something here. Done. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:78: const uint32 kMaxCounterNameSize = 20; On 2015/10/06 16:37:31, Primiano Tucci wrote: > Actually doesn't seem you use this anywhere else, just put the 20 below Done. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:80: int did_read = sscanf(counter_line, "%s", counter_name); On 2015/10/06 16:37:31, Primiano Tucci wrote: > replace %s with %19[^\n]s, so this won't overflow if you get a name larger than > 20 chars Done. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider.cc:115: if (fgets(line, kMaxLineSize, smaps_file) == nullptr) On 2015/10/06 16:37:31, Primiano Tucci wrote: > maybe set line[0] = '\0' before the fgets so if you happen to read a blank line > the if below won't do random things in production code Done. https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_maps_dump_provider_unittest.cc (right): https://codereview.chromium.org/1332943002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_maps_dump_provider_unittest.cc:14: #include <fcntl.h> On 2015/10/06 16:37:31, Primiano Tucci wrote: > either you don't need fcntl and remove it, or just move the ifdef guard up Done.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1332943002/#ps280001 (title: "Fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1332943002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1332943002/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/48ce0ef9537a41150fa0d59edd47c318661aeee4 Cr-Commit-Position: refs/heads/master@{#352675} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
