Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1796803003:
Add statistics about free space to microdump format. (Closed)
|
Created:
4 years, 9 months ago by Tobias Sargeant Modified:
4 years, 7 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd statistics about free space to microdump format.
When a crash occurs as a result of an allocation failure, it is useful
to know approximately what regions of the virtual address space remain
available, so that we know whether the crash should be attributed to
memory fragmentation, or some other cause.
BUG=525938
R=primiano@chromium.org
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/adca10c8ff9884d5c705fe60de2e0420ff9e73c6
Patch Set 1 #
Total comments: 10
Patch Set 2 : address comments; remove uses of libc/stl. #Patch Set 3 : remove #include <algorithm> #
Total comments: 30
Patch Set 4 : #
Total comments: 1
Patch Set 5 : don't dump mapping summary on 64 bit, make NextOrdererdMapping simpler. #Patch Set 6 : fix stupid error in Log2Floor #
Total comments: 5
Patch Set 7 : Address comments, Fix NextOrderedMapping. #
Total comments: 17
Patch Set 8 : Address PS7 comments #
Total comments: 3
Messages
Total messages: 24 (2 generated)
tobiasjs@chromium.org changed reviewers: + primiano@chromium.org, torne@chromium.org
PTAL. It was a bit more complicated to collect these stats than initially expected, because: a) the mappings vector isn't sorted b) the ranges in the mappings vector can overlap The output produced tallies with manual inspection of /proc/*/maps.
On 2016/03/14 12:32:30, Tobias Sargeant wrote: > PTAL. It was a bit more complicated to collect these stats than initially > expected, because: > > a) the mappings vector isn't sorted > b) the ranges in the mappings vector can overlap > > The output produced tallies with manual inspection of /proc/*/maps. Apologies for the lateness. I am hyper-backlogged, didn't forget about this one. Will try to get to this tomorrow.
> Apologies for the lateness. I am hyper-backlogged, didn't forget about this one. > Will try to get to this tomorrow. Hi Primiano, Low priority, but could you please take a look at this once you have some spare time?
On 2016/04/06 10:18:51, Tobias Sargeant wrote: > > Apologies for the lateness. I am hyper-backlogged, didn't forget about this > one. > > Will try to get to this tomorrow. > > Hi Primiano, > > Low priority, but could you please take a look at this once you have some spare > time? Oops sorry this fell off my radar after my easter holidays inbox bankrupcy. Was sheriffing all the day today, will get on this tomorrow. Plz nag me live if I don't.
First attempt, sorry for the huge delay. Didn't have time yet to check the math/logic. Will do that tomorrow. but some initial safety considerations. TL;DR welcome to breakpad, where neither libc nor libc++ nor anything that calls into them is safe to call :) https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:34: in general in breakpad client we don't allow anything that might call into libc, as we cannot trust libc itself (see explanation below) https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:382: static bool MappingsAreAdjacent(const MappingInfo *a, const MappingInfo *b) { should these static functions just be anonymous namespace functions? https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:382: static bool MappingsAreAdjacent(const MappingInfo *a, const MappingInfo *b) { nit: * should be close to the type, i.e. MappingInfo* . same below. https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:420: static const int HBITS = sizeof(size_t)*8; nit: space between ) * 8 https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:422: std::fill(hole_hist, hole_hist + HBITS, 0); nope, in breakpad we cannot rely on any std lib function, as they migh call libc function that we cannot trust (because they might take locks which are held when in a crash state). my_memset if what you have to live with :) https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:472: snprintf(buf, 32, " %d %0zX %0zX", hole_cnt, hole_max, hole_tot); no snprintf for same reason. Please use the LogAppend() facilities to dump hex.
I think I matched your delay with my own. https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:34: On 2016/04/07 18:07:39, Primiano Tucci wrote: > in general in breakpad client we don't allow anything that might call into libc, > as we cannot trust libc itself (see explanation below) Acknowledged. https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:420: static const int HBITS = sizeof(size_t)*8; On 2016/04/07 18:07:39, Primiano Tucci wrote: > nit: space between ) * 8 Done. https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:422: std::fill(hole_hist, hole_hist + HBITS, 0); On 2016/04/07 18:07:39, Primiano Tucci wrote: > nope, in breakpad we cannot rely on any std lib function, as they migh call libc > function that we cannot trust (because they might take locks which are held when > in a crash state). > > my_memset if what you have to live with :) Done. https://codereview.chromium.org/1796803003/diff/1/src/client/linux/microdump_... src/client/linux/microdump_writer/microdump_writer.cc:472: snprintf(buf, 32, " %d %0zX %0zX", hole_cnt, hole_max, hole_tot); On 2016/04/07 18:07:39, Primiano Tucci wrote: > no snprintf for same reason. Please use the LogAppend() facilities to dump hex. Done.
Ok the logic here makes sense to me. Not sure about the load-bias part. I don't think that should affect this logic. Some finer-grained comments, overall looks good. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:64: // Because of load biasing, we can end up with a situation where two Hmm I thought that after the latest linker changes, mappings can not overlap because of the zero guard regions. When does this happen? https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:74: size_t NextOrderedMapping( Not sure I get the full logic here. Why is not enough just getting the next by start-address? (in other words: why are you looking at the size at all here?) https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:169: const int SZ = sizeof(unsigned int) * 3; can we just use hex for everything, for consistency with the current format (where even the cpu count is hex). Will make the processor part also easier https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:446: unsigned int hole_hist[HBITS]; can you call this hole_histogram? For a long while (until I saw the code below) I thought this was going to be some kind of history :) https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:459: size_t hole_tot = 0; I'd probably rename this to hole_sum (or unmapped_total). I figured out only down below what this really was :) https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:470: break; isn't this going to ignore the big hole that you can fave between the last mapping and the end of the address space? https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:481: while (hole_sz) { Can we just copy chrome base::Log2Floor(uint32_t n) and use it here? This is going to loop x32 on every mapping. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:497: LogAppend(" "); LogAppendDecimal(hole_cnt); are you appending the count three times here? I suppose you meant to append tot, cnt and sum? https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:498: LogAppend(" "); LogAppend(hole_cnt); Also plz newlines after each statement https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:500: for (unsigned int i = 0; i < HBITS; ++i) { can we saturate this up and down? First of all, all your holes will be at least 4k big. So the buckets 0-11 should always be zero. Also I think we want to cap up at ~1GB. Not sure we (chrome) should rely on having more thatn 1GB of contiguous address space. Also for 64bit this line is going to be loooooooooooooooooong and perhaps overflow the logcat limit.
https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:500: for (unsigned int i = 0; i < HBITS; ++i) { On 2016/05/06 11:07:57, Primiano Tucci wrote: > can we saturate this up and down? > First of all, all your holes will be at least 4k big. So the buckets 0-11 should > always be zero. > Also I think we want to cap up at ~1GB. Not sure we (chrome) should rely on > having more thatn 1GB of contiguous address space. > Also for 64bit this line is going to be loooooooooooooooooong and perhaps > overflow the logcat limit. We should probably just not do this at all on 64bit, there's no point.
https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:74: size_t NextOrderedMapping( On 2016/05/06 11:07:56, Primiano Tucci wrote: > Not sure I get the full logic here. Why is not enough just getting the next by > start-address? (in other words: why are you looking at the size at all here?) size here is mappings array length, not the size of one mapping. This is finding the entry with the lowest start address greater than mappings[curr]->start_addr. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:169: const int SZ = sizeof(unsigned int) * 3; On 2016/05/06 11:07:56, Primiano Tucci wrote: > can we just use hex for everything, for consistency with the current format > (where even the cpu count is hex). Will make the processor part also easier Can do, but would still need to trim leading zeros, to be more succinct. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:470: break; On 2016/05/06 11:07:57, Primiano Tucci wrote: > isn't this going to ignore the big hole that you can fave between the last > mapping and the end of the address space? It is, but we don't know exactly how big that hole is, because we don't know where the end of the allocatable address space is. Same with space before the first mapping. That's why we log the lowest and highest mapped addresses, and the histogram of holes. That way we can work out what to do later. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:500: for (unsigned int i = 0; i < HBITS; ++i) { On 2016/05/06 11:07:57, Primiano Tucci wrote: > can we saturate this up and down? > First of all, all your holes will be at least 4k big. So the buckets 0-11 should > always be zero. > Also I think we want to cap up at ~1GB. Not sure we (chrome) should rely on > having more thatn 1GB of contiguous address space. > Also for 64bit this line is going to be loooooooooooooooooong and perhaps > overflow the logcat limit. I'm not sure we want to cap at 1GB. On 32 bit that really only saves one extra bucket, and on 64 bit I'd guess that there aren't going to be many address ranges in the tail (and we could also just skip this entirely for 64 bit as torne suggested). I didn't want to make any assumptions about page size, so I figured the easiest approach was just to skip zero entries in the histogram, which would have the effect of not outputting anything for values smaller than the page size (whatever that happened to be).
https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:74: size_t NextOrderedMapping( On 2016/05/06 11:33:45, Tobias Sargeant wrote: > On 2016/05/06 11:07:56, Primiano Tucci wrote: > > Not sure I get the full logic here. Why is not enough just getting the next by > > start-address? (in other words: why are you looking at the size at all here?) > > size here is mappings array length, not the size of one mapping. This is finding > the entry with the lowest start address greater than mappings[curr]->start_addr. Oh ok I misread it. This seems overly complicated. I am probably still missing something. Why this is not just: best = it.back() for (it = mappings.begin(); it != mappings.end(); ++it) { if (it->start_addr > curr && it->start_addr < best) best = it; } https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:169: const int SZ = sizeof(unsigned int) * 3; On 2016/05/06 11:33:45, Tobias Sargeant wrote: > On 2016/05/06 11:07:56, Primiano Tucci wrote: > > can we just use hex for everything, for consistency with the current format > > (where even the cpu count is hex). Will make the processor part also easier > > Can do, but would still need to trim leading zeros, to be more succinct. LogAppend<T> will emit leading zeros only to pad the actual T size. So if you use a unsigned char will be at most one zero, not bad. As long as we stay in the line length (can't remember, it's somewhere, somewehat between 500 and 1k chars) it's ok https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:470: break; On 2016/05/06 11:33:45, Tobias Sargeant wrote: > On 2016/05/06 11:07:57, Primiano Tucci wrote: > > isn't this going to ignore the big hole that you can fave between the last > > mapping and the end of the address space? > > It is, but we don't know exactly how big that hole is, because we don't know > where the end of the allocatable address space is. Same with space before the > first mapping. That's why we log the lowest and highest mapped addresses, and > the histogram of holes. That way we can work out what to do later. Hmm but then you will never know if that hole is the largest one or not. Say that you have only two mappings: 1:[0x1000, 0x2000], 2: [0x3000, 0x4000] You will conclude that your largest available hole is 0x1000 (0x3000 - 0x2000). The reality instead is there was a huge gap after 2. At this point probably the best thing to do is: keep the hole logic as it is dump also the lowest and highest mapped address. So we can work out the boundary cases after the fact. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:500: for (unsigned int i = 0; i < HBITS; ++i) { On 2016/05/06 11:13:36, Torne wrote: > On 2016/05/06 11:07:57, Primiano Tucci wrote: > > can we saturate this up and down? > > First of all, all your holes will be at least 4k big. So the buckets 0-11 > should > > always be zero. > > Also I think we want to cap up at ~1GB. Not sure we (chrome) should rely on > > having more thatn 1GB of contiguous address space. > > Also for 64bit this line is going to be loooooooooooooooooong and perhaps > > overflow the logcat limit. > > We should probably just not do this at all on 64bit, there's no point. SGTM but nothing in this CL is preventing this to happen on 64 bit https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:500: for (unsigned int i = 0; i < HBITS; ++i) { On 2016/05/06 11:33:45, Tobias Sargeant wrote: > On 2016/05/06 11:07:57, Primiano Tucci wrote: > > can we saturate this up and down? > > First of all, all your holes will be at least 4k big. So the buckets 0-11 > should > > always be zero. > > Also I think we want to cap up at ~1GB. Not sure we (chrome) should rely on > > having more thatn 1GB of contiguous address space. > > Also for 64bit this line is going to be loooooooooooooooooong and perhaps > > overflow the logcat limit. > > I'm not sure we want to cap at 1GB. On 32 bit that really only saves one extra > bucket, and on 64 bit I'd guess that there aren't going to be many address > ranges in the tail (and we could also just skip this entirely for 64 bit as > torne suggested). > > I didn't want to make any assumptions about page size, so I figured the easiest > approach was just to skip zero entries in the histogram, which would have the > effect of not outputting anything for values smaller than the page size > (whatever that happened to be). ok sorry I completely missed the if (!hole_hist[i]) continue; statement. Makes sense to keep this as it as long as we omit zeros.
https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:64: // Because of load biasing, we can end up with a situation where two On 2016/05/06 11:07:56, Primiano Tucci wrote: > Hmm I thought that after the latest linker changes, mappings can not overlap > because of the zero guard regions. > When does this happen? It definitely came up during writing this CL. When were these linker changes? Will zero guard regions appear as mappings themselves? https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:446: unsigned int hole_hist[HBITS]; On 2016/05/06 11:07:56, Primiano Tucci wrote: > can you call this hole_histogram? For a long while (until I saw the code below) > I thought this was going to be some kind of history :) Done. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:459: size_t hole_tot = 0; On 2016/05/06 11:07:56, Primiano Tucci wrote: > I'd probably rename this to hole_sum (or unmapped_total). I figured out only > down below what this really was :) Done. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:481: while (hole_sz) { On 2016/05/06 11:07:56, Primiano Tucci wrote: > Can we just copy chrome base::Log2Floor(uint32_t n) and use it here? > This is going to loop x32 on every mapping. Done. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:497: LogAppend(" "); LogAppendDecimal(hole_cnt); On 2016/05/06 11:07:56, Primiano Tucci wrote: > are you appending the count three times here? > I suppose you meant to append tot, cnt and sum? Certainly did! https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:498: LogAppend(" "); LogAppend(hole_cnt); On 2016/05/06 11:07:56, Primiano Tucci wrote: > Also plz newlines after each statement Done.
https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:64: // Because of load biasing, we can end up with a situation where two On 2016/05/06 13:29:31, Tobias Sargeant wrote: > On 2016/05/06 11:07:56, Primiano Tucci wrote: > > Hmm I thought that after the latest linker changes, mappings can not overlap > > because of the zero guard regions. > > When does this happen? > > It definitely came up during writing this CL. When were these linker changes? https://codereview.chromium.org/1218493004 And there should be an equivalent in the Android system linker in M > Will zero guard regions appear as mappings themselves? Yes (and you should count them as NON-holes as they steal address space). But breakpad should never see overlapping regions ever (this CL was to fix the bug). Curious to see if you have any instance of mmaps that overlap. I wonder if you did just hit https://bugs.chromium.org/p/chromium/issues/detail?id=499747#c24 that petrcermak fixed very recently (last week) https://codereview.chromium.org/1796803003/diff/60001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/60001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:63: int Log2Floor(uint64_t n) { Maybe just add a comment saying: // From chromium src/base/bits.h actually this seems a bit different from the base one. Any reason for not keeping that? The reason why I asked to take the one from base is because I trusted that to be covered and tested. I try to introduce as less new code as possible in breakpad, because breakpad is one of those things extremely hard to detect and debug if something goes wrong. I just tested this out of curiosity (sorry I am a super paranoid person as usual): http://pastebin.com/YiRmTJ5C This function takes 9 seconds to compute the Log2Floor(128) (as opposite to base one which is a bunch of loops). And if you build that with -O2 this seems to never terminate. Which is a good way to lose all the crashes :)
> I just tested this out of curiosity (sorry I am a super paranoid person as > usual): > http://pastebin.com/YiRmTJ5C > > This function takes 9 seconds to compute the Log2Floor(128) (as opposite to base > one which is a bunch of loops). > And if you build that with -O2 this seems to never terminate. > Which is a good way to lose all the crashes :) Yeah, ++i should be --i. Hadn't run it (frustrating to roll into webview). Would have caught it before submit when I ran manually, but I'll learn the lesson and copy-paste the chromium version.
OK Looks good, just some final comments. Also plz check sure that $ make check still works (doc: https://chromium.googlesource.com/breakpad/breakpad/) https://codereview.chromium.org/1796803003/diff/100001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/100001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:175: enum LeadingZeros { kKeepLeadingZeros, kDiscardLeadingZeros }; can we avoid complicating this logic? I don't think a bunch of leading zeros is going to make such a great difference. Just cast to the right type to minimize the number of zeros. Sorry but this is breakpad and my overall rule of thumb for reviewing changes here is: minimize the numbers of line changed, don't change something unless strictly necessary. Unfortunately breakpad has a sad story w.r.t. test coverage. The good news is that it is going to change in crashpad. The bad new is that this is still breakpad :) https://codereview.chromium.org/1796803003/diff/100001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:473: MappingsAreAdjacent(mappings[curr], mappings[curr + 1])) add curly braces around this loop (the while is multiline)
https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:74: size_t NextOrderedMapping( On 2016/05/06 13:29:23, Primiano Tucci wrote: > On 2016/05/06 11:33:45, Tobias Sargeant wrote: > > On 2016/05/06 11:07:56, Primiano Tucci wrote: > > > Not sure I get the full logic here. Why is not enough just getting the next > by > > > start-address? (in other words: why are you looking at the size at all > here?) > > > > size here is mappings array length, not the size of one mapping. This is > finding > > the entry with the lowest start address greater than > mappings[curr]->start_addr. > > Oh ok I misread it. This seems overly complicated. I am probably still missing > something. Why this is not just: > > best = it.back() > for (it = mappings.begin(); it != mappings.end(); ++it) { > if (it->start_addr > curr && it->start_addr < best) > best = it; > } Could do this, although you'd have to set best = curr, not back, so that you could distinguish the case where curr is actually the highest address mapping. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:169: const int SZ = sizeof(unsigned int) * 3; On 2016/05/06 13:29:23, Primiano Tucci wrote: > On 2016/05/06 11:33:45, Tobias Sargeant wrote: > > On 2016/05/06 11:07:56, Primiano Tucci wrote: > > > can we just use hex for everything, for consistency with the current format > > > (where even the cpu count is hex). Will make the processor part also easier > > > > Can do, but would still need to trim leading zeros, to be more succinct. > > LogAppend<T> will emit leading zeros only to pad the actual T size. > So if you use a unsigned char will be at most one zero, not bad. > As long as we stay in the line length (can't remember, it's somewhere, somewehat > between 500 and 1k chars) it's ok I modified LogAppend<T> to optionally strip leading zeros. Is that ok? I don't think I can cast hole_cnt or the histogram counts to anything less than uint16_t, and that probably means wasting 2-3 characters per entry, whereas stripping leading zeros doesn't seem too hard. https://codereview.chromium.org/1796803003/diff/40001/src/client/linux/microd... src/client/linux/microdump_writer/microdump_writer.cc:470: break; > At this point probably the best thing to do is: > keep the hole logic as it is > dump also the lowest and highest mapped address. So we can work out the boundary > cases after the fact. This is what happens. The log line is: H lo_addr hi_addr hole_cnt hole_max hole sum [bucket:count]* https://codereview.chromium.org/1796803003/diff/100001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/100001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:98: mappings[next]->start_addr < mappings[best]->start_addr) { Testing showed this is wrong. I rewrote it (actually reverted it back to basically what I started with), but tried to make it more readable. Hopefully you feel the final result is more readable. https://codereview.chromium.org/1796803003/diff/100001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:175: enum LeadingZeros { kKeepLeadingZeros, kDiscardLeadingZeros }; On 2016/05/10 16:20:49, Primiano Tucci wrote: > can we avoid complicating this logic? I don't think a bunch of leading zeros is > going to make such a great difference. > Just cast to the right type to minimize the number of zeros. > Sorry but this is breakpad and my overall rule of thumb for reviewing changes > here is: minimize the numbers of line changed, don't change something unless > strictly necessary. > Unfortunately breakpad has a sad story w.r.t. test coverage. The good news is > that it is going to change in crashpad. The bad new is that this is still > breakpad :) Done. I made the keys and values in the histogram uint8_t, and the counts saturate at 0xff. The count of all holes is a uint16_t now. It seemed unnecessary to saturate that, but maybe it should be? https://codereview.chromium.org/1796803003/diff/100001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:473: MappingsAreAdjacent(mappings[curr], mappings[curr + 1])) On 2016/05/10 16:20:49, Primiano Tucci wrote: > add curly braces around this loop (the while is multiline) Done.
LGTM % final comments https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:81: bool MappingsAreAdjacent(const MappingInfo* a, const MappingInfo* b) { nit: just for consistency (with your own code below) these (and below on line 92) should be const& not const*. In general IIRC the guideline is to use const& for input arguments, (unless you need nullptr) to distinguish from output arguments. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:104: const size_t invalid = mappings.size(); not a huge difference but I typically see this as -1 or std::numeric_limits<size_t>::max(). Not a huge deal but .size() is a totally useless op here :) Also this should allow some smaller compiler optimiazion below in the if, as the -1 be const propagated as it's known at compile time. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:449: bool DumpFreeSpace() { make this void. You never seeem to sue the return value (which is fine). https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:470: unsigned int hole_cnt = 0; why not making this also a size_t? you are going to cast it anyways https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:477: while (curr != mappings.size() - 1 && shouldn't this be s/-1/-2/, as below you access mappings[curr + 1]. If you have 4 mappings in the vector, you are still looping if curr = (4-1)=3 but then you'll try to dereference mapping[4] which is out of bound https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:509: LogAppend(static_cast<uint16_t>(hole_cnt)); maybe you could do a saturated cast (stick at UINT16_MAX if cnt > that) https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:517: LogAppend(static_cast<uint8_t>(i)); ditto hjere about saturation
https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:477: while (curr != mappings.size() - 1 && On 2016/05/20 15:18:22, Primiano Tucci wrote: > shouldn't this be s/-1/-2/, as below you access mappings[curr + 1]. > If you have 4 mappings in the vector, you are still looping if curr = (4-1)=3 > but then you'll try to dereference mapping[4] which is out of bound It might be better to write curr < mappings.size() - 1, rather than !=, but it's otherwise correct. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:509: LogAppend(static_cast<uint16_t>(hole_cnt)); On 2016/05/20 15:18:22, Primiano Tucci wrote: > maybe you could do a saturated cast (stick at UINT16_MAX if cnt > that) Can you use base::saturated_cast here, or is there some other way of spelling it?
https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:477: while (curr != mappings.size() - 1 && On 2016/05/20 15:32:54, Tobias Sargeant wrote: > On 2016/05/20 15:18:22, Primiano Tucci wrote: > > shouldn't this be s/-1/-2/, as below you access mappings[curr + 1]. > > If you have 4 mappings in the vector, you are still looping if curr = (4-1)=3 > > but then you'll try to dereference mapping[4] which is out of bound > > It might be better to write curr < mappings.size() - 1, rather than !=, but it's > otherwise correct. oh ok forget my comment, I am just wrong. If size=4 you are going to loop only in 0 1 2, which is fine. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:509: LogAppend(static_cast<uint16_t>(hole_cnt)); On 2016/05/20 15:32:55, Tobias Sargeant wrote: > On 2016/05/20 15:18:22, Primiano Tucci wrote: > > maybe you could do a saturated cast (stick at UINT16_MAX if cnt > that) > > Can you use base::saturated_cast here, unfortunately there is no fancy chromium base here > or is there some other way of spelling it? I suppose you could do something like template<typename T> T saturated_cast(uint32_t value) { return value > std::numeric_limits<T>::max() ? std::numeric_limits<T>::max() ? static_cast<T>(value); }
https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:81: bool MappingsAreAdjacent(const MappingInfo* a, const MappingInfo* b) { On 2016/05/20 15:18:22, Primiano Tucci wrote: > nit: just for consistency (with your own code below) these (and below on line > 92) should be const& not const*. > In general IIRC the guideline is to use const& for input arguments, (unless you > need nullptr) to distinguish from output arguments. Done. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:104: const size_t invalid = mappings.size(); On 2016/05/20 15:18:22, Primiano Tucci wrote: > not a huge difference but I typically see this as -1 or > std::numeric_limits<size_t>::max(). > Not a huge deal but .size() is a totally useless op here :) > > Also this should allow some smaller compiler optimiazion below in the if, as the > -1 be const propagated as it's known at compile time. Done. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:449: bool DumpFreeSpace() { On 2016/05/20 15:18:22, Primiano Tucci wrote: > make this void. You never seeem to sue the return value (which is fine). Done. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:470: unsigned int hole_cnt = 0; On 2016/05/20 15:18:22, Primiano Tucci wrote: > why not making this also a size_t? you are going to cast it anyways Done. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:509: LogAppend(static_cast<uint16_t>(hole_cnt)); On 2016/05/20 17:52:48, Primiano Tucci wrote: > On 2016/05/20 15:32:55, Tobias Sargeant wrote: > > On 2016/05/20 15:18:22, Primiano Tucci wrote: > > > maybe you could do a saturated cast (stick at UINT16_MAX if cnt > that) > > > > Can you use base::saturated_cast here, > unfortunately there is no fancy chromium base here > > > or is there some other way of spelling it? > > I suppose you could do something like > > template<typename T> T saturated_cast(uint32_t value) { > return value > std::numeric_limits<T>::max() ? std::numeric_limits<T>::max() ? > static_cast<T>(value); > } > Done. https://codereview.chromium.org/1796803003/diff/120001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:517: LogAppend(static_cast<uint8_t>(i)); On 2016/05/20 15:18:22, Primiano Tucci wrote: > ditto hjere about saturation Done. https://codereview.chromium.org/1796803003/diff/140001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/140001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:527: LogAppend(saturated_cast<uint8_t>(hole_histogram[i])); And added a saturated cast here too, to simplify the histogram generation code above.
PS8 LGTM https://codereview.chromium.org/1796803003/diff/140001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/140001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:527: LogAppend(saturated_cast<uint8_t>(hole_histogram[i])); On 2016/05/23 10:51:59, Tobias Sargeant wrote: > And added a saturated cast here too, to simplify the histogram generation code > above. makes sense. thanks.
Description was changed from ========== Add statistics about free space to microdump format. When a crash occurs as a result of an allocation failure, it is useful to know approximately what regions of the virtual address space remain available, so that we know whether the crash should be attributed to memory fragmentation, or some other cause. BUG=525938 ========== to ========== Add statistics about free space to microdump format. When a crash occurs as a result of an allocation failure, it is useful to know approximately what regions of the virtual address space remain available, so that we know whether the crash should be attributed to memory fragmentation, or some other cause. BUG=525938 R=primiano@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/adca10c8ff9884d5c705fe6... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as adca10c8ff9884d5c705fe60de2e0420ff9e73c6 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1796803003/diff/140001/src/client/linux/micro... File src/client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/1796803003/diff/140001/src/client/linux/micro... src/client/linux/microdump_writer/microdump_writer.cc:501: int log2_hole_sz = Log2Floor(hole_sz); Somebody just reported that this variable is unused and cause some compilation warnings. see https://github.com/google/breakpad/commit/adca10c8ff9884d5c705fe60de2e0420ff9... |
