Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2228563002:
Tracing V2: Proto message and utils improvements. (Closed)
DescriptionTracing V2: Proto message and utils improvements.
Appenders has been moved to header and polished for uniform style.
Proto utils are extended to support ZigZag encoding.
BUG=608719
Committed: https://crrev.com/034f4638ca40cd3b77d55e72f773af24961f8591
Cr-Commit-Position: refs/heads/master@{#410852}
Patch Set 1 #
Total comments: 8
Patch Set 2 : minors #
Total comments: 20
Patch Set 3 : test zig-zag #Patch Set 4 : style #
Total comments: 6
Messages
Total messages: 33 (20 generated)
The CQ bit was checked by kraynov@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 kraynov@chromium.org
kraynov@chromium.org changed reviewers: + alph@chromium.org, petrcermak@chromium.org, primiano@chromium.org
PTAL
lgtm https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:90: buf[i] = static_cast<uint8_t>((value & 0x7F) | msb); nit: & 0x7F is redundant https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils_unittest.cc:21: if (memcmp(expected, buf, length)) nit: return memcmp() == 0 https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils_unittest.cc:62: EXPECT_EQ(4294967293u, ZigZagEncode(-2147483647)); Check -2147483648 as well? https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_zero_message.h:67: void AppendVarInt(uint32_t field_id, T value) { Maybe DCHECK the value is positive?
The CQ bit was checked by kraynov@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...)
The CQ bit was checked by kraynov@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...
LGTM with a couple of style comments. Thanks, Petr https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:18: // TODO(kraynov) Change namespace to tracing::proto::internal. nit: missing colon after closing parenthesis "):" https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:19: // This stuff is required in headers and it's too low-level to be exposed. nit: no need for "stuff". You can just say "This is required" https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:34: // Varint maximum encoded size == (sizeof(T) * 8 + 6) / 7. Shouldn't this be removed now? What does the comment refer to? https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:36: // Largest possible singilar value is 64-bit varint (10 bytes at most). s/singilar/singular/. What is a "singular value"? Don't you mean a "single value"? https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:84: // A fixed kMessageLengthFieldSize amount of bytes is reserved to encode I think you should just say "kMessageLengthFieldSize bytes are reserved to encode...". I'm not sure what the style guide says, but maybe it should be "|kMessageLengthFieldSize| bytes are reserved ..." (please check). https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:87: DCHECK_LE(value, kMaxMessageLength) << "Message is too large"; nit: s/large/long/ https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:31: // According to C++ standard right shift of negative value has nit: add comma after "standard" https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:34: FAIL() << "Platform has unsupported arithmetic"; maybe be more specific (e.g. "shifting arithmetic" or "negative number shifting arithmetic")? https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_zero_message.h:75: // WriteVarInt treat signed values in two's complement form. nit: s/treat/treats/. Also I think "writes" or "encodes" would be more appropriate ("treats" is kind of ambiguous and "treat X in form Y" is a little mystifying). https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_zero_message.h:102: // TODO(kraynov) AppendTinyInt. nit: missing colon "):". Also, please include the action in the TODO. Should it be "Add AppendTinyInt" or "Implement AppendTinyInt"?
The CQ bit was unchecked by kraynov@chromium.org
The CQ bit was checked by kraynov@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 kraynov@chromium.org
The CQ bit was checked by kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2228563002/#ps60001 (title: "style")
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...)
Sorry, forgot to reply for comments https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:90: buf[i] = static_cast<uint8_t>((value & 0x7F) | msb); On 2016/08/08 19:59:50, alph wrote: > nit: & 0x7F is redundant Done. https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils_unittest.cc:21: if (memcmp(expected, buf, length)) On 2016/08/08 19:59:50, alph wrote: > nit: return memcmp() == 0 Done. https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils_unittest.cc:62: EXPECT_EQ(4294967293u, ZigZagEncode(-2147483647)); On 2016/08/08 19:59:50, alph wrote: > Check -2147483648 as well? Done. https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2228563002/diff/1/components/tracing/core/pro... components/tracing/core/proto_zero_message.h:67: void AppendVarInt(uint32_t field_id, T value) { On 2016/08/08 19:59:50, alph wrote: > Maybe DCHECK the value is positive? It's suitable for signed value as well. Added a comment. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:18: // TODO(kraynov) Change namespace to tracing::proto::internal. On 2016/08/09 09:55:53, petrcermak wrote: > nit: missing colon after closing parenthesis "):" Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:19: // This stuff is required in headers and it's too low-level to be exposed. On 2016/08/09 09:55:53, petrcermak wrote: > nit: no need for "stuff". You can just say "This is required" Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:34: // Varint maximum encoded size == (sizeof(T) * 8 + 6) / 7. On 2016/08/09 09:55:53, petrcermak wrote: > Shouldn't this be removed now? What does the comment refer to? Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:36: // Largest possible singilar value is 64-bit varint (10 bytes at most). On 2016/08/09 09:55:53, petrcermak wrote: > s/singilar/singular/. What is a "singular value"? Don't you mean a "single > value"? Done. (replaced with "Simple"). https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:84: // A fixed kMessageLengthFieldSize amount of bytes is reserved to encode On 2016/08/09 09:55:53, petrcermak wrote: > I think you should just say "kMessageLengthFieldSize bytes are reserved to > encode...". I'm not sure what the style guide says, but maybe it should be > "|kMessageLengthFieldSize| bytes are reserved ..." (please check). Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils.h:87: DCHECK_LE(value, kMaxMessageLength) << "Message is too large"; On 2016/08/09 09:55:53, petrcermak wrote: > nit: s/large/long/ Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:31: // According to C++ standard right shift of negative value has On 2016/08/09 09:55:53, petrcermak wrote: > nit: add comma after "standard" Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:34: FAIL() << "Platform has unsupported arithmetic"; On 2016/08/09 09:55:53, petrcermak wrote: > maybe be more specific (e.g. "shifting arithmetic" or "negative number shifting > arithmetic")? Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... File components/tracing/core/proto_zero_message.h (right): https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_zero_message.h:75: // WriteVarInt treat signed values in two's complement form. On 2016/08/09 09:55:53, petrcermak wrote: > nit: s/treat/treats/. Also I think "writes" or "encodes" would be more > appropriate ("treats" is kind of ambiguous and "treat X in form Y" is a little > mystifying). Done. https://codereview.chromium.org/2228563002/diff/20001/components/tracing/core... components/tracing/core/proto_zero_message.h:102: // TODO(kraynov) AppendTinyInt. On 2016/08/09 09:55:53, petrcermak wrote: > nit: missing colon "):". Also, please include the action in the TODO. Should it > be "Add AppendTinyInt" or "Implement AppendTinyInt"? Done.
kraynov@chromium.org changed reviewers: + oysteine@chromium.org
+oysteine Oystein, could you please take a look?
lgtm
The CQ bit was checked by kraynov@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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Tracing V2: Proto message and utils improvements. Appenders has been moved to header and polished for uniform style. Proto utils are extended to support ZigZag encoding. BUG=608719 ========== to ========== Tracing V2: Proto message and utils improvements. Appenders has been moved to header and polished for uniform style. Proto utils are extended to support ZigZag encoding. BUG=608719 Committed: https://crrev.com/034f4638ca40cd3b77d55e72f773af24961f8591 Cr-Commit-Position: refs/heads/master@{#410852} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/034f4638ca40cd3b77d55e72f773af24961f8591 Cr-Commit-Position: refs/heads/master@{#410852}
Message was sent while issue was closed.
Post-the-fact LGTM w/ minor comments. No action needed, will fix them in my next CL. https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:19: // This is required in headers and it's too low-level to be exposed. This was the entire point of "proto" namespace. Not expose internal details that deal with proto. https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:79: // used to backfill fixed-size reservations for the length of field using a no, the meaning here was really "the lenght field", that is the field that tells the length. https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:85: inline void WriteRedundantLength(uint32_t value, uint8_t* buf) { WriteRedundantLength is IMHO a bad name. What is a redundant length? The thing that is redundant here is the varint encoding. You just removed the most important part of the name, that is "VarInt".
Message was sent while issue was closed.
https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:19: // This is required in headers and it's too low-level to be exposed. On 2016/08/24 08:33:53, Primiano Tucci wrote: > This was the entire point of "proto" namespace. Not expose internal details that > deal with proto. Acknowledged. https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:79: // used to backfill fixed-size reservations for the length of field using a On 2016/08/24 08:33:53, Primiano Tucci wrote: > no, the meaning here was really "the lenght field", that is the field that tells > the length. Let's be consistent with protobuf terminology: - Field means some data corresponding to a number in message descriptor - Tag means field type and number tuple - "Length" is a part of length-delimited field https://codereview.chromium.org/2228563002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:85: inline void WriteRedundantLength(uint32_t value, uint8_t* buf) { On 2016/08/24 08:33:53, Primiano Tucci wrote: > WriteRedundantLength is IMHO a bad name. > What is a redundant length? > The thing that is redundant here is the varint encoding. You just removed the > most important part of the name, that is "VarInt". The only purpose of this method is overwriting placeholder for length. It takes exactly |kMessageLengthFieldSize| (not the best name for constant because of collision with "field" meaning in protobuf, but it's better than something non-obvious like |kMessageLengthSize| or verbose |kMessageLengthPlaceholderSize|) number of bytes. It basically writes the length value for field, VarInt is not a point in this abstraction. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
