Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197563002:
tracing v2: minor refactoring to TraceRingBuffer test helpers (Closed)
|
Created:
4 years, 4 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 4 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, oystein (OOO til 10th of July) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing v2: minor refactoring to TraceRingBuffer test helpers
Minor refactoring in preparation of the chunk writer logic,
mostly about testing helper methods that will be shared by
upcoming unittests.
This CL also introduces a ReserveBytesUnsafe() method to
ScatteredStreamWriter which will be used by the writer
fast-paths.
BUG=608719
TEST=TraceRingBufferTest.*,ScatteredStreamWriterTest.*
Committed: https://crrev.com/9911b654f723c480228e9bbfc376222eebe34297
Cr-Commit-Position: refs/heads/master@{#410035}
Patch Set 1 #
Total comments: 14
Patch Set 2 : alph review #Patch Set 3 : constexpr instead of const to fix linking errors #Patch Set 4 : move const outside of class #Patch Set 5 : git cl format #
Total comments: 2
Patch Set 6 : simpler check #
Dependent Patchsets: Messages
Total messages: 35 (25 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/v2/patch-status/codereview.chromium.or...
primiano@chromium.org changed reviewers: + alph@chromium.org
quite boring, just need to make the next cl less scary.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/sca... File components/tracing/core/scattered_stream_writer.h (right): https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/sca... components/tracing/core/scattered_stream_writer.h:59: template <size_t size> uint8_t* ReserveBytesUnsafe() { Why it needs to be a template? Won't a simple function with a const parameter once inlined produce the same result? https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:64: chunk->clear_owner(); If you make the owner_ atomic you could omit locking on returning the chunk. https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:65: num_chunks_taken_--; ditto https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:69: return chunk->begin() >= chunks_[num_chunks_ - 1].end() || I believe unrelated pointer comparison is an undefined behavior. E.g. if chunk eventually happens to be the bankrupcy_chunk_ https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:80: owner_(base::kInvalidThreadId), owner_(0) https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:51: void set_owner(uint32_t owner) { owner_ = owner; } DCHECK(owner != 0) https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:55: uint32_t owner_; // 0 -> Chunk is not owned. static const uint32_t kNoOwner = 0; ?
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/v2/patch-status/codereview.chromium.or...
thanks https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/sca... File components/tracing/core/scattered_stream_writer.h (right): https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/sca... components/tracing/core/scattered_stream_writer.h:59: template <size_t size> uint8_t* ReserveBytesUnsafe() { On 2016/07/30 07:41:14, alph_slow wrote: > Why it needs to be a template? > Won't a simple function with a const parameter once inlined produce the same > result? Fair point. I think I was just thinking to our previous conversation on some other function, but here makes total sense just an arg. I checked and gets effectively inlined on release (at least without dheck_is_on) https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:64: chunk->clear_owner(); On 2016/07/30 07:41:14, alph_slow wrote: > If you make the owner_ atomic you could omit locking on returning the chunk. I was thinking to this but I'd wait to have the code that reads this before. The think is: either these can become acquire/release semantic (which are free on x86 and lighter on arm), but for that I have to see clearly what the reader is doing. If I just make atomic exchanges, two atomic exchanges (or increment) are going to be as expensive as a lock (and still prone to be misused). Added a TODO https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:69: return chunk->begin() >= chunks_[num_chunks_ - 1].end() || On 2016/07/30 07:41:14, alph_slow wrote: > I believe unrelated pointer comparison is an undefined behavior. E.g. if chunk > eventually happens to be the bankrupcy_chunk_ Hmm not sure I follow. this is comparing just uint8_t* pointers of chunks.begin / chunk.end ? In the case of chunk = bankrupcy_chunk that must have a begin / end that is not within the chunk's storage area. Where is the undefined behavior? https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:80: owner_(base::kInvalidThreadId), On 2016/07/30 07:41:14, alph_slow wrote: > owner_(0) Oops. https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... File components/tracing/core/trace_ring_buffer.h (right): https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:51: void set_owner(uint32_t owner) { owner_ = owner; } On 2016/07/30 07:41:14, alph_slow wrote: > DCHECK(owner != 0) Done. https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.h:55: uint32_t owner_; // 0 -> Chunk is not owned. On 2016/07/30 07:41:15, alph_slow wrote: > static const uint32_t kNoOwner = 0; ? Done.
https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2197563002/diff/1/components/tracing/core/tra... components/tracing/core/trace_ring_buffer.cc:69: return chunk->begin() >= chunks_[num_chunks_ - 1].end() || Thinking more, I think this would be UB if I was comparing the chunk vs &chunks_[x] (the chunks pointer themselves). But here I am comparing really the value of **fields** of three chunks, which is just a member variable set on construction. I don't think this is any different then begin() and end() being an int value rather than uint8_t*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/v2/patch-status/codereview.chromium.or...
primiano@chromium.org changed reviewers: + oysteine@chromium.org
+oystein, welcome back :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2197563002/diff/80001/components/tracing/core... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2197563002/diff/80001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:71: return chunk->begin() >= chunks_[num_chunks_ - 1].end() || I'm not sure how it will break ;-) but C standard says that comparing two pointers from distinct objects is an UB.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2197563002/diff/80001/components/tracing/core... File components/tracing/core/trace_ring_buffer.cc (right): https://codereview.chromium.org/2197563002/diff/80001/components/tracing/core... components/tracing/core/trace_ring_buffer.cc:71: return chunk->begin() >= chunks_[num_chunks_ - 1].end() || On 2016/08/04 19:27:35, alph_slow wrote: > I'm not sure how it will break ;-) but C standard says that comparing two > pointers from distinct objects is an UB. "If two pointers p and q of the same type point to different objects that are not members of the same object or elements of the same array or to different functions, or if only one of them is null, the results of p<q, p>q, p<=q, and p>=q are unspecified." So my reading is that what they are saying here is: if you have function whatever() { int a[4]; int b[4]; assert(&a[2] < &a[3]) is defined behavior. assert(&a[1] < &b[1]) is UB } In other words, you should not rely on what is the stack layout of two variables (or similarly if they are heap allocated). The only think you can rely on is arrays and field of a struct/class, as those are defined. Here instead my case is: start() and end() of a chunk contain the address of: A. either a portion of the a big contiguous array (for the real chunks). B. a disjoint array (for the bankrupcy chunk). Now, the UB here would be if I rely on the fact that A is < or > B, because I can't possibly know where any of them would be allocated relative to each other. But here I'm just checking whether they overlap or not, which I believe is defined. Having said this, reading this code made me realize that my implementation here is totally moronic. I just need to check whether chunk == &bankrupcy_chunk_ and that's all :) I moved this code from the unittest (which did not have access to the TRB internals) and I didn't realize that now I do :)
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2197563002/#ps100001 (title: "simpler check")
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.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== tracing v2: minor refactoring to TraceRingBuffer test helpers Minor refactoring in preparation of the chunk writer logic, mostly about testing helper methods that will be shared by upcoming unittests. This CL also introduces a ReserveBytesUnsafe() method to ScatteredStreamWriter which will be used by the writer fast-paths. BUG=608719 TEST=TraceRingBufferTest.*,ScatteredStreamWriterTest.* ========== to ========== tracing v2: minor refactoring to TraceRingBuffer test helpers Minor refactoring in preparation of the chunk writer logic, mostly about testing helper methods that will be shared by upcoming unittests. This CL also introduces a ReserveBytesUnsafe() method to ScatteredStreamWriter which will be used by the writer fast-paths. BUG=608719 TEST=TraceRingBufferTest.*,ScatteredStreamWriterTest.* Committed: https://crrev.com/9911b654f723c480228e9bbfc376222eebe34297 Cr-Commit-Position: refs/heads/master@{#410035} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9911b654f723c480228e9bbfc376222eebe34297 Cr-Commit-Position: refs/heads/master@{#410035} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
