Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2049653002:
tracing v2: Add TraceRingBuffer class. (Closed)
|
Created:
4 years, 6 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 5 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. |
Descriptiontracing v2: Add TraceRingBuffer class.
TraceRingBuffer handles the chunking of the large continuous
tracing buffer. Some features, such as the support for shared
metadata (interned string index, process name, etc.), are still
missing.
BUG=608719
Committed: https://crrev.com/9294ab8ff1484d31d48803e51a160ba0d0c8e368
Cr-Commit-Position: refs/heads/master@{#403135}
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase + oysteine review #
Total comments: 30
Patch Set 3 : petrcermak + fix win #
Total comments: 4
Patch Set 4 : missing nits #
Dependent Patchsets: Messages
Total messages: 31 (14 generated)
The CQ bit was checked by primiano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049653002/1
primiano@chromium.org changed reviewers: + kraynov@chromium.org, oysteine@chromium.org
If you agree I'd start upstreaming the pieces even if they are not fully final yet. Some initial notes. My proposal is to temporarily keep all these classes under a tracing::v2 namespace, so during the interim period is clear what is v2 and what is not, and drop "v2" only when everything is done. The rational with core/ (as opposite to common) is that: 1. I don't expect pieces of the codebase to depend on these classes, those are impl details. /components/X/core/ seems a pattern (https://cs.chromium.org/search/?q=f:components/%5Cw%2B/core&sq=package:chromi...) 2. These classes should depend only on base and not on the content layer (not sure if there is a way to enforce this via deps)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2016/06/08 10:04:08, Primiano Tucci wrote: > If you agree I'd start upstreaming the pieces even if they are not fully final > yet. > Some initial notes. My proposal is to temporarily keep all these classes under a > tracing::v2 namespace, so during the interim period is clear what is v2 and what > is not, and drop "v2" only when everything is done. > The rational with core/ (as opposite to common) is that: > 1. I don't expect pieces of the codebase to depend on these classes, those are > impl details. /components/X/core/ seems a pattern > (https://cs.chromium.org/search/?q=f:components/%5Cw%2B/core&sq=package:chromi...) > 2. These classes should depend only on base and not on the content layer (not > sure if there is a way to enforce this via deps) core/ makes sense to me, if the goal is to follow something like https://sites.google.com/a/chromium.org/dev/developers/design-documents/struc... Having both common/ and core/ seems a little confusing to me though. I'll admit I'm a little fuzzy on what 'common is supposed to be. https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:27: void Initialize(uint8_t* begin, uint8_t* end); Maybe 'end' can just be omitted? Looks like end = begin + kSize is pretty embedded in the usage (especially since kSize is a member of Chunk). Otherwise maybe kSize should be moved to TraceRingBuffer::kChunkSize. https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:38: uint32_t proto_used_size() const { Should this layer be proto aware? Maybe it could be juset set_used_size() / used_size(). https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:42: bool is_returned() const { return owner_ == base::kInvalidThreadId; } is_returned() / and set_returned() was a little confusing to me. Maybe is_owned() / clear_owner() instead?
primiano@chromium.org changed reviewers: + petrcermak@chromium.org
Hmm looks like Oystein is OOO for two weeks. Petr can I ask you to cover these initial CLs? addressed Oystein comments in the meantime https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:27: void Initialize(uint8_t* begin, uint8_t* end); On 2016/06/15 07:52:52, Oystein (OOO to July11) wrote: > Maybe 'end' can just be omitted? Looks like end = begin + kSize is pretty > embedded in the usage (especially since kSize is a member of Chunk). Otherwise > maybe kSize should be moved to TraceRingBuffer::kChunkSize. Done. https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:38: uint32_t proto_used_size() const { On 2016/06/15 07:52:52, Oystein (OOO to July11) wrote: > Should this layer be proto aware? Maybe it could be juset set_used_size() / > used_size(). Yup you are right, this is not proto aware. Renamed to set_used_size() https://codereview.chromium.org/2049653002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:42: bool is_returned() const { return owner_ == base::kInvalidThreadId; } On 2016/06/15 07:52:52, Oystein (OOO to July11) wrote: > is_returned() / and set_returned() was a little confusing to me. Maybe > is_owned() / clear_owner() instead? Done.
On 2016/06/15 07:52:52, Oystein (OOO to July11) wrote: > Having both common/ and core/ seems a little confusing to me though. I'll admit > I'm a little fuzzy on what 'common is supposed to be. Ah the difference, in my understanding, is that common/ is code that runs both in browser and child and can be depended on by both. core/ is an internal implementatio detail. In this case still both browser+renderer but other pieces of the codebase shouldn't depend on it.
As always looks very good overall. I've left a few inline comment. One description nit: There's a missing comma after "etc)": "[...], such as the support for shared metadata (interned string index, process name, etc), [...]". Thanks, Petr https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:14: DCHECK_GE(size, Chunk::kSize); maybe check DCHECK_GT(num_chunks_, 0) since that's what you're really after? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:20: chunk_begin = chunk_begin + Chunk::kSize; chunk_begin += Chunk::kSize alternatively, use multiplication (it makes the code more readable in my opinion): for (size_t i = 0; i < num_chunks_; ++i) chunks_[i].Initialize(chunk_begin + i * Chunk::kSize) https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:30: Chunk* chunk = nullptr; Why don't you declare this inside the loop on line 32??? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:41: // Bankrupcy: there are more threads than chunks. All chunks were on flight. nit: s/on/in/ https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:44: uint8_t* const begin = &bankrupcy_chunk_storage_.get()[0]; Opinion: I don't think that naming this "begin" makes the code any more readable, so I'd just fold the variable. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:53: base::AutoLock lock(lock_); Would it make sense to DCHECK that the chunk is actually in |chunks_| or would it hurt performance too much? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:63: void TraceRingBuffer::Chunk::Clear() { Shouldn't this also clear owner? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:22: static const size_t kSize = 32768; Would it make sense to define this with a bit shift (to guarantee that it's a power of two)? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:43: void clear_owner() { owner_ = base::kInvalidThreadId; } perhaps call |set_owner(base::kInvalidThreadId)| ? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:49: // Accesses to owner_ must happen under the buffer |lock_|. nit: "|owner_|" https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:50: base::PlatformThreadId owner_; // kInvalidThreadId -> Chunk is returned. You mean has been returned? Is to be reused? Is *being* returned? Returned *where*? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:72: std::unique_ptr<uint8_t[]> bankrupcy_chunk_storage_; would it make sense to rename this to |emergency_chunk_storage_| to keep the comment and variable name in sync? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:72: std::unique_ptr<uint8_t[]> bankrupcy_chunk_storage_; Just to clarify, it can be the case that multiple threads think that they own the bankrupcy chunk storage and write over each other (so the data in it is most likely going to be complete garbage)? https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... File components/tracing/core/trace_ring_buffer_unittest.cc (right): https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer_unittest.cc:7: #include "base/bit_cast.h" I don't think you're actually using this. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer_unittest.cc:45: for (size_t i = 0; i < kPayloadSize; ++i) { do you need the braces here?
Description was changed from ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc) are still missing. BUG=608719 ========== to ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc), are still missing. BUG=608719 ==========
Patchset #3 (id:40001) has been deleted
Thanks! https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:14: DCHECK_GE(size, Chunk::kSize); On 2016/06/28 10:22:29, petrcermak wrote: > maybe check DCHECK_GT(num_chunks_, 0) since that's what you're really after? makes sense, thanks. Done https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:20: chunk_begin = chunk_begin + Chunk::kSize; On 2016/06/28 10:22:29, petrcermak wrote: > chunk_begin += Chunk::kSize > > alternatively, use multiplication (it makes the code more readable in my > opinion): > > for (size_t i = 0; i < num_chunks_; ++i) > chunks_[i].Initialize(chunk_begin + i * Chunk::kSize) Oh definitely. The only reason why it is that way is because that was the result of a lot of re-formattings on my side, and didn't realize that I ended up with a += at the end. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:30: Chunk* chunk = nullptr; On 2016/06/28 10:22:29, petrcermak wrote: > Why don't you declare this inside the loop on line 32??? Same as above, historical evolution of this code. That's why I needed a review :) Thanks. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:41: // Bankrupcy: there are more threads than chunks. All chunks were on flight. On 2016/06/28 10:22:29, petrcermak wrote: > nit: s/on/in/ Done. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:44: uint8_t* const begin = &bankrupcy_chunk_storage_.get()[0]; On 2016/06/28 10:22:29, petrcermak wrote: > Opinion: I don't think that naming this "begin" makes the code any more > readable, so I'd just fold the variable. Oh, as above. Before there was a "begin" and "end", which made more sense :) Agree, now that folding is the way. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:53: base::AutoLock lock(lock_); On 2016/06/28 10:22:29, petrcermak wrote: > Would it make sense to DCHECK that the chunk is actually in |chunks_| or would > it hurt performance too much? Hmm that would make the debug (or DCHECK_ALWAYS_ON) code really slow, as I expect we'll have quite some chunks and the dcheck should have to iterate over them. This should be quite unlikely to happen, as the chunk ctor is private IIRC and the only way to get a chunk is via TakeChunk. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:63: void TraceRingBuffer::Chunk::Clear() { On 2016/06/28 10:22:29, petrcermak wrote: > Shouldn't this also clear owner? No this one is deliberate. I mean whether conceptually it should happen is borderline. In practice the owner is always reset by the caller after the clear so this would end up in rendundant code as we'd clear and immediately reset the owner.
You seem to have missed some of my previous comments. Also, I've just realized that you should do s/etc/etc./ in the patch description. Apart from these two things, LGTM :-) Petr
LGTM if my comments does not point a problem. https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:13: : num_chunks_(size / Chunk::kSize), current_chunk_idx_(0) { What if (size % Chunk::kSize) != 0 ? https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:42: bool is_owned() const { return owner_ != base::kInvalidThreadId; } Is there a guarantee that is_owned(), clear_owner() and set_owner() are going to be called in the same thread?
Description was changed from ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc), are still missing. BUG=608719 ========== to ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc.), are still missing. BUG=608719 ==========
Patchset #4 (id:80001) has been deleted
Oops sorry for the missing comments. Hope I addressed all of them this time. 2nd round :) https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:22: static const size_t kSize = 32768; On 2016/06/28 10:22:30, petrcermak wrote: > Would it make sense to define this with a bit shift (to guarantee that it's a > power of two)? Doesn't necessarily have to be a power of two IIRC. 32768 makes it easier to tell that is 32k. Maybe better 32 * 1024, fixed. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:43: void clear_owner() { owner_ = base::kInvalidThreadId; } On 2016/06/28 10:22:29, petrcermak wrote: > perhaps call |set_owner(base::kInvalidThreadId)| ? not a huge difference really, and technically lower_methods should be only trivial accessors. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:49: // Accesses to owner_ must happen under the buffer |lock_|. On 2016/06/28 10:22:29, petrcermak wrote: > nit: "|owner_|" Done. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:50: base::PlatformThreadId owner_; // kInvalidThreadId -> Chunk is returned. On 2016/06/28 10:22:29, petrcermak wrote: > You mean has been returned? Is to be reused? Is *being* returned? Returned > *where*? reworded as "not owned". It is the terminology we use today in TraceLog where threads "return" chunk to the main buffer. But I agree that without that context it's not clear as a cmment. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:72: std::unique_ptr<uint8_t[]> bankrupcy_chunk_storage_; On 2016/06/28 10:22:29, petrcermak wrote: > would it make sense to rename this to |emergency_chunk_storage_| to keep the > comment and variable name in sync? Hmm bankrupcy gives me more the idea that you give up on something. Emergency makes me feel as if we are still trying to make things work properly, which is not the case here. We're just trying not to crash. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:72: std::unique_ptr<uint8_t[]> bankrupcy_chunk_storage_; On 2016/06/28 10:22:30, petrcermak wrote: > Just to clarify, it can be the case that multiple threads think that they own > the bankrupcy chunk storage and write over each other (so the data in it is most > likely going to be complete garbage)? Correct. If this is used this will contain complete garbage. But this is not part of the begin-end range so will never be consumed. Is only a wait to make writers not crash. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... File components/tracing/core/trace_ring_buffer_unittest.cc (right): https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer_unittest.cc:7: #include "base/bit_cast.h" On 2016/06/28 10:22:30, petrcermak wrote: > I don't think you're actually using this. Good spot. I think this is going to be introduced in the future and I mistakenly cherry-picked this line too early. https://codereview.chromium.org/2049653002/diff/20001/components/tracing/core... components/tracing/core/trace_ring_buffer_unittest.cc:45: for (size_t i = 0; i < kPayloadSize; ++i) { On 2016/06/28 10:22:30, petrcermak wrote: > do you need the braces here? Technically it's not mandatory to omit them, but yes, minimizing the numers of line is the goal of my life :P https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:13: : num_chunks_(size / Chunk::kSize), current_chunk_idx_(0) { On 2016/06/28 13:15:16, kraynov wrote: > What if (size % Chunk::kSize) != 0 ? we'll waste the reminder and use less than the passed buffer. shouldn't cause problems right? https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2049653002/diff/60001/components/tracing/core... components/tracing/core/trace_ring_buffer.h:42: bool is_owned() const { return owner_ != base::kInvalidThreadId; } On 2016/06/28 13:15:16, kraynov wrote: > Is there a guarantee that is_owned(), clear_owner() and set_owner() are going to > be called in the same thread? No, see below, the expectations is that they are going to be accessed under the lock_. Unfortunatelly I don't have a reference here to the parent obj so cannot DCHECK that the lock is held. But on the other side, these are impl details used only by the parent class.
lgtm
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2049653002/#ps100001 (title: "missing nits")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc.), are still missing. BUG=608719 ========== to ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc.), are still missing. BUG=608719 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc.), are still missing. BUG=608719 ========== to ========== tracing v2: Add TraceRingBuffer class. TraceRingBuffer handles the chunking of the large continuous tracing buffer. Some features, such as the support for shared metadata (interned string index, process name, etc.), are still missing. BUG=608719 Committed: https://crrev.com/9294ab8ff1484d31d48803e51a160ba0d0c8e368 Cr-Commit-Position: refs/heads/master@{#403135} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9294ab8ff1484d31d48803e51a160ba0d0c8e368 Cr-Commit-Position: refs/heads/master@{#403135} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
