Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2134683002:
tracing v2: Add base class for zero-copy protobuf (ProtoZero) (Closed)
|
Created:
4 years, 5 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 5 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, kraynov Base URL:
https://chromium.googlesource.com/chromium/src.git@proto_utils Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing v2: Add base class for zero-copy protobuf (ProtoZero)
This CL introduces one of the major building blocks for Tracing V2.
ProtoZeroMessage is the base class that the generated C++ stubs
(from the .proto files) will extend and use. This is, in essence,
the equivalent of the libprotobuf runtime used by conventional
protos, reduced to the bare minimum required for the append-only
API surface we care about in Tracing V2.
This class allows in-flight serialization of proto fields directly
into the underlying ScatteredStreamBuffer, without performing any
copy nor requiring any dynamic memory allocation.
This class is not meant to be instantiated directly by the tracing
clients. The next CLs will introduce a TracingBufferWriter which
will glue together the ring buffer implementation and return
event proto objects from that.
BUG=608719
Committed: https://crrev.com/cf033562024e494ac7a8edd01ef64a4cdfcaf9c6
Cr-Commit-Position: refs/heads/master@{#405482}
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : . #Patch Set 4 : rebase #Patch Set 5 : Rebase #Patch Set 6 : add comments #
Total comments: 76
Patch Set 7 : alph review #
Total comments: 2
Patch Set 8 : petrcermak review #
Total comments: 14
Patch Set 9 : oysteine + 2nd petrcermak review + rebase #Patch Set 10 : rebase #
Messages
Total messages: 27 (10 generated)
Description was changed from ========== add proto_zero_message BUG= ========== to ========== tracing v2: Add base class for zero-copy protobuf (ProtoZero) This CL introduces one of the major building blocks for Tracing V2. ProtoZeroMessage is the base class that the generated C++ stubs (from the .proto files) will extend and use. This is, in essence, the equivalent of the libprotobuf runtime used by conventional protos, reduced to the bare minimum required for the append-only API surface we care about in Tracing V2. This class allows in-flight serialization of proto fields directly into the underlying ScatteredStreamBuffer, without performing any copy nor requiring any dynamic memory allocation. This class is not meant to be instantiated directly by the tracing clients. The next CLs will introduce a TracingBufferWriter which will glue together the ring buffer implementation and return event proto objects from that. BUG=608719 ==========
primiano@chromium.org changed reviewers: + alph@chromium.org, oysteine@chromium.org, petrcermak@chromium.org
well well well, let's get to the juicy pieces finally. ;-)
lgtm https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_utils.h:31: constexpr size_t kMaxMessageLength = 1u << (kMessageLengthFieldSize * 7); -1 to be precise. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:33: kMaxNestingDepth * (sizeof(ProtoZeroMessage) - nit: offsetof https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:70: T* message = reinterpret_cast<T*>(nested_messages_arena_); How about adding a static assert here that sizeof(T) == sizeof(ProtoZeroMessage) Also consider using a placement new instead of reinterpret_cast. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message_unittest.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:201: for (uint32_t i = 0; i < 128; ++i) { may get signed/unsigned comparison error on some platforms. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:210: msg->AppendVarIntU32(3, 42); Test EndNestedMessage & Finalize calls on nested messages as well.
https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_utils.h:31: constexpr size_t kMaxMessageLength = 1u << (kMessageLengthFieldSize * 7); On 2016/07/12 00:20:42, alph wrote: > -1 to be precise. oh good catch. Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:33: kMaxNestingDepth * (sizeof(ProtoZeroMessage) - On 2016/07/12 00:20:42, alph wrote: > nit: offsetof Hmm if you don't mind I'd leave this as it is. The final math would be the same, but I feel offsetof would make it a bit more unreadable. What I really want to say here is: the arena should be big enough to contain kMax elements, without counting the arena itself in nested elements. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:70: T* message = reinterpret_cast<T*>(nested_messages_arena_); On 2016/07/12 00:20:42, alph wrote: > How about adding a static assert here that sizeof(T) == sizeof(ProtoZeroMessage) Oh yes definitely, we really need this to prevent subclasses to introduce extra state (they are auto generate though, but you never know). At some point I though to that but then I forgot along the way. Thanks. > Also consider using a placement new instead of reinterpret_cast. So this is interesting, in the beginning I was doing a placement new indeed. The problem with placement new is that invokes the constructor (which itself is a good thing). The problem with that is that I found out that uninitialized fields can be actually initialized (the usual C++ undefined behavior thing). Specifically what was happening is that in clang debug builds the arena gets initialized even if it shouldn't. As a result the arena of nested messages (which by design overflows the parent storage) did clobber the stack/heap (Depending where the root message was allocated). This, instead, avoids to invoke any ctor and avoid any unwanted initialization. I checked this code locally with Asan and it seems happy. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message_unittest.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:201: for (uint32_t i = 0; i < 128; ++i) { On 2016/07/12 00:20:43, alph wrote: > may get signed/unsigned comparison error on some platforms. Hm there seem to be quite a few of this: https://cs.chromium.org/search/?q=for%5C+%5C(uint.*%3C%5C+%5Cd%2B&sq=package:... I think in cases like this the compiler can see that 128 can be promoted safely to unsigned. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:210: msg->AppendVarIntU32(3, 42); On 2016/07/12 00:20:42, alph wrote: > Test EndNestedMessage & Finalize calls on nested messages as well. Done for Finalize. Also you reminded me to add a test to check that finalize is actually idempotent. EndNestedMessage instead is an internal impl. detail (is called by finalize).
Looks good overall. Here's a bunch https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:22: // Do add any code here, use the Reset() method below. s/Do/Don't/ ? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:22: // Do add any code here, use the Reset() method below. nit: s/below/below instead/ https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:24: // of root (non-nested) messages. Nested messages are allocated into the nit: s/into/in/ (I've never heard "allocated into") https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:47: sealed_ = false; You don't actually DCHECK this anywhere. Shouldn't it be DCHECK'ed in every single Append* method? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:52: if (nested_message_) You do this a lot. How about EndNestedMessageIfNecessary() instead? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:64: // TODO(kraynov): this could be perf-optimized. See http://crbug.com/624311 . nit: remove unnecessary space before period. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:105: uint8_t* data_end = In most other methods you declare and assign to |data_end| separately. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:142: uint8_t* data_end = ditto (separate declaration and assignment) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:33: // Optional. If is_valid() the corresponding memory region (which length nit: add a comma after "is_valid()" https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:33: // Optional. If is_valid() the corresponding memory region (which length "which length" is very difficult to understand. "with length"? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:33: // Optional. If is_valid() the corresponding memory region (which length In what sense is it "Optional"? Is it always present when is_valid() is true? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:75: void EndNestedMessage(); Maybe add a comment saying that this is called by Finalize and Append* methods? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:94: // Only PODs fields are allowed. This class' dtor is never called. nit: "POD fields" (no need for the double plural) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:94: // Only PODs fields are allowed. This class' dtor is never called. supernit: From what I could find, "class's" is preferred over "class'" (even though both are correct). https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:111: // When true no more changes to the message are allowed. This is to DCHECK nit: add comma after "true" https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:118: // message per nesting level as he proto-zero API contract mandates that s/as he/as the/ https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:119: // nested fields can be filled only in a stacked fashion. In othwe words, s/othwe/other/ (one key to the right :-D) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:121: // parent messag (or the parent message itself is finalized) and cannot be nit: s/messag\b/message/ https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:122: // accessed anymore after that point. nit: s/after that point/afterwards/ https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:132: // DO NOT add any fields below nested_messages_arena_. The memory layout of shouldn't this be "|nested_message_arena|"? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:133: // nested messaged would overflow the storage allocated by the root message. s/messaged/messages/ https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message_unittest.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:42: Shouldn't you also reset |messages_|? Otherwise, the tests will potentially affect each other. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:96: // Field id > 32 are expected to be varint encoded (preamble > 1 byte) nit: s/id/ids/ (because of "are") https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:178: GetNextSerializedBytes(3); Why don't you call EXPECT_EQ on this anyway (also applies to line 186)? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:181: // 203 bytes for |nest_mesg_2| (see below)+ 5 bytes for its preamble + nit: missing space between "below)" and "+". https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:182: // 2 bytes for the 0x43 varint = 210 bytes. nit: double space before "=" https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:199: for (size_t depth = 0; depth < ProtoZeroMessage::kMaxNestingDepth; ++depth) { These two loops would be much more readable if implemented as a recursive helper function. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:214: std::string full_buf = GetNextSerializedBytes(GetNumSerializedBytes()); Maybe at least check the length as well? You could check the contents of the buffer with another recursive helper function. Checking just the hash is too black-box in my opinion.
... of comments (sent the previous message too soon). Thanks, Petr
kraynov@chromium.org changed reviewers: + kraynov@chromium.org
lgtm https://codereview.chromium.org/2134683002/diff/120001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/120001/components/tracing/cor... components/tracing/core/proto_zero_message.h:134: ALIGNAS(sizeof(void*)) uint8_t nested_messages_arena_[512]; Please explain how are you going to construct this object, since this approach is illegal for stack-allocated objects.
https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:22: // Do add any code here, use the Reset() method below. On 2016/07/12 10:18:46, petrcermak wrote: > s/Do/Don't/ ? EHm, yeah :) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:22: // Do add any code here, use the Reset() method below. On 2016/07/12 10:18:46, petrcermak wrote: > nit: s/below/below instead/ Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:24: // of root (non-nested) messages. Nested messages are allocated into the On 2016/07/12 10:18:46, petrcermak wrote: > nit: s/into/in/ (I've never heard "allocated into") In my mind "allocated into" is about the actual storage (e.g., allocated into X pages, allocated into that buffer), while "allocated in" is more about the context (allocated in foobar.cc, allocated in MethodName()) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:47: sealed_ = false; On 2016/07/12 10:18:47, petrcermak wrote: > You don't actually DCHECK this anywhere. EPIC FAIL. good spot, yes. :P > Shouldn't it be DCHECK'ed in every single Append* method? Just WriteToStream and Finalize is enough. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:52: if (nested_message_) On 2016/07/12 10:18:47, petrcermak wrote: > You do this a lot. How about EndNestedMessageIfNecessary() instead? I knew that you were going to suggest it. I though to that but IMHO is more readable having the full line. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:64: // TODO(kraynov): this could be perf-optimized. See http://crbug.com/624311 . On 2016/07/12 10:18:46, petrcermak wrote: > nit: remove unnecessary space before period. It's on purpose, so that the clickable link does not contain the dot. It's a "finesse", essentially :) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:105: uint8_t* data_end = On 2016/07/12 10:18:47, petrcermak wrote: > In most other methods you declare and assign to |data_end| separately. I knew also this :P There is a reason. In the other methods the uint8_t* data_end = bla would wrap over two lines. Since I have to spend two lines I find more legible having the declaration and assignment being separate (and the assignment fits in one line). In this case however even the assignment itself won't fit on a single line, so why bothering with the separate declaration? https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:142: uint8_t* data_end = On 2016/07/12 10:18:46, petrcermak wrote: > ditto (separate declaration and assignment) ditto (assignment doesn't fit in one line) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:33: // Optional. If is_valid() the corresponding memory region (which length On 2016/07/12 10:18:47, petrcermak wrote: > In what sense is it "Optional"? Is it always present when is_valid() is true? Optional in the sense that you can not call set_size_field (in which case is_valid() == false) at all. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:33: // Optional. If is_valid() the corresponding memory region (which length On 2016/07/12 10:18:47, petrcermak wrote: > "which length" is very difficult to understand. "with length"? its should make it clearer. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:33: // Optional. If is_valid() the corresponding memory region (which length On 2016/07/12 10:18:47, petrcermak wrote: > nit: add a comma after "is_valid()" Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:75: void EndNestedMessage(); On 2016/07/12 10:18:47, petrcermak wrote: > Maybe add a comment saying that this is called by Finalize and Append* methods? Done. actually realized that this should be private. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:94: // Only PODs fields are allowed. This class' dtor is never called. On 2016/07/12 10:18:47, petrcermak wrote: > supernit: From what I could find, "class's" is preferred over "class'" (even > though both are correct). "The Elements of Style and the Canadian Press Stylebook prefer the form in -s's with the exception of Biblical and classical proper names and common phrases that do not take the extra s" I found this class pretty biblical :P (in the sense that took ages to me to write) Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:94: // Only PODs fields are allowed. This class' dtor is never called. On 2016/07/12 10:18:47, petrcermak wrote: > nit: "POD fields" (no need for the double plural) Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:111: // When true no more changes to the message are allowed. This is to DCHECK On 2016/07/12 10:18:47, petrcermak wrote: > nit: add comma after "true" Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:118: // message per nesting level as he proto-zero API contract mandates that On 2016/07/12 10:18:47, petrcermak wrote: > s/as he/as the/ Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:119: // nested fields can be filled only in a stacked fashion. In othwe words, On 2016/07/12 10:18:47, petrcermak wrote: > s/othwe/other/ (one key to the right :-D) Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:121: // parent messag (or the parent message itself is finalized) and cannot be On 2016/07/12 10:18:47, petrcermak wrote: > nit: s/messag\b/message/ Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:122: // accessed anymore after that point. On 2016/07/12 10:18:47, petrcermak wrote: > nit: s/after that point/afterwards/ Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:132: // DO NOT add any fields below nested_messages_arena_. The memory layout of On 2016/07/12 10:18:47, petrcermak wrote: > shouldn't this be "|nested_message_arena|"? Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:133: // nested messaged would overflow the storage allocated by the root message. On 2016/07/12 10:18:47, petrcermak wrote: > s/messaged/messages/ Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message_unittest.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:42: On 2016/07/12 10:18:47, petrcermak wrote: > Shouldn't you also reset |messages_|? True. Done. > Otherwise, the tests will potentially affect each other. Not really, the entire class is destroyed and recreated for each fixture, do the dtor would have caught this. The reason why I do these explicitly here is because crashes in the dtor are always harder to debug. See https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:96: // Field id > 32 are expected to be varint encoded (preamble > 1 byte) On 2016/07/12 10:18:48, petrcermak wrote: > nit: s/id/ids/ (because of "are") Done. Also the right number is 16 not 32 :) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:178: GetNextSerializedBytes(3); On 2016/07/12 10:18:47, petrcermak wrote: > Why don't you call EXPECT_EQ on this anyway (also applies to line 186)? I don't care too much agbout the contents of these 3 bytes (they are already covered by the simpler test above). I just need to skip 3 bytes to get to the juicy ones. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:181: // 203 bytes for |nest_mesg_2| (see below)+ 5 bytes for its preamble + On 2016/07/12 10:18:47, petrcermak wrote: > nit: missing space between "below)" and "+". Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:182: // 2 bytes for the 0x43 varint = 210 bytes. On 2016/07/12 10:18:47, petrcermak wrote: > nit: double space before "=" Done. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:199: for (size_t depth = 0; depth < ProtoZeroMessage::kMaxNestingDepth; ++depth) { On 2016/07/12 10:18:48, petrcermak wrote: > These two loops would be much more readable if implemented as a recursive helper > function. uh? not sure I agree, then you'd have to jump up and down to see what the helper does. The content of these loops is pretty meaningless, I just need to append *something* https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:214: std::string full_buf = GetNextSerializedBytes(GetNumSerializedBytes()); On 2016/07/12 10:18:47, petrcermak wrote: > Maybe at least check the length as well? Very hard you can get a different length with the same hash. > You could check the contents of the > buffer with another recursive helper function. Checking just the hash is too > black-box in my opinion. So my theory here is: if you screw something, some other test (with more details) should fail. The real purpose of this test is to see if any code path crashes. added a comment. https://codereview.chromium.org/2134683002/diff/120001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/120001/components/tracing/cor... components/tracing/core/proto_zero_message.h:134: ALIGNAS(sizeof(void*)) uint8_t nested_messages_arena_[512]; On 2016/07/12 10:31:03, kraynov wrote: > Please explain how are you going to construct this object, since this approach > is illegal for stack-allocated objects. It is not, that is the neat thing. This works even if you stack allocate the root message.
LGTM with some final comments. Thanks, Petr https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:24: // of root (non-nested) messages. Nested messages are allocated into the On 2016/07/12 17:25:16, Primiano Tucci wrote: > On 2016/07/12 10:18:46, petrcermak wrote: > > nit: s/into/in/ (I've never heard "allocated into") > > In my mind "allocated into" is about the actual storage (e.g., allocated into X > pages, allocated into that buffer), while "allocated in" is more about the > context (allocated in foobar.cc, allocated in MethodName()) Acknowledged. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:52: if (nested_message_) On 2016/07/12 17:25:16, Primiano Tucci wrote: > On 2016/07/12 10:18:47, petrcermak wrote: > > You do this a lot. How about EndNestedMessageIfNecessary() instead? > > I knew that you were going to suggest it. I though to that but IMHO is more > readable having the full line. And I knew this was going to be your reply :-P https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:64: // TODO(kraynov): this could be perf-optimized. See http://crbug.com/624311 . On 2016/07/12 17:25:16, Primiano Tucci wrote: > On 2016/07/12 10:18:46, petrcermak wrote: > > nit: remove unnecessary space before period. > > It's on purpose, so that the clickable link does not contain the dot. > It's a "finesse", essentially :) Acknowledged. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:105: uint8_t* data_end = On 2016/07/12 17:25:16, Primiano Tucci wrote: > On 2016/07/12 10:18:47, petrcermak wrote: > > In most other methods you declare and assign to |data_end| separately. > > I knew also this :P > There is a reason. In the other methods the uint8_t* data_end = bla would wrap > over two lines. Since I have to spend two lines I find more legible having the > declaration and assignment being separate (and the assignment fits in one line). > > In this case however even the assignment itself won't fit on a single line, so > why bothering with the separate declaration? Acknowledged. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:142: uint8_t* data_end = On 2016/07/12 17:25:16, Primiano Tucci wrote: > On 2016/07/12 10:18:46, petrcermak wrote: > > ditto (separate declaration and assignment) > > ditto (assignment doesn't fit in one line) Acknowledged. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message.h:94: // Only PODs fields are allowed. This class' dtor is never called. On 2016/07/12 17:25:17, Primiano Tucci wrote: > On 2016/07/12 10:18:47, petrcermak wrote: > > supernit: From what I could find, "class's" is preferred over "class'" (even > > though both are correct). > > "The Elements of Style and the Canadian Press Stylebook prefer the form in -s's > with the exception of Biblical and classical proper names and common phrases > that do not take the extra s" > I found this class pretty biblical :P (in the sense that took ages to me to > write) > > Done. :-D :-D :-D https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message_unittest.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:178: GetNextSerializedBytes(3); On 2016/07/12 17:25:17, Primiano Tucci wrote: > On 2016/07/12 10:18:47, petrcermak wrote: > > Why don't you call EXPECT_EQ on this anyway (also applies to line 186)? > > I don't care too much agbout the contents of these 3 bytes (they are already > covered by the simpler test above). I just need to skip 3 bytes to get to the > juicy ones. Acknowledged. https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:199: for (size_t depth = 0; depth < ProtoZeroMessage::kMaxNestingDepth; ++depth) { On 2016/07/12 17:25:17, Primiano Tucci wrote: > On 2016/07/12 10:18:48, petrcermak wrote: > > These two loops would be much more readable if implemented as a recursive > helper > > function. > > uh? not sure I agree, then you'd have to jump up and down to see what the helper > does. The content of these loops is pretty meaningless, I just need to append > *something* I still think that the following would be more readable: void BuildNestedMessage(size_t depth, ProtoZeroMessage* msg) { for (uint32_t i = 0; i < 128; ++i) { msg->AppendBytes(1, kTestBytes, sizeof(kTestBytes)); } ProtoZeroMessage* child_msg = msg->BeginNestedMessage<FakeMessage>(2); if (depth >= ProtoZeroMessage::kMaxNestingDepth - 1) { BuildNestedMessage(depth + 1, child_msg); msg->AppendVarIntU32(3, 42); } } BuildNestedMessage(0, root_msg); but it's of course up to you ;-) https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:214: std::string full_buf = GetNextSerializedBytes(GetNumSerializedBytes()); On 2016/07/12 17:25:17, Primiano Tucci wrote: > On 2016/07/12 10:18:47, petrcermak wrote: > > Maybe at least check the length as well? > Very hard you can get a different length with the same hash. > > > You could check the contents of the > > buffer with another recursive helper function. Checking just the hash is too > > black-box in my opinion. > So my theory here is: if you screw something, some other test (with more > details) should fail. > The real purpose of this test is to see if any code path crashes. > added a comment. Acknowledged. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:72: // introduce extra state fields (they won't be initialized by Reset()). nit: s/they won't/which wouldn't/ would sound better in my opinion.
Extremely excited by this :D https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... File components/tracing/core/proto_zero_message.cc (right): https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:27: static_assert(base::is_trivially_destructible<ProtoZeroMessage>::value, Why are these static_asserts in the constructor? Shouldn't they be in the class declaration instead? It'd make it a little clearer that the constructor is (and should be) empty. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:102: DCHECK_LT(size, proto::kMaxMessageLength); Feels like this should be a stronger check (CHECK_LT?). Or is the idea that it's the stream writer's responsibility to check for buffer overruns? https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:119: DCHECK(!sealed_); DCHECK_IS_ON() (at least that's used around the DCHECK(sealed_) in the header, I assume that's because the variable is always referenced?) https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:73: static_assert(sizeof(T) == sizeof(ProtoZeroMessage), Maybe an std::is_base_of assert too. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:94: inline void WriteToStream(const uint8_t* src_begin, const uint8_t* src_end) { nit: isn't 'inline' redundant here? https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:98: const size_t size = static_cast<size_t>(src_end - src_begin); DCHECK(src_begin < src_end) maybe?
https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... File components/tracing/core/proto_zero_message_unittest.cc (right): https://codereview.chromium.org/2134683002/diff/100001/components/tracing/cor... components/tracing/core/proto_zero_message_unittest.cc:199: for (size_t depth = 0; depth < ProtoZeroMessage::kMaxNestingDepth; ++depth) { On 2016/07/12 17:41:17, petrcermak wrote: > On 2016/07/12 17:25:17, Primiano Tucci wrote: > > On 2016/07/12 10:18:48, petrcermak wrote: > > > These two loops would be much more readable if implemented as a recursive > > helper > > > function. > > > > uh? not sure I agree, then you'd have to jump up and down to see what the > helper > > does. The content of these loops is pretty meaningless, I just need to append > > *something* > > I still think that the following would be more readable: > > void BuildNestedMessage(size_t depth, ProtoZeroMessage* msg) { > for (uint32_t i = 0; i < 128; ++i) { > msg->AppendBytes(1, kTestBytes, sizeof(kTestBytes)); > } > ProtoZeroMessage* child_msg = msg->BeginNestedMessage<FakeMessage>(2); > if (depth >= ProtoZeroMessage::kMaxNestingDepth - 1) { > BuildNestedMessage(depth + 1, child_msg); > msg->AppendVarIntU32(3, 42); > } > } > > BuildNestedMessage(0, root_msg); > > but it's of course up to you ;-) ahhhhh I completely misunderstood what you meant. I though you just wanted a helper for the AppendBytes/Varint, not fot the actual nested construction. Yeah a recursive setter makes way more sense. done. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... File components/tracing/core/proto_zero_message.cc (right): https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:27: static_assert(base::is_trivially_destructible<ProtoZeroMessage>::value, On 2016/07/12 21:03:29, oystein wrote: > Why are these static_asserts in the constructor? Shouldn't they be in the class > declaration instead? It'd make it a little clearer that the constructor is (and > should be) empty. Two things here: 1) If I add them to the .h that will make compilation slower for all translation units that pull that header (possibly a lot) 2) the cocnret problem is that the static_assert args are still subjected to public/private protection rules, it's a bit weird. If I just put it at the end of the header I get an error "nested_message_arena_ is a private field yada yada". So I have to put this code inside a context which is allowed to access private fields, even if this is just a compile time check. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:102: DCHECK_LT(size, proto::kMaxMessageLength); On 2016/07/12 21:03:29, oystein wrote: > Feels like this should be a stronger check (CHECK_LT?). Or is the idea that it's > the stream writer's responsibility to check for buffer overruns? this cannot overrun and cause memory corruption. Even if you pass a gigantic buffer here, the stream_writer_ will split the write into chunks and keep requesting chunks until the buffer has been written (or the delegate returns a nullptr, in which case *boom*). The only reason of this dcheck is that if you manage to write a gigantic buffer, the size of the message will be truncated to 4 bytes and the importer will fail. Given that it is unlikely and doesn't cause a memory corruption, doesn't feel it justifies a CHECK. Having said this I am removing this DCHECK here at all, thinking more this dcheck is just misplaced here, for the following reason: - There is no problem here writing a large buffer, as this size is actually encoded as a conventional (i.e. non-fixed-size) varint. - The thing that will be truncated is the size field of the entire message (that one is a fixed-length varint) - But the size field of the message can overflow even if a single write doesn't (imagine you make 10000 calls to AppendBytes with a small size). The place there this check should be in place is ::Finalize(). Here is pointless and confusing. removed. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.cc:119: DCHECK(!sealed_); On 2016/07/12 21:03:29, oystein wrote: > DCHECK_IS_ON() (at least that's used around the DCHECK(sealed_) in the header, I > assume that's because the variable is always referenced?) Oops yes. forgot this at last minute. I suppose this is what made most bots red :) https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:72: // introduce extra state fields (they won't be initialized by Reset()). On 2016/07/12 17:41:17, petrcermak wrote: > nit: s/they won't/which wouldn't/ would sound better in my opinion. Done. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:73: static_assert(sizeof(T) == sizeof(ProtoZeroMessage), On 2016/07/12 21:03:29, oystein wrote: > Maybe an std::is_base_of assert too. TIL std::is_base_of. Excellent, done. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:94: inline void WriteToStream(const uint8_t* src_begin, const uint8_t* src_end) { On 2016/07/12 21:03:29, oystein wrote: > nit: isn't 'inline' redundant here? This is something I never really remember.I think you are right. Also at the end the compiler will always decide what to do. https://codereview.chromium.org/2134683002/diff/140001/components/tracing/cor... components/tracing/core/proto_zero_message.h:98: const size_t size = static_cast<size_t>(src_end - src_begin); On 2016/07/12 21:03:29, oystein wrote: > DCHECK(src_begin < src_end) maybe? Done.
lgtm!
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, kraynov@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2134683002/#ps160001 (title: "oysteine + 2nd petrcermak review + rebase")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, alph@chromium.org, kraynov@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2134683002/#ps180001 (title: "rebase")
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 base class for zero-copy protobuf (ProtoZero) This CL introduces one of the major building blocks for Tracing V2. ProtoZeroMessage is the base class that the generated C++ stubs (from the .proto files) will extend and use. This is, in essence, the equivalent of the libprotobuf runtime used by conventional protos, reduced to the bare minimum required for the append-only API surface we care about in Tracing V2. This class allows in-flight serialization of proto fields directly into the underlying ScatteredStreamBuffer, without performing any copy nor requiring any dynamic memory allocation. This class is not meant to be instantiated directly by the tracing clients. The next CLs will introduce a TracingBufferWriter which will glue together the ring buffer implementation and return event proto objects from that. BUG=608719 ========== to ========== tracing v2: Add base class for zero-copy protobuf (ProtoZero) This CL introduces one of the major building blocks for Tracing V2. ProtoZeroMessage is the base class that the generated C++ stubs (from the .proto files) will extend and use. This is, in essence, the equivalent of the libprotobuf runtime used by conventional protos, reduced to the bare minimum required for the append-only API surface we care about in Tracing V2. This class allows in-flight serialization of proto fields directly into the underlying ScatteredStreamBuffer, without performing any copy nor requiring any dynamic memory allocation. This class is not meant to be instantiated directly by the tracing clients. The next CLs will introduce a TracingBufferWriter which will glue together the ring buffer implementation and return event proto objects from that. BUG=608719 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== tracing v2: Add base class for zero-copy protobuf (ProtoZero) This CL introduces one of the major building blocks for Tracing V2. ProtoZeroMessage is the base class that the generated C++ stubs (from the .proto files) will extend and use. This is, in essence, the equivalent of the libprotobuf runtime used by conventional protos, reduced to the bare minimum required for the append-only API surface we care about in Tracing V2. This class allows in-flight serialization of proto fields directly into the underlying ScatteredStreamBuffer, without performing any copy nor requiring any dynamic memory allocation. This class is not meant to be instantiated directly by the tracing clients. The next CLs will introduce a TracingBufferWriter which will glue together the ring buffer implementation and return event proto objects from that. BUG=608719 ========== to ========== tracing v2: Add base class for zero-copy protobuf (ProtoZero) This CL introduces one of the major building blocks for Tracing V2. ProtoZeroMessage is the base class that the generated C++ stubs (from the .proto files) will extend and use. This is, in essence, the equivalent of the libprotobuf runtime used by conventional protos, reduced to the bare minimum required for the append-only API surface we care about in Tracing V2. This class allows in-flight serialization of proto fields directly into the underlying ScatteredStreamBuffer, without performing any copy nor requiring any dynamic memory allocation. This class is not meant to be instantiated directly by the tracing clients. The next CLs will introduce a TracingBufferWriter which will glue together the ring buffer implementation and return event proto objects from that. BUG=608719 Committed: https://crrev.com/cf033562024e494ac7a8edd01ef64a4cdfcaf9c6 Cr-Commit-Position: refs/heads/master@{#405482} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cf033562024e494ac7a8edd01ef64a4cdfcaf9c6 Cr-Commit-Position: refs/heads/master@{#405482} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
