Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784783003:
On heap tracking datastructure overflow, degrade instead of CHECK() (Closed)
DescriptionOn heap tracking datastructure overflow, degrade instead of CHECK()
This allows the heap profiler to on systems with heavy memory usage
(sometimes caused by strange profiles).
Next step is to surface the overflow on the tracing UI.
BUG=698079
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments. #
Total comments: 3
Messages
Total messages: 17 (11 generated)
ajwong@chromium.org changed reviewers: + dskiba@chromium.org
The CQ bit was checked by ajwong@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
primiano@chromium.org changed reviewers: + primiano@chromium.org
Makes sense to me, just some comments https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.cc (right): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.cc:84: bool AllocationRegister::Insert(const void* address, do we need this bool here? If it's just for the test I'd just keep this a void and check that: - the insert doesn't fail. - the address is not in the list. https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.h (left): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:51: static const KVIndex kInvalidKVIndex = static_cast<KVIndex>(-1); uh why? initializing a static const int-type in a header should always be fine. I see this in other places, e.g.: tools/gn/ordered_set.h base/numerics/safe_conversions_impl.h components/sync/base/enum_set.h Eventually a constexpr might solve ambiguities, but I am not sure that will build on all compilers. maybe try? https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:82: std::numeric_limits<decltype(num_inserts_dropped_)>::max()) { I'd make num_inserts_dropped either a bool or a int64 to avoid this extra check here. with 64-bit, even if you overflow on each instruction, it would take some thousands year before you actually wrap over (by vaguely remembering intel's argument about making the TSC counter a 64-bit register) https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:150: int32_t NumInsertsDropped() const { return num_inserts_dropped_; } nit: lower_with_under() for trivial accessors. https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:191: // NULL if the hash table has run out of memory. s/NULL/nullptr. It's C++11 year :) https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:249: FixedHashMap<NumBuckets, Key, Value, KeyHasher>::kInvalidKVIndex(-1); I never seen a -1 more strongly typed than this. :P
This CL misses a case when backtrace hastable (backtraces_, see InsertBacktrace) overflows. I've seen that happen. I think the counter (num_inserts_dropped_) should be moved from FixedHashMap to AllocationRegister, and should also include failures due to backtrace_ table overflow. Another way is to add dummy "oveflowed" backtrace and use it when we failed to insert a backtrace.
On 2017/03/30 17:57:58, DmitrySkiba wrote: > This CL misses a case when backtrace hastable (backtraces_, see InsertBacktrace) > overflows. I've seen that happen. Oh right, well spotted, you have to if-check also AllocationRegister::InsertBacktrace() > I think the counter (num_inserts_dropped_) should be moved from FixedHashMap to > AllocationRegister, and should also include failures due to backtrace_ table > overflow. That, or in the trace we have to report both the counters from the two hashtables. > Another way is to add dummy "oveflowed" backtrace and use it when we failed to > insert a backtrace. Yeah I guess we could reserve the first entry for an "overlown" backtrace (and return that from InsertBacktrace), so that the allocations would be mapped to the "overflow" backtrace.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ajwong@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.cc (right): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.cc:84: bool AllocationRegister::Insert(const void* address, On 2017/03/30 13:32:40, Primiano Tucci wrote: > do we need this bool here? > If it's just for the test I'd just keep this a void and check that: > - the insert doesn't fail. > - the address is not in the list. I feel like the API is cleaner to have a bool return. Is there a downside to this? https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.h (left): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:51: static const KVIndex kInvalidKVIndex = static_cast<KVIndex>(-1); On 2017/03/30 13:32:40, Primiano Tucci wrote: > uh why? initializing a static const int-type in a header should always be fine. > I see this in other places, e.g.: > tools/gn/ordered_set.h > base/numerics/safe_conversions_impl.h > components/sync/base/enum_set.h > > Eventually a constexpr might solve ambiguities, but I am not sure that will > build on all compilers. maybe try? This is a common C++ mistake that only often doesn't bite people. The relevant spec citation is [class.static.data]p3 """ 3 If a non-volatile const static data member is of integral or enumeration type, its declaration in the class definition can specify a brace-or-equal-initializer in which every initializer-clause that is an assignmentexpression is a constant expression (5.19). A static data member of literal type can be declared in the class definition with the constexpr specifier; if so, its declaration shall specify a brace-or-equal-initializer in which every initializer-clause that is an assignment-expression is a constant expression. [ Note: In both these cases, the member may appear in constant expressions. — end note ] The member shall still be defined in a namespace scope if it is odr-used (3.2) in the program and the namespace scope definition shall not contain an initializer """ Basically, they made an exception for integral types and enums such that you can specify the value in the header WITHOUT turning the line into a _definition_ thereby allowing constant propagation WITHOUT also creating ODR violations. However, technically, you must still define the variable external to the class. Normally you'd just do this in a .cc, but for templates, that doesn't work, so you fallback on yet-another C++ craziness where multiple instantiations of the same template type in multiple translation units are collapsed without creating an ODR violation. Put in simple text, if you have a static const int member in a templated class, you must also write the storage for it explicitly via a partial specialization inside the same headerfile, otherwise *sometimes* (often on weird incremental relinks) you will get a link error due to an undefined symbol (which is how I ran in to this...I got a link error). https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:82: std::numeric_limits<decltype(num_inserts_dropped_)>::max()) { On 2017/03/30 13:32:41, Primiano Tucci wrote: > I'd make num_inserts_dropped either a bool or a int64 to avoid this extra check > here. > with 64-bit, even if you overflow on each instruction, it would take some > thousands year before you actually wrap over (by vaguely remembering intel's > argument about making the TSC counter a 64-bit register) Is that really necessary? The branch prediction on this path is gonna be pretty awesome so I don't know what cost we'd be saving on a our common processors? If we force 64-bit, then it means in all 32-bit builds, we trade a highly-predictable branch for a known 2-instruction operation. https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:150: int32_t NumInsertsDropped() const { return num_inserts_dropped_; } On 2017/03/30 13:32:40, Primiano Tucci wrote: > nit: lower_with_under() for trivial accessors. Done. https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:191: // NULL if the hash table has run out of memory. On 2017/03/30 13:32:40, Primiano Tucci wrote: > s/NULL/nullptr. It's C++11 year :) Done. https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:249: FixedHashMap<NumBuckets, Key, Value, KeyHasher>::kInvalidKVIndex(-1); On 2017/03/30 13:32:41, Primiano Tucci wrote: > I never seen a -1 more strongly typed than this. :P Yeah.... bleh to templates. As noted in chat, we could just replace all this with enum { kInvalidIterator = -1 }; which I did.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Unfortunately while most of this CL looks good to me: - MSVC is unhappy (See the bug linked below) - there is a bug with the case when we overflow of backtraces before allocations (see test attached). Gievn that you are OOO, I took the liberty of rewriting this CL in https://codereview.chromium.org/2787383002/ . I kept all ajwong changes, modulo: - fixing the crash - fixing the -1 MSVC issue - improving the test. - adding the backtrace sentinel See delta patchset there. ajwong: I thought more and we *Really* need a sentinel for the backtrace as dskiba@ suggested. Even if we are very carefully of keeping the -1 around, that will still crash the serializer when accessing the backtrace for an allocation that was recorded when the backtrace table was out of storage. https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.cc (right): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.cc:84: bool AllocationRegister::Insert(const void* address, On 2017/03/31 04:28:30, awong wrote: > On 2017/03/30 13:32:40, Primiano Tucci wrote: > > do we need this bool here? > > If it's just for the test I'd just keep this a void and check that: > > - the insert doesn't fail. > > - the address is not in the list. > > I feel like the API is cleaner to have a bool return. Is there a downside to > this? Not strong enough to justify another round on this. You won. :P https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.h (left): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:51: static const KVIndex kInvalidKVIndex = static_cast<KVIndex>(-1); On 2017/03/31 04:28:31, awong wrote: > On 2017/03/30 13:32:40, Primiano Tucci wrote: > > uh why? initializing a static const int-type in a header should always be > fine. > > I see this in other places, e.g.: > > tools/gn/ordered_set.h > > base/numerics/safe_conversions_impl.h > > components/sync/base/enum_set.h > > > > Eventually a constexpr might solve ambiguities, but I am not sure that will > > build on all compilers. maybe try? > > This is a common C++ mistake that only often doesn't bite people. The relevant > spec citation is [class.static.data]p3 > > """ > 3 If a non-volatile const static data member is of integral or enumeration type, > its declaration in the class > definition can specify a brace-or-equal-initializer in which every > initializer-clause that is an assignmentexpression > is a constant expression (5.19). A static data member of literal type can be > declared in the > class definition with the constexpr specifier; if so, its declaration shall > specify a brace-or-equal-initializer > in which every initializer-clause that is an assignment-expression is a constant > expression. [ Note: In both > these cases, the member may appear in constant expressions. — end note ] The > member shall still be defined > in a namespace scope if it is odr-used (3.2) in the program and the namespace > scope definition shall not > contain an initializer > """ > > Basically, they made an exception for integral types and enums such that you can > specify the value in the header WITHOUT turning the line into a _definition_ > thereby allowing constant propagation WITHOUT also creating ODR violations. > > However, technically, you must still define the variable external to the class. > Normally you'd just do this in a .cc, but for templates, that doesn't work, so > you fallback on yet-another C++ craziness where multiple instantiations of the > same template type in multiple translation units are collapsed without creating > an ODR violation. Put in simple text, if you have a static const int member in a > templated class, you must also write the storage for it explicitly via a partial > specialization inside the same headerfile, otherwise *sometimes* (often on weird > incremental relinks) you will get a link error due to an undefined symbol (which > is how I ran in to this...I got a link error). The defense rests, your honor https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:82: std::numeric_limits<decltype(num_inserts_dropped_)>::max()) { On 2017/03/31 04:28:30, awong wrote: > On 2017/03/30 13:32:41, Primiano Tucci wrote: > > I'd make num_inserts_dropped either a bool or a int64 to avoid this extra > check > > here. > > with 64-bit, even if you overflow on each instruction, it would take some > > thousands year before you actually wrap over (by vaguely remembering intel's > > argument about making the TSC counter a 64-bit register) > > Is that really necessary? The branch prediction on this path is gonna be pretty > awesome so I don't know what cost we'd be saving on a our common processors? > > If we force 64-bit, then it means in all 32-bit builds, we trade a > highly-predictable branch for a known 2-instruction operation. Another battle I don't care enough to respin a discussion. You win also here (but don't get too used to this pattern :P) https://codereview.chromium.org/2784783003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_register.h:249: FixedHashMap<NumBuckets, Key, Value, KeyHasher>::kInvalidKVIndex(-1); On 2017/03/31 04:28:30, awong wrote: > On 2017/03/30 13:32:41, Primiano Tucci wrote: > > I never seen a -1 more strongly typed than this. :P > > Yeah.... bleh to templates. > > As noted in chat, we could just replace all this with > > enum { kInvalidIterator = -1 }; > > which I did. Acknowledged. But MSVC didn't like it unfortunately. https://codereview.chromium.org/2784783003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.cc (right): https://codereview.chromium.org/2784783003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:106: } minor thing, but this branch should return true, as the insert technically succeeded here (by replacing the old entry). https://codereview.chromium.org/2784783003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.cc:163: auto& backtrace_and_count = backtraces_.Get(index); I think now you created a bug here. If you run out of backtraces_ space but not allocations, you can end up with an allocation which is mapped to backtrace index -1, which will BOOM in the Get here (tell me again how useless is that guard page now :P). https://codereview.chromium.org/2784783003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_register.h (right): https://codereview.chromium.org/2784783003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_register.h:51: enum { kInvalidKVIndex = static_cast<KVIndex>(-1) }; unfortunately now we are one bug out and one bug in. Apparently this is not enough for MSVC. See also crbug.com/707288 enum : KVIndex { ...} seems to do the trick |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
