Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2303343002:
tracing v2: building blocks for reading-back trace protobufs (Closed)
|
Created:
4 years, 3 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 3 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, alph Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing v2: building blocks for reading-back trace protobufs
This CL introduces the core building blocks to read-back the
proto-encoded trace buffer. This will be used by next CLs
at trace finalization time to convert protobuf -> v1 JSON.
BUG=643674
Committed: https://crrev.com/7a4e4ee2ed6c4b31692a02f6ee7951d356cc74ef
Cr-Commit-Position: refs/heads/master@{#417591}
Patch Set 1 #
Total comments: 4
Patch Set 2 : add tests + rebase #Patch Set 3 : . #
Total comments: 41
Patch Set 4 : oysteine + petrcermak review #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 28 (17 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: + kraynov@chromium.org, oysteine@chromium.org
https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.cc (right): https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.cc:26: } while (*pos++ & 0x80); Termination condition looks too tricky https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:102: const uint8_t* end, You use STL-style when defined ContinuousMemoryRange where end means one byte past the last byte.
https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.cc (right): https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.cc:26: } while (*pos++ & 0x80); On 2016/09/02 16:06:49, kraynov wrote: > Termination condition looks too tricky uh, but the only other option I see is doing: while(True) { ... .. if (*pos & 0x80) break; pos++ } pos++; return pos; which looks duplicate and odd. Not sure if there is a better way to re-write this entire loop. Will think to that on Monday. https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... components/tracing/core/proto_utils.h:102: const uint8_t* end, On 2016/09/02 16:06:49, kraynov wrote: > You use STL-style when defined ContinuousMemoryRange where end means one byte > past the last byte. The same thing is here (unless I screwed something, which is not WAI). I should probably add a comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
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 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...
On 2016/09/02 16:54:59, Primiano Tucci wrote: > https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... > File components/tracing/core/proto_utils.cc (right): > > https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... > components/tracing/core/proto_utils.cc:26: } while (*pos++ & 0x80); > On 2016/09/02 16:06:49, kraynov wrote: > > Termination condition looks too tricky > > uh, but the only other option I see is doing: > > while(True) { > ... > .. > if (*pos & 0x80) > break; > pos++ > } > pos++; > return pos; > > which looks duplicate and odd. > Not sure if there is a better way to re-write this entire loop. Will think to > that on Monday. > > https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... > File components/tracing/core/proto_utils.h (right): > > https://codereview.chromium.org/2303343002/diff/1/components/tracing/core/pro... > components/tracing/core/proto_utils.h:102: const uint8_t* end, > On 2016/09/02 16:06:49, kraynov wrote: > > You use STL-style when defined ContinuousMemoryRange where end means one byte > > past the last byte. > > The same thing is here (unless I screwed something, which is not WAI). I should > probably add a comment. Yes, I see, you use STL-style. Just add a comment. LGTM then :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
petrcermak@chromium.org changed reviewers: + petrcermak@chromium.org
LGTM with a couple of (non-binding) comments. BTW, I don't understand why you say "reading-back" instead of "reading back". It seems like it should be just a regular verb. Thanks, Petr https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.cc (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:5: #include "components/tracing/core/proto_utils.h" Is there a particular reason for this import ordering? If not, I would expect "components" to be after "base". https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:28: } while (*pos++ & 0x80); for readability, maybe add parentheses: *(pos++) https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:38: CHECK_LT(pos, end); I know that you can use CHECK_LT here, but maybe use CHECK_PTR_LE for consistency? https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:43: raw_field_id >>= 3; It took me a while to understand what this is for. Could you add a comment explaining the fact that you shift by 3 because the 3 LSB are in *field_type? https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:67: if (*field_type == kFieldTypeLengthDelimited) { I think that readability might be worth the one duplicated line here: case kFieldTypeVarInt: pos = ParseVarInt(pos, end, field_intvalue); break; case kFieldTypeLengthDelimited: ... https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:106: // Parses a protobuf field, returning its id, type and value. "returning" is misleading here. The value is not returned, but assigned to the output parameters. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:107: // Returns a pointer to the next unconsumed byte (|start| < retval <= end), that grammar nit: either there shouldn't be a comma or s/that/which/ (see http://www.oxforddictionaries.com/us/words/clauses-american#non-restrictive_r...). https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:109: // In the case of a kFieldTypeLengthDelimited field |field_intvalue| will store Here, on the other hand, should be a comma after "field". Otherwise, the sentence is confusing. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:110: // the length of the payload (either a string or a nested message). In this case nit: there should be a comma after "In this case" https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:20: size_t encoded_size; Do you really need the encoded size? Isn't it essentially just strlen(encoded)? https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:52: size_t encoded_size; ditto https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:118: for (size_t i = 0; i < arraysize(kVarIntExpectations); ++i) { I would put this into a separate test (it seems independent). https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:155: uint64_t value = 0; perhaps use some value that you know can't be output (e.g. 123456789) to check that zero is read correctly? https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:165: // Test field parsing. These seem to be two separate tests, so I'd put them into separate TEST blocks. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:169: uint32_t field_id = 0; Maybe use some values that you know can't be "returned" by the methods (e.g. 9999, 9999) to check that zeros are handled properly?
lgtm w/ nitty comments https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.cc (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:39: *field_type = static_cast<FieldType>(*pos & 7); Hmm, the code embeds a lot of magic numbers here. Could we move some of these to named constants, to make things a little more readable? https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:43: raw_field_id >>= 3; On 2016/09/08 16:13:25, petrcermak wrote: > It took me a while to understand what this is for. Could you add a comment > explaining the fact that you shift by 3 because the 3 LSB are in *field_type? Maybe use the same named constant in both cases to make it extra clear. More math but clearer perhaps? https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:51: memcpy(field_intvalue, pos, 8); sizeof(uint64_t) for clarity? https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:53: pos += 8; Ditto https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:57: CHECK_PTR_LE(pos + 4, end); Ditto for uint32_t
https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:21: enum FieldType : uint32_t { Why is this an uint32_t (and hence everywhere ParseField is called), if it's read as an & 7?
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.cc (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:5: #include "components/tracing/core/proto_utils.h" On 2016/09/08 16:13:25, petrcermak wrote: > Is there a particular reason for this import ordering? If not, I would expect > "components" to be after "base". yes: the first include is the corresponding .h file then system includes then chrome includes see https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:28: } while (*pos++ & 0x80); On 2016/09/08 16:13:25, petrcermak wrote: > for readability, maybe add parentheses: *(pos++) There are 260 cases of *xx++ [1] vs 56 of *(xx++) [2] majority wins :) [1] https://cs.chromium.org/search/?q=%5C*%5Cw%2B%5C%2B%5C%2B+-f:third_party&sq=p... [2] https://cs.chromium.org/search/?q=%5C*%5C(%5Cw%2B%5C%2B%5C%2B%5C)+-f:third_pa... https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:38: CHECK_LT(pos, end); On 2016/09/08 16:13:25, petrcermak wrote: > I know that you can use CHECK_LT here, but maybe use CHECK_PTR_LE for > consistency? Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:39: *field_type = static_cast<FieldType>(*pos & 7); On 2016/09/08 21:26:04, oystein wrote: > Hmm, the code embeds a lot of magic numbers here. Could we move some of these to > named constants, to make things a little more readable? ok done and added some comments https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:43: raw_field_id >>= 3; On 2016/09/08 21:26:04, oystein wrote: > On 2016/09/08 16:13:25, petrcermak wrote: > > It took me a while to understand what this is for. Could you add a comment > > explaining the fact that you shift by 3 because the 3 LSB are in *field_type? > > Maybe use the same named constant in both cases to make it extra clear. More > math but clearer perhaps? ok ok used more names. Just Stockholm syndrome after a month workin on protos. To me that "3" was perfectly clear and unambiguous. I see that might not be the case in general though :P https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:51: memcpy(field_intvalue, pos, 8); On 2016/09/08 21:26:04, oystein wrote: > sizeof(uint64_t) for clarity? Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:53: pos += 8; On 2016/09/08 21:26:04, oystein wrote: > Ditto Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:57: CHECK_PTR_LE(pos + 4, end); On 2016/09/08 21:26:04, oystein wrote: > Ditto for uint32_t Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.cc:67: if (*field_type == kFieldTypeLengthDelimited) { On 2016/09/08 16:13:25, petrcermak wrote: > I think that readability might be worth the one duplicated line here: > > case kFieldTypeVarInt: > pos = ParseVarInt(pos, end, field_intvalue); > break; > > case kFieldTypeLengthDelimited: > ... right makes sense. after various edits didn't ralize this is saving just one line. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils.h (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:21: enum FieldType : uint32_t { On 2016/09/08 22:16:07, oystein wrote: > Why is this an uint32_t (and hence everywhere ParseField is called), if it's > read as an & 7? So, I was about to switch this to uint8_t after your comment but.... I then talked with kraynov@ he convinced me that, in the cases where this is used to form back a uint32_t , if this is a variable it will end up requiring an extra register expansion (From 8 to 32 bi). concretely shouldn't make any big difference with the code as it is today, because we always use constant, so kSometing << 3 ends up with the same instructions because the compiler knows the value of kSomehing so that becomes an operation between register and immediate. However, if you have FieldType field_type = getFieldTypeFromSomewhere() and then do uint32_t = uint32_var << 3 || field_type having FieldType being a uint32 is slightly more efficient (I tested the dasm) In practice shouldn't be a big deal anyways. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:106: // Parses a protobuf field, returning its id, type and value. On 2016/09/08 16:13:25, petrcermak wrote: > "returning" is misleading here. The value is not returned, but assigned to the > output parameters. Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:107: // Returns a pointer to the next unconsumed byte (|start| < retval <= end), that On 2016/09/08 16:13:25, petrcermak wrote: > grammar nit: either there shouldn't be a comma or s/that/which/ (see > http://www.oxforddictionaries.com/us/words/clauses-american#non-restrictive_r...). Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:109: // In the case of a kFieldTypeLengthDelimited field |field_intvalue| will store On 2016/09/08 16:13:25, petrcermak wrote: > Here, on the other hand, should be a comma after "field". Otherwise, the > sentence is confusing. Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils.h:110: // the length of the payload (either a string or a nested message). In this case On 2016/09/08 16:13:25, petrcermak wrote: > nit: there should be a comma after "In this case" Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... File components/tracing/core/proto_utils_unittest.cc (right): https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:20: size_t encoded_size; On 2016/09/08 16:13:25, petrcermak wrote: > Do you really need the encoded size? Isn't it essentially just strlen(encoded)? no, because of the \x00 unfortunately https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:52: size_t encoded_size; On 2016/09/08 16:13:25, petrcermak wrote: > ditto ditto :P https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:118: for (size_t i = 0; i < arraysize(kVarIntExpectations); ++i) { On 2016/09/08 16:13:25, petrcermak wrote: > I would put this into a separate test (it seems independent). Ok restructured the tests. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:155: uint64_t value = 0; On 2016/09/08 16:13:25, petrcermak wrote: > perhaps use some value that you know can't be output (e.g. 123456789) to check > that zero is read correctly? Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:165: // Test field parsing. On 2016/09/08 16:13:25, petrcermak wrote: > These seem to be two separate tests, so I'd put them into separate TEST blocks. Done. https://codereview.chromium.org/2303343002/diff/60001/components/tracing/core... components/tracing/core/proto_utils_unittest.cc:169: uint32_t field_id = 0; On 2016/09/08 16:13:25, petrcermak wrote: > Maybe use some values that you know can't be "returned" by the methods (e.g. > 9999, 9999) to check that zeros are handled properly? Done.
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, kraynov@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2303343002/#ps100001 (title: "oysteine + petrcermak review")
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:100001)
Message was sent while issue was closed.
Description was changed from ========== tracing v2: building blocks for reading-back trace protobufs This CL introduces the core building blocks to read-back the proto-encoded trace buffer. This will be used by next CLs at trace finalization time to convert protobuf -> v1 JSON. BUG=643674 ========== to ========== tracing v2: building blocks for reading-back trace protobufs This CL introduces the core building blocks to read-back the proto-encoded trace buffer. This will be used by next CLs at trace finalization time to convert protobuf -> v1 JSON. BUG=643674 Committed: https://crrev.com/7a4e4ee2ed6c4b31692a02f6ee7951d356cc74ef Cr-Commit-Position: refs/heads/master@{#417591} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7a4e4ee2ed6c4b31692a02f6ee7951d356cc74ef Cr-Commit-Position: refs/heads/master@{#417591} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
