Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1891543003:
[tracing] Turn StackFrame into struct. (Closed)
|
Created:
4 years, 8 months ago by Dmitry Skiba Modified:
4 years, 8 months ago 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] Turn StackFrame into struct.
This change turns StackFrame (aka const char*) into a struct and
introduces 'type' field which controls how stack frame is formatted
when it's written to trace file. As an example, thread name, which
previously was just a string like any other function name, is now
formatted as '[Thread: %s]'.
More stack frame types will be added in the future, for example
native allocation tracing will add 'program counter' type.
BUG=602701
Committed: https://crrev.com/28c3d160bd3817b51e4c9a20353642ad60dca0ea
Cr-Commit-Position: refs/heads/master@{#388553}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Use value to identify StackFrame #Patch Set 3 : rebase #
Total comments: 20
Patch Set 4 : Kill AllocationContext::Empty #Patch Set 5 : Add "the ones closer to main()" #Messages
Total messages: 20 (6 generated)
Description was changed from ========== [tracing] Turn StackFrame into struct. TODO BUG= ========== to ========== [tracing] Turn StackFrame into struct. This change turns StackFrame (aka const char*) into a struct and introduces 'type' field which controls how stack frame is formatted when it's written to trace file. As an example, thread name, which previously was just a string like any other function name, is now formatted as '[Thread: %s]'. More stack frame types will be added in the future, for example native allocation tracing will add 'program counter' type. BUG=602701 ==========
dskiba@google.com changed reviewers: + primiano@chromium.org, ssid@chromium.org
Guys, this is part from 'Implement allocation tracing' CL which just turns StackFrame into a struct. Having stack frame type, and formatting according to that type, has proven useful, and my allocation tracing CL will be in review for some time. Maybe lets land this first, so that we can experiment with adding more stack frame types? Landing this will benefit allocation tracing CL too, because it will become much smaller. Sorry for so many reviews, Primiano :) But this one is basically what is you've already reviewed once, just without native allocation tracing stuff.
Splitting this totally makes sense, thanks for doing it. Overall this change makes lot of sense. THe only thing that I don't buy here is the introduction of frame_count. Probably I am missing something but this seems to make the code harder to maintain and reason about. Are we ever going to push into the backtrace a string pointer or a PC which is nullptr? I can't think of any use case like that. It feels to me that you are introducing the frame_count so that you can push nullptr as a valid stack frame. Not sure we need to do that. What am I missing? https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context.cc (left): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.cc:23: ctx.backtrace.frames[i] = nullptr; if you omit the null initialization of frames here, your equiality operator below will be flaky. YOu will have no guarantee then that two Backtraces with the same amount of frames will be bit identical. I'd keep this nullptr initialization (And ditch the frame_count) https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.cc:16: return std::memcmp(&lhs, &rhs, sizeof(StackFrame)) < 0; can we just compare the value? We'll never have two stack frame with same value but different type, right? https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.cc:20: return std::memcmp(&lhs, &rhs, sizeof(StackFrame)) == 0; same here https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.cc:60: return base::SuperFastHash(reinterpret_cast<const char*>(&frame), as above, here i'd just return std::hash of the |value|, the type is just extra information, but value itself is enough to disambiguate. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.h:44: enum class Type: uintptr_t { out of curiosity why this needs to be a uintptr_t? isn't the default (int IIRC) enough? https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.h:60: bool BASE_EXPORT operator < (const StackFrame& lhs, const StackFrame& rhs); should we turn that map in the deduplicator into an unordered_map, so we just need hash and operator== ? (ok to do that in a separate cl) https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.h:70: size_t frame_count; not sure I understand from the code below why do we need to keep track of the frame count? Isn't just simpler to stop at the first nullptr? This feels to introduce another thing to think about. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker.cc:128: // Add the thread name as the first entry nit: missing period at end of comment. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker.cc:129: if (thread_name_) { nit: you can drop the braces at this point :) https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker.cc:133: for (const auto& event_name: pseudo_stack_) { I think this is regressing w.r.t the previous logic and can have out-of-bound errors. If pseudo_stack.size() >= kMaxFrameCount and you pushed a thread, you will start writing past the end of bakctrace. I quite liked the way the previous for loop was written, and that was definitely safer :) https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc:36: auto actual_bottom = actual + ctx.backtrace.frame_count; why do we need this change? https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc:51: StackFrame null_frame = StackFrame::FromTraceEventName(nullptr); I am not sure I understand the need of this. Why would we need to support nullptr names?
OK, so the key thing is that while StackFrame::value is a const void*, it's not necessarily a valid pointer. It's just a pointer-sized storage for a value which interpretation depends on a type. For now all types have pointer values, but in the future there might be a type with an integer value. Or a type with a pointer value where nullptr has some special meaning. For example, we jumped through hoops to bring thread name in, but now we can just store thread id as value and retrieve thread name when trace file is dumped. That's why I introduced frame_count and made comparison operators / hash to check type too - to make it easy to add more types. And to win some performance I made type to be uintptr_t, so that there is no padding and I can cast StackFrame instances to bytes. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context.cc (left): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.cc:23: ctx.backtrace.frames[i] = nullptr; On 2016/04/14 10:15:33, Primiano Tucci wrote: > if you omit the null initialization of frames here, your equiality operator > below will be flaky. YOu will have no guarantee then that two Backtraces with > the same amount of frames will be bit identical. > I'd keep this nullptr initialization (And ditch the frame_count) Equality operator was changed to compare only frame_count number of elements, so no problem here. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.h:44: enum class Type: uintptr_t { On 2016/04/14 10:15:33, Primiano Tucci wrote: > out of curiosity why this needs to be a uintptr_t? isn't the default (int IIRC) > enough? Because else there will be padding on 64-bit platforms, which will cause comparison and hash functions, which work on struct's bytes, to fail. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.h:60: bool BASE_EXPORT operator < (const StackFrame& lhs, const StackFrame& rhs); On 2016/04/14 10:15:33, Primiano Tucci wrote: > should we turn that map in the deduplicator into an unordered_map, so we just > need hash and operator== ? (ok to do that in a separate cl) Why minimizing number of operators? Yes, we could do that, and remove operator<, but why? https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker.cc:129: if (thread_name_) { On 2016/04/14 10:15:33, Primiano Tucci wrote: > nit: you can drop the braces at this point :) I would prefer not to, unless some style guide says I must :) I really don't understand why NOT having braces around one liners is allowed at all, given Apple's 'goto fail' bug. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker.cc:133: for (const auto& event_name: pseudo_stack_) { On 2016/04/14 10:15:33, Primiano Tucci wrote: > I think this is regressing w.r.t the previous logic and can have out-of-bound > errors. > If pseudo_stack.size() >= kMaxFrameCount and you pushed a thread, you will start > writing past the end of bakctrace. > I quite liked the way the previous for loop was written, and that was definitely > safer :) Hmm, thread is pushed first (at the top of the function), when there surely is a space. This loop checks that we have space for at least one element (backtrace != backtrace_end), and then pushes one more element. So I don't see how it can cause out-of-bound errors. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker_unittest.cc:36: auto actual_bottom = actual + ctx.backtrace.frame_count; On 2016/04/14 10:15:33, Primiano Tucci wrote: > why do we need this change? Because we want to check only valid frames, within frame_count. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc:51: StackFrame null_frame = StackFrame::FromTraceEventName(nullptr); On 2016/04/14 10:15:33, Primiano Tucci wrote: > I am not sure I understand the need of this. > Why would we need to support nullptr names? It's just to assert that nullptr is not a special value anymore.
I'd really prefer that we keep the code simpler and assume that null value == end of stack frames, unless there is a reason to. That makes the code easier to reason about IMHO. I don't see those many other use cases where we expect to push null values as stack. Very likely we're not going to have anything other than char* or void* addresses there, becuase whatever else would be slow. Also, plz don't rely on the memory layout of objects for their sorting. I don't want to have to worry about struct field alignment. drop the memcmp and let's just compare the |value|. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.h:44: enum class Type: uintptr_t { On 2016/04/15 07:01:39, Dmitry Skiba wrote: > On 2016/04/14 10:15:33, Primiano Tucci wrote: > > out of curiosity why this needs to be a uintptr_t? isn't the default (int > IIRC) > > enough? > > Because else there will be padding on 64-bit platforms, which will cause > comparison and hash functions, which work on struct's bytes, to fail. Yeah but see my comment. You shouldn't rely on these detail. Just compare the .value and that will be enough. Don't rely on the memory layout of c++ objects. https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context.h:60: bool BASE_EXPORT operator < (const StackFrame& lhs, const StackFrame& rhs); On 2016/04/15 07:01:39, Dmitry Skiba wrote: > On 2016/04/14 10:15:33, Primiano Tucci wrote: > > should we turn that map in the deduplicator into an unordered_map, so we just > > need hash and operator== ? (ok to do that in a separate cl) > > Why minimizing number of operators? Yes, we could do that, and remove operator<, > but why? Oh is not for minimizing operators, I just realized that that thing should not have been a map in the first place. ANyways, this is not a priority, we can do in a separate CL. these are fine https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker.cc:129: if (thread_name_) { On 2016/04/15 07:01:40, Dmitry Skiba wrote: > On 2016/04/14 10:15:33, Primiano Tucci wrote: > > nit: you can drop the braces at this point :) > > I would prefer not to, unless some style guide says I must :) I really don't > understand why NOT having braces around one liners is allowed at all, given > Apple's 'goto fail' bug. There is no must, indeed. my rationale is: unnecessary braces increase number of lines. Longer files are harder to scroll and read :) up to you really https://codereview.chromium.org/1891543003/diff/1/base/trace_event/heap_profi... base/trace_event/heap_profiler_allocation_context_tracker.cc:133: for (const auto& event_name: pseudo_stack_) { On 2016/04/15 07:01:39, Dmitry Skiba wrote: > On 2016/04/14 10:15:33, Primiano Tucci wrote: > > I think this is regressing w.r.t the previous logic and can have out-of-bound > > errors. > > If pseudo_stack.size() >= kMaxFrameCount and you pushed a thread, you will > start > > writing past the end of bakctrace. > > I quite liked the way the previous for loop was written, and that was > definitely > > safer :) > > Hmm, thread is pushed first (at the top of the function), when there surely is a > space. This loop checks that we have space for at least one element (backtrace > != backtrace_end), and then pushes one more element. So I don't see how it can > cause out-of-bound errors. Oh you are right, I completelly missed the if(backtrace == backtrace_end) part
OK, I agree that it's easier to treat StackFrame as basically a wrapper around a (unique) pointer. We don't need to interpret StackFrame as bytes (which is very fragile), operators become easier, etc. So I made this change. But even with StackFrame-is-pointer I see benefits of frame_count. Or rather, I see only disadvantages of not having it: 1. We have to always fully initialize Backtrace::frames. This is why AllocationContext constructor is private and we have AllocationContext::Empty() - because initializing all backtrace frames is costly and we wanted to avoid that. With frame_count we need to initialize just two values: 'type_name' and 'backtrace.frame_count' (we may even move that to Backtrace ctor). Similarly, AllocationContextTracker::GetContextSnapshot() doesn't have to zero all unused frames, it just sets a single number. 2. Given a Backtrace, we don't know how many valid frames there are, so we have to either check each value as we're iterating them (as StackFrameDeduplicator::Insert did), or determine how many valid values we have before iterating them (as GetSubbuckets in heap_profiler_heap_dump_write.cc did). Again, frame_count gives us that answer clearly. Yes, adding 'frame_count' incurs overhead. But that overhead is small: each StackFrame is 8 bytes (on 32-bit), and we have 12 of them, so frame_count inflates Backtrace by ~4%. Even when StackFrame was just a pointer, overhead was just 8%. So I don't understand why you don't like frame_count, and why it wasn't there to begin with.
Ping. Please review.
Ok looked at this with a fresh (kind of) mind. frame_count makes sense. Just please at this point let's get rid of Empty() and let's make everything in the default ctor. Your new implementation is so fast that is not worth having two ctors (the default one and Empty()) just to avoid two zero-write to frame_count and the value. Plz just look at comments, I think I have just a major one about end = begin + backtrace.frame_count (+1) It's late and very likely I'm just missing something obvious about the +1 there. I think your code there is eating one frame, but at the same time not sure why the test still would pass in that case. The other are minor comments and I trust your judgement there. LGTM with comments addressed. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.cc:24: return lhs.value != rhs.value; small thing. I'd probably just implement this as !(lhs == rhs), so if in future somebody changes ==, this doesn't go out of sync. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.cc:32: AllocationContext ctx; At this point can we remove this and make this into the ctor? This perf optimization between the default ctor and Empty does not make any sense anymore with your change. Let's keep the code easier (Empty is just used by tests) https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:57: const void* value; since both of these are const char* should this also stay const char* so you can avoid the casting below? https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:65: // If the stack is higher than what can be stored here, the bottom frames small req, not really related with your change: can you add a "(the one closer to main() ) to "the bottom frames"? When it comes to stack for me is always a pain to work out what people mean with top and bottom when it comes to a stack :) https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:133: for (const auto& event_name: pseudo_stack_) { to be honest this would probably be more readable if you did just s/const auto& event_name/const char* event_name/ https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:83: const StackFrame* end = begin + backtrace.frame_count; shouldn't this be begin + frame_count + 1 If I am reading correctly the code below is expecting end to be past the last element, right? Here if count == 0 start == end. while in the previous code if the thing is empty end = start + 1 Or am I misreading something? https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_stack_frame_deduplicator.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_stack_frame_deduplicator.cc:92: break; maybe add a default: NOTREACHED() (not 100% sure that won't cause a compiler error though, try) https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc:50: TEST(StackFrameDeduplicatorTest, SingleBacktraceWithNull) { thanks, really appreciate this :)
https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.cc:24: return lhs.value != rhs.value; On 2016/04/19 19:45:06, Primiano Tucci wrote: > small thing. I'd probably just implement this as !(lhs == rhs), so if in future > somebody changes ==, this doesn't go out of sync. Done. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.cc:32: AllocationContext ctx; On 2016/04/19 19:45:06, Primiano Tucci wrote: > At this point can we remove this and make this into the ctor? This perf > optimization between the default ctor and Empty does not make any sense anymore > with your change. Let's keep the code easier (Empty is just used by tests) Done. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:57: const void* value; On 2016/04/19 19:45:06, Primiano Tucci wrote: > since both of these are const char* should this also stay const char* so you can > avoid the casting below? Yeah, but my next CL adds PROGRAM_COUNTER which is 'const void*' type. So it's better to use 'const void*' from the start to avoid unnecessary changes. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:65: // If the stack is higher than what can be stored here, the bottom frames On 2016/04/19 19:45:06, Primiano Tucci wrote: > small req, not really related with your change: can you add a "(the one closer > to main() ) to "the bottom frames"? When it comes to stack for me is always a > pain to work out what people mean with top and bottom when it comes to a stack > :) Actually, that is not true when it comes to native traces, hmm... The way it works now is that I request (kMaxFrameCount + kKnownFrameCount) frames and get top-bottom frames. I.e. to get bottom kMaxFrameCount ones I'll have to ask for larger number of frames and then position to the bottom. However, I think that the way it works now (reporting kMaxFrameCount top frames) is more useful, because you see exact trace which caused an allocation. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:133: for (const auto& event_name: pseudo_stack_) { On 2016/04/19 19:45:06, Primiano Tucci wrote: > to be honest this would probably be more readable if you did just > s/const auto& event_name/const char* event_name/ Done. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:83: const StackFrame* end = begin + backtrace.frame_count; On 2016/04/19 19:45:06, Primiano Tucci wrote: > shouldn't this be begin + frame_count + 1 > If I am reading correctly the code below is expecting end to be past the last > element, right? > Here if count == 0 start == end. while in the previous code if the thing is > empty end = start + 1 > Or am I misreading something? I think this is the same - previously if all frames were nullptrs, code would rewind 'end' to 'begin' (last iteration is where end = begin + 1, *(end - 1) is nullptr, so we go inside and end--, and then we stop looping because begin == end. And if there are N non-nullptr frames, previously we would stop looping when *(end - 1) != nullptr, i.e. when end == (begin + N). https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_stack_frame_deduplicator.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_stack_frame_deduplicator.cc:92: break; On 2016/04/19 19:45:06, Primiano Tucci wrote: > maybe add a default: NOTREACHED() (not 100% sure that won't cause a compiler > error though, try) Actually, since we're switching on enum (enum class even) value, compiler issues a warning, which is treated as error, if not all values are handled: ../../base/trace_event/heap_profiler_stack_frame_deduplicator.cc:82:13: error: enumeration value 'BLA' not handled in switch [-Werror,-Wswitch] switch (frame.type) { adding 'default: NOTREACHED()' suppresses that, i.e. converts compile time error into runtime error. Latest thread about that on chromium-dev: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/A1NV70Hx980 https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc:50: TEST(StackFrameDeduplicatorTest, SingleBacktraceWithNull) { On 2016/04/19 19:45:06, Primiano Tucci wrote: > thanks, really appreciate this :) Acknowledged :)
https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:57: const void* value; On 2016/04/19 22:14:14, Dmitry Skiba wrote: > On 2016/04/19 19:45:06, Primiano Tucci wrote: > > since both of these are const char* should this also stay const char* so you > can > > avoid the casting below? > > Yeah, but my next CL adds PROGRAM_COUNTER which is 'const void*' type. So it's > better to use 'const void*' from the start to avoid unnecessary changes. yeah I realized this too late. ignore my comment. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:65: // If the stack is higher than what can be stored here, the bottom frames On 2016/04/19 22:14:14, Dmitry Skiba wrote: > On 2016/04/19 19:45:06, Primiano Tucci wrote: > > small req, not really related with your change: can you add a "(the one closer > > to main() ) to "the bottom frames"? When it comes to stack for me is always a > > pain to work out what people mean with top and bottom when it comes to a stack > > :) > > Actually, that is not true when it comes to native traces, hmm... The way it > works now is that I request (kMaxFrameCount + kKnownFrameCount) frames and get > top-bottom frames. I.e. to get bottom kMaxFrameCount ones I'll have to ask for > larger number of frames and then position to the bottom. > > However, I think that the way it works now (reporting kMaxFrameCount top frames) > is more useful, because you see exact trace which caused an allocation. should we keep it consistent and just bump the depth when we use native frames? Otherwise if you have two functions that reach inside the same final path, but start for different points, they won't align. I'd keep this this so that main() (or whatever) is always there. If necessary bump the MaxFrameCount. THe all clustering logic might get screwed if you keep only the most leaf functions. https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:83: const StackFrame* end = begin + backtrace.frame_count; On 2016/04/19 22:14:14, Dmitry Skiba wrote: > On 2016/04/19 19:45:06, Primiano Tucci wrote: > > shouldn't this be begin + frame_count + 1 > > If I am reading correctly the code below is expecting end to be past the last > > element, right? > > Here if count == 0 start == end. while in the previous code if the thing is > > empty end = start + 1 > > Or am I misreading something? > > I think this is the same - previously if all frames were nullptrs, code would > rewind 'end' to 'begin' (last iteration is where end = begin + 1, *(end - 1) is > nullptr, so we go inside and end--, and then we stop looping because begin == > end. > > And if there are N non-nullptr frames, previously we would stop looping when > *(end - 1) != nullptr, i.e. when end == (begin + N). Ok you are right. your code is consistent with the previous one. I just wonder whether even the original one is bugged in the case of an empty stacktrace. I think we should hit that DCHECK if begin == end. Maybe we early out at some earlier point? Mind checking if that is the case? (not sure if is it covered by some unittest) https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_stack_frame_deduplicator.cc (right): https://codereview.chromium.org/1891543003/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_stack_frame_deduplicator.cc:92: break; On 2016/04/19 22:14:14, Dmitry Skiba wrote: > On 2016/04/19 19:45:06, Primiano Tucci wrote: > > maybe add a default: NOTREACHED() (not 100% sure that won't cause a compiler > > error though, try) > > Actually, since we're switching on enum (enum class even) value, compiler issues > a warning, which is treated as error, if not all values are handled: > > ../../base/trace_event/heap_profiler_stack_frame_deduplicator.cc:82:13: error: > enumeration value 'BLA' not handled in switch [-Werror,-Wswitch] > switch (frame.type) { > > adding 'default: NOTREACHED()' suppresses that, i.e. converts compile time error > into runtime error. Latest thread about that on chromium-dev: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/A1NV70Hx980 Sg, leave it as it is then.
The CQ bit was checked by dskiba@google.com
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/1891543003/#ps80001 (title: "Add "the ones closer to main()"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891543003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891543003/80001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Turn StackFrame into struct. This change turns StackFrame (aka const char*) into a struct and introduces 'type' field which controls how stack frame is formatted when it's written to trace file. As an example, thread name, which previously was just a string like any other function name, is now formatted as '[Thread: %s]'. More stack frame types will be added in the future, for example native allocation tracing will add 'program counter' type. BUG=602701 ========== to ========== [tracing] Turn StackFrame into struct. This change turns StackFrame (aka const char*) into a struct and introduces 'type' field which controls how stack frame is formatted when it's written to trace file. As an example, thread name, which previously was just a string like any other function name, is now formatted as '[Thread: %s]'. More stack frame types will be added in the future, for example native allocation tracing will add 'program counter' type. BUG=602701 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1907593002/ by dskiba@google.com. The reason for reverting is: Broke build on Windows x64 (warning was treated as error): heap_profiler_allocation_context.cc(58): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1911433003/ by cmumford@chromium.org. The reason for reverting is: Caused tree breakage..
Message was sent while issue was closed.
Description was changed from ========== [tracing] Turn StackFrame into struct. This change turns StackFrame (aka const char*) into a struct and introduces 'type' field which controls how stack frame is formatted when it's written to trace file. As an example, thread name, which previously was just a string like any other function name, is now formatted as '[Thread: %s]'. More stack frame types will be added in the future, for example native allocation tracing will add 'program counter' type. BUG=602701 ========== to ========== [tracing] Turn StackFrame into struct. This change turns StackFrame (aka const char*) into a struct and introduces 'type' field which controls how stack frame is formatted when it's written to trace file. As an example, thread name, which previously was just a string like any other function name, is now formatted as '[Thread: %s]'. More stack frame types will be added in the future, for example native allocation tracing will add 'program counter' type. BUG=602701 Committed: https://crrev.com/28c3d160bd3817b51e4c9a20353642ad60dca0ea Cr-Commit-Position: refs/heads/master@{#388553} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/28c3d160bd3817b51e4c9a20353642ad60dca0ea Cr-Commit-Position: refs/heads/master@{#388553} |
