Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043913006:
tracing v2: Add proto_utils.h (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@tv_ups2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing v2: Add proto_utils.h
Adds basic support for proto encoding. This will be used by the
TracingV2 core classes which, for performance reasons, need
to serialize protos independently of libprotobuf.
More discussion in the design doc: bit.ly/TracingV2
BUG=608719
Committed: https://crrev.com/557ed0bee30e466fd37e1c1a40a8ed26654dbf96
Cr-Commit-Position: refs/heads/master@{#404671}
Patch Set 1 #
Total comments: 10
Patch Set 2 : address review comments #
Total comments: 8
Patch Set 3 : remove inline from WriteRedundantVarIntUnsigned #
Total comments: 22
Patch Set 4 : alph+petr review #
Total comments: 4
Patch Set 5 : nit + fix last test #Patch Set 6 : rebase #Patch Set 7 : rebase #
Dependent Patchsets: Messages
Total messages: 28 (11 generated)
primiano@chromium.org changed reviewers: + kraynov@chromium.org, oysteine@chromium.org
https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:72: inline void WriteRedundantVarIntUnsigned(uint32_t value, Can you expand a bit on why this is necessary? Fixed-size varints sounds self-contradictory to me :). Is this because the proto wire format only supports varint encoding for integers? (It may also be pushing the limits of what should be an inlined function, especially since it pulls in DCHECK etc).
https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:72: inline void WriteRedundantVarIntUnsigned(uint32_t value, On 2016/06/15 08:45:44, Oystein wrote: > Can you expand a bit on why this is necessary? Fixed-size varints sounds > self-contradictory to me :). Is this because the proto wire format only supports > varint encoding for integers? Right. So the problem here is that when you have length-limited fields, such as strings or nested messages, the proto encoding format mandates that their size must be written using VarInt (no option for fixed int). The only thing that can be "fixed" are numeric fields, which can be fixed32/64, but the size field of nested objects is varint. It's not a problem for strings, as we can strlen them upfront. But for nested messages, we cannot know the size upfront. Hence the trick of reserving 4 bytes and using redundant encoding. Apparently this technique is already used internally, for the same design reasons.See go/hklai . > > (It may also be pushing the limits of what should be an inlined function, > especially since it pulls in DCHECK etc). So, the reason why I made this inline is because IIRC is used only in one place really. But happy to make it non-inline and hope/expect that the compiler will do that.
https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:37: // 64-bit fixed-length field types: fixed32, sfixed32, float. nit: 32 https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:66: // TODO(kraynov): add support for signed integers and zig-zag encoding. WriteVarIntS32 (S for zig-zag) WriteVarIntS64 WriteVarInt32 WriteVarInt64 These names?
https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:23: kFieldTypeLengthLimited = 2, ...LengthDelimited For consistency with https://developers.google.com/protocol-buffers/docs/encoding https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:33: inline uint32_t MakeTagLengthLimited(uint32_t field_id) { ditto Delimited
primiano@chromium.org changed reviewers: + petrcermak@chromium.org
+Petr https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:23: kFieldTypeLengthLimited = 2, On 2016/07/05 11:25:09, kraynov wrote: > ...LengthDelimited > For consistency with > https://developers.google.com/protocol-buffers/docs/encoding Done. https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:33: inline uint32_t MakeTagLengthLimited(uint32_t field_id) { On 2016/07/05 11:25:09, kraynov wrote: > ditto Delimited Done. https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:37: // 64-bit fixed-length field types: fixed32, sfixed32, float. On 2016/06/22 16:38:32, kraynov wrote: > nit: 32 Done. https://codereview.chromium.org/2043913006/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:66: // TODO(kraynov): add support for signed integers and zig-zag encoding. On 2016/06/22 16:38:32, kraynov wrote: > WriteVarIntS32 (S for zig-zag) > WriteVarIntS64 > WriteVarInt32 > WriteVarInt64 > > These names? Yup, correct. But no rush, we won't need them until much later.
Description was changed from ========== tracing v2: Add proto_utils.h Adds basic support for proto encoding. BUG=608719 ========== to ========== tracing v2: Add proto_utils.h Adds basic support for proto encoding. This will be used by the TracingV2 core classes which, for performance reasons, need to serialize protos independently of libprotobuf. More discussion in the design doc: bit.ly/TracingV2 BUG=608719 ==========
alph@chromium.org changed reviewers: + alph@chromium.org
https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:48: inline uint8_t* WriteVarIntInternal(T value, uint8_t* target) { Maybe add a static check that T is unsigned? https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:50: *target = static_cast<uint8_t>(value | 0x80); nit: *target++ = https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:76: size_t length, As it seems to be a hot path, I'd better make it a template with length parameter. So the compiler could inline and unroll the loop. https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:67: EXPECT_VARINT32_EQ("\x00", 1, 0); nit: you can drop the length arg and infer it from the expected value.
Looks very good overall. Here's just a couple of code style comments (sorry for being so pedantic). Thanks, Petr https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:28: inline uint32_t MakeTagVarInt(uint32_t field_id) { Does it really make sense to define this for each type? It seems that defining a generic function would be just as good: inline uint32_t MakeTag(unit32_t field_type, uint32_t field_id) { return (field_id << 3) | field_type; } The calls would have almost the same length and be equally readable in my opinion: MakeTagLengthLimited(1234); MakeTag(kFieldTypeLengthLimited, 1234); I suppose it would then make sense to name the enum so that the compiler could check the type of the |field_type|. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:58: inline uint8_t* WriteVarIntU32(uint32_t value, uint8_t* target) { Again, does it really make sense to define these? I think that calling the templated function would be equally readable. Furthermore, this would simplify the testing code (see my comment below). https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:72: // Concretely this is used when writing a nested message, where the size of nit: comma after "Concretely". I'd personally prefer "In particular, ..." https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:73: // nested message is not known until it is ended. In this case a fixed amount of nit: s/nested/the nested/ https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:73: // nested message is not known until it is ended. In this case a fixed amount of supernit: s/ended/finished/ https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:73: // nested message is not known until it is ended. In this case a fixed amount of nit: comma after "In this case" https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:75: void WriteRedundantVarIntUnsigned(uint32_t value, size_t length, uint8_t* buf) { Should the name be "VarInt" whe it's fixed-size? Also, I think it should be "U32" instead of "Unsigned" to be consistent with WriteVarIntU32 et al. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:15: bool VarIntHelperU32(const char* expected, size_t length, uint32_t value) { If you expose the templated version of WriteVar... instead, you can template this function as well to avoid code duplication: template <typename T> bool VarIntHelper(const char* expected, size_t length, T value) { ... } https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:15: bool VarIntHelperU32(const char* expected, size_t length, uint32_t value) { I think "helper" is a bad name here because it doesn't give the reader any idea what the function does. I think something like "CheckWriteVarInt" would be much better. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:42: EXPECT_EQ(0x08u, MakeTagVarInt(1)); Could you also check this with zero? Thanks! https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:119: WriteRedundantVarIntUnsigned(0x332211, sizeof(buf), buf); I suggest you also try writing the largest number that still fits into the 4 bytes.
Thanks. Replies inline below. https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:48: inline uint8_t* WriteVarIntInternal(T value, uint8_t* target) { On 2016/07/06 17:29:52, alph wrote: > Maybe add a static check that T is unsigned? Done. https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:50: *target = static_cast<uint8_t>(value | 0x80); On 2016/07/06 17:29:52, alph wrote: > nit: *target++ = Done. https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:76: size_t length, On 2016/07/06 17:29:52, alph wrote: > As it seems to be a hot path, I'd better make it a template with length > parameter. So the compiler could inline and unroll the loop. I was planning to do a final perf-optimization pass (tracked in crbug.com/624311) and address this as part of that. But both you and kraynov asked for the same, so I'll do this one here now :) https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2043913006/diff/20001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:67: EXPECT_VARINT32_EQ("\x00", 1, 0); On 2016/07/06 17:29:52, alph wrote: > nit: you can drop the length arg and infer it from the expected value. you mean using strlen? The problem is this case here where I have "\x00" where strlen would return 0 and not 1. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:28: inline uint32_t MakeTagVarInt(uint32_t field_id) { On 2016/07/07 09:46:59, petrcermak wrote: > Does it really make sense to define this for each type? It seems that defining a > generic function would be just as good: > > inline uint32_t MakeTag(unit32_t field_type, uint32_t field_id) { > return (field_id << 3) | field_type; > } > > The calls would have almost the same length and be equally readable in my > opinion: > > MakeTagLengthLimited(1234); > MakeTag(kFieldTypeLengthLimited, 1234); > > I suppose it would then make sense to name the enum so that the compiler could > check the type of the |field_type|. So I conceptually agree. In practice the only reason why I didn't make that in the first place is because I am not 100% sure that all compilers will do const propagation and inline this method as it should. I don't have bandwith to deal with compiler optimizations right now (but I plan to do that later), and if it turns out that it's not the case, optimize these is easier than rewriting the code in the caller. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:58: inline uint8_t* WriteVarIntU32(uint32_t value, uint8_t* target) { On 2016/07/07 09:46:59, petrcermak wrote: > Again, does it really make sense to define these? I think that calling the > templated function would be equally readable. Furthermore, this would simplify > the testing code (see my comment below). Hmm here the reality is that due to the TODO(kraynov) below I am not sure how the templated WriteVarInt will evolve, and I don't want to touch the caller code later too much. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:72: // Concretely this is used when writing a nested message, where the size of On 2016/07/07 09:46:59, petrcermak wrote: > nit: comma after "Concretely". I'd personally prefer "In particular, ..." Done. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:73: // nested message is not known until it is ended. In this case a fixed amount of On 2016/07/07 09:46:59, petrcermak wrote: > nit: comma after "In this case" Done. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:73: // nested message is not known until it is ended. In this case a fixed amount of On 2016/07/07 09:46:59, petrcermak wrote: > supernit: s/ended/finished/ not sure about this but... done. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:73: // nested message is not known until it is ended. In this case a fixed amount of On 2016/07/07 09:46:59, petrcermak wrote: > nit: comma after "In this case" reworded all this a bit. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils.h:75: void WriteRedundantVarIntUnsigned(uint32_t value, size_t length, uint8_t* buf) { On 2016/07/07 09:46:59, petrcermak wrote: > Should the name be "VarInt" whe it's fixed-size? Yeah I know it's an odd concept, but this is truly a "redundant varint". I don't want to call it fixed, beause that would be confusing w.r.t. fixed integer encoding in proto. > Also, I think it should be > "U32" instead of "Unsigned" to be consistent with WriteVarIntU32 et al. Agree with this. Done https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:15: bool VarIntHelperU32(const char* expected, size_t length, uint32_t value) { On 2016/07/07 09:47:00, petrcermak wrote: > I think "helper" is a bad name here because it doesn't give the reader any idea > what the function does. I think something like "CheckWriteVarInt" would be much > better. Done. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:15: bool VarIntHelperU32(const char* expected, size_t length, uint32_t value) { On 2016/07/07 09:47:00, petrcermak wrote: > If you expose the templated version of WriteVar... instead, you can template > this function as well to avoid code duplication: > > template <typename T> > bool VarIntHelper(const char* expected, size_t length, T value) { > ... > } See reason in the other file to not expose the templated version. https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:42: EXPECT_EQ(0x08u, MakeTagVarInt(1)); On 2016/07/07 09:47:00, petrcermak wrote: > Could you also check this with zero? Thanks! Good spot, but unfortunately a field_id==0 is not allowed in protobuf (will fail the import). Not worth IMHO adding a check in the inlined functions, as I'm afraid that it would cause the compiler to to something silly https://codereview.chromium.org/2043913006/diff/40001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:119: WriteRedundantVarIntUnsigned(0x332211, sizeof(buf), buf); On 2016/07/07 09:47:00, petrcermak wrote: > I suggest you also try writing the largest number that still fits into the 4 > bytes. Good point, done.
LGTM with some final comments. Thanks, Petr https://codereview.chromium.org/2043913006/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:76: // are reserved to encode the size field and backfilled at the end. nit: "... amount ... IS reserved ... " (this is a tricky one, I'm fine either way). https://codereview.chromium.org/2043913006/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:76: // are reserved to encode the size field and backfilled at the end. Is the semantics "reserved (...) and backfilled (...)" or "reserved to (encode (...) and backfilled (...))"? If it's the latter, then it should be "backfill".
https://codereview.chromium.org/2043913006/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2043913006/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:76: // are reserved to encode the size field and backfilled at the end. On 2016/07/07 13:08:17, petrcermak wrote: > Is the semantics "reserved (...) and backfilled (...)" or "reserved to (encode > (...) and backfilled (...))"? If it's the latter, then it should be "backfill". The former: reserved (to encode the size field) and (backfilled at the end) https://codereview.chromium.org/2043913006/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:76: // are reserved to encode the size field and backfilled at the end. On 2016/07/07 13:08:17, petrcermak wrote: > nit: "... amount ... IS reserved ... " (this is a tricky one, I'm fine either > way). I knew you would have pointed this out. I am not sure either, but yeah "is" sounds a bit better.
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/2043913006/#ps80001 (title: "nit + fix last test")
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-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
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/2043913006/#ps120001 (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 proto_utils.h Adds basic support for proto encoding. This will be used by the TracingV2 core classes which, for performance reasons, need to serialize protos independently of libprotobuf. More discussion in the design doc: bit.ly/TracingV2 BUG=608719 ========== to ========== tracing v2: Add proto_utils.h Adds basic support for proto encoding. This will be used by the TracingV2 core classes which, for performance reasons, need to serialize protos independently of libprotobuf. More discussion in the design doc: bit.ly/TracingV2 BUG=608719 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
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 proto_utils.h Adds basic support for proto encoding. This will be used by the TracingV2 core classes which, for performance reasons, need to serialize protos independently of libprotobuf. More discussion in the design doc: bit.ly/TracingV2 BUG=608719 ========== to ========== tracing v2: Add proto_utils.h Adds basic support for proto encoding. This will be used by the TracingV2 core classes which, for performance reasons, need to serialize protos independently of libprotobuf. More discussion in the design doc: bit.ly/TracingV2 BUG=608719 Committed: https://crrev.com/557ed0bee30e466fd37e1c1a40a8ed26654dbf96 Cr-Commit-Position: refs/heads/master@{#404671} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/557ed0bee30e466fd37e1c1a40a8ed26654dbf96 Cr-Commit-Position: refs/heads/master@{#404671} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
