Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19642005:
Make TracedValue lower overhead.
DescriptionMake TracedValue lower overhead.
Basic API support to make ConvertableToTraceFormat objects lower
overhead by writing/reading from a Pickle, instead of
base::Value objects.
BUG=trace-viewer:206
R=dsinclair, nduca
Patch Set 1 #Patch Set 2 : Fixed lint errors. #
Total comments: 62
Patch Set 3 : Keys written as raw pointer to pickle, cleaned up JSON generation, incorporated base::Value objects… #Patch Set 4 : Added pickle unit test, cleaned up traced value's unit tests. #
Total comments: 32
Patch Set 5 : #
Total comments: 4
Patch Set 6 : Validations for dictionary entries, CR comments, and additional check before writting. #
Total comments: 3
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h File base/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:18: #define STATIC_STR(value) value, sizeof(value) I don't like this (Because I'm hiding the fact that we're sending out two parameters, but makes the API a tiny bit prettier IMHO), but calling sizeof inside the function returns the pointer size, and sizeof(key[0]), returns the size of the character, however, if we get sizeof at the moment of the definition, we get the right value. I can expose methods for dictionary keys if needed, but since const char* keys/values are preferred, I didn't do that. WDYT?. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... File base/trace_event_value_unittest.cc (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... base/trace_event_value_unittest.cc:104: TEST(TracedValue, ObjectWithNestedValuesTest) { Here I'm basically testing that the support for nested structures works fine, and I think this is really necessary because the cc/ objects do that a lot through base::Value objects. Also, the TracedObject interface allows for a similar API IMO. https://codereview.chromium.org/19642005/diff/5001/base/traced_object.h File base/traced_object.h (right): https://codereview.chromium.org/19642005/diff/5001/base/traced_object.h#newco... base/traced_object.h:16: virtual ~TracedObject() {} This is the interface I mentioned, implementing this would allow us to push objects of whatever type we want into the pickle, and it would separate the core TracedValue code from classes that need to be traced through it. Is this a pattern we'd like to adopt?.
+ernstm
some initial feedback. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h File base/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:33: void BeginDictionary(const char* key_in_parent_dict, int size_of_key); any way we can make this not require key_in_parent_dict? that seems pretty awkward https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:48: void Push(const char* key, int size_of_key, const char* value, i think you can simplify this using the following assertion: if a user passes a const char* into the api, then they are sayign that the const char* will live forever. meaning we only have to store the raw pointer in the pickle. if they have something that may disappear, they would pass in a std::string, which we'd push by value into the pickle. want to try making that change? https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... File base/trace_event_value_unittest.cc (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... base/trace_event_value_unittest.cc:92: EXPECT_EQ("{\"StaticStrValue\": \"Value\", "\ one thing you could do to simplify your test is use the json reader api to turn the string a base::Value and then do your assertions that way. It'll be easier to read and easier to hack on that way
https://codereview.chromium.org/19642005/diff/5001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/19642005/diff/5001/base/base.gypi#newcode584 base/base.gypi:584: 'traced_object.h', nit: indenting https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc File base/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:10: // Public nit: remove comment. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:19: std::stack<Opcode> op_stack; I think this needs a different name, it's the current array or dictionary. I was originally thinking it contained all operations. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:32: // to prepend a comma. This is only for the array case? If so, can we move it up to just the array case statement, then let it fall through? (Put a comment // Fall-through required.) https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:35: StringAppendF(out, ", "); Isn't this, essentially AppendCommaInsideArray(out, op_stack.top(), element_count_stack.top()? https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:40: StringAppendF(out, "{"); An array in JSON is [] not {}. This would start a dictionary. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:46: op_stack.pop(); nit: Move this down one line to keep with the element stack pop. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:53: AppendCommaInsideDictionary(out, element_count_stack.top()++); Can we pull the AppendCommand methods outside of the switch statement, and just have one method? Something like the following: AppendComma(out, op_code, element_count) { if (op_code == END_DICTIONARY || op_code == END_ARRAY) return; if (element_count == 0) return; if (op_code == KEY_AS_RAW_POINTER || VALUE_AS_RAW_POINTER) { append_comma return; } if (op_code != BEGIN_ARRAY) return; append_comma } https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:167: } These are all the same and could be done with a macro inside the header: #define TRACE_PUSH(type) \ void TraceValue::Push(const char* key, int size_of_key, ##type value) { \ PushKey(key, size_of_key); \ PushValueToPickle(value); \ } TRACE_PUSH(const std::string&) TRACE_PUSH(int) TRACE_PUSH(int64) etc. #undef TRACE_PUSH https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:213: DCHECK_EQ(BEGIN_ARRAY, op_codes_.top()); Can be macro'ized same as dictionary ones. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:257: // Private nit: remove. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:276: pickle_.WriteString(value); The single param versions could be macro'ized. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:312: value.PushInto(this); I feel like TracedObject shouldn't be needed. What about making this so we can pass a TracedValue in here. We then pull the pickle out of the TracedValue and stuff it into this pickle as WriteData(pickle->data(), pickle->size()); Then, when we're outputting as trace format, we see it's a pickle and output it inline? https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:336: StringAppendF(out, "\"%s\"", out_data); This needs to handle strings with "'s in them. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:343: StringAppendF(out, "\"%s\"", out_string.c_str()); This needs to deal with strings with "'s in them. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:403: } // namespace nit: // namespace base https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h File base/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. These should probably be in base/debug with the other trace_event code. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:18: #define STATIC_STR(value) value, sizeof(value) On 2013/07/21 22:23:49, rterrazas wrote: > I don't like this (Because I'm hiding the fact that we're sending out two > parameters, but makes the API a tiny bit prettier IMHO), but calling sizeof > inside the function returns the pointer size, and sizeof(key[0]), returns the > size of the character, however, if we get sizeof at the moment of the > definition, we get the right value. > > I can expose methods for dictionary keys if needed, but since const char* > keys/values are preferred, I didn't do that. > > WDYT?. Let's drop this. It seems like the kind of thing that's going to cause issues later. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:23: // JSON format without depending on base::Value objects. Remove the JSON and just say trace format. We don't expose outside of our implementation that it's JSON. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:25: public base::NonThreadSafe { nit: indenting https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:50: void Push(const char* key, int size_of_key, const std::string& value); Why do you need size_of_key? Is this the sizeof(char) or is it the string length? If it's the string lenght, why not just do strlen(key) inside the methods? https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:74: void PushKey(const char* key, int size_of_key); Any reason why this isn't PushkeyToPickle to be consistent with the Value versions? https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:120: // in order to enforce concistency in the operations. nit: in in nit: consistency https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:121: std::stack<Opcode> op_codes_; From reading the code, this isn't all the op_codes seen, but just the BEGIN DICTIONARY and BEGIN ARRAY does? If that's correct, I think we need a better name/comment to go here. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:126: } // Closing namespace base. nit: // namespace base https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:128: #endif // Endif for BASE_TRACE_EVENT_VALUE_H_. nit: remove comment. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... File base/trace_event_value_unittest.cc (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... base/trace_event_value_unittest.cc:38: EXPECT_EQ("{{\"TestTracedObjectJson\"}}", out_str); This should be [["TestTracedObjectJson"]] https://codereview.chromium.org/19642005/diff/5001/base/traced_object.h File base/traced_object.h (right): https://codereview.chromium.org/19642005/diff/5001/base/traced_object.h#newco... base/traced_object.h:16: virtual ~TracedObject() {} On 2013/07/21 22:23:49, rterrazas wrote: > This is the interface I mentioned, implementing this would allow us to push > objects of whatever type we want into the pickle, and it would separate the core > TracedValue code from classes that need to be traced through it. > > Is this a pattern we'd like to adopt?. As above, I don't think this is needed. Is there a specific usecase where we need this over just passing TracedValue's into Push?
seems like we should be able to avoid the comma as an opcode. The data should be in the pickle not the formatting stuff. Ideally you'd look ahead to the next opcode to decide whether to issue another comma when serializing.
i think we may want to avoid the Dan's idea of returning tracedvaleus as a way
to avoid traced_object.h's stuff --- we want to replace BlahBlah::AsValue with
at most one allocation, e.g. share the pickle between all the different data
types.
i do think we want to experiment with how to pickle these types a bit more. One
promising thing I've seen is that you define traits. E.g.
template
struct TraceEventValueTraits<int> {
void PushToPickle(int value, TraceEventValuePusher pusher) {
pusher.pushRaw(INT, value);
}
}
Where Pusher is an a class that just gives the traits object access to the
Push() internal method where you can push a typename and an opcode.
And we do that for all our basic types.
Then the push() macro (or template) just instantiates
TraceEventvalueTraits<T> foo;
foo.push(this.pusher);
This causes the template system to go looking for value traits of the given type
So then when you have something like a rect, you just add to rect.h a traits for
this. Et voila, you are done.
https://codereview.chromium.org/19642005/diff/5001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/19642005/diff/5001/base/base.gypi#newcode584 base/base.gypi:584: 'traced_object.h', On 2013/07/23 14:56:29, dsinclair wrote: > nit: indenting Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc File base/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:10: // Public On 2013/07/23 14:56:29, dsinclair wrote: > nit: remove comment. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:19: std::stack<Opcode> op_stack; On 2013/07/23 14:56:29, dsinclair wrote: > I think this needs a different name, it's the current array or dictionary. I was > originally thinking it contained all operations. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:32: // to prepend a comma. This is really for the case when we are within an array, so if within the array, we have a value, which is an array/dictionary, we preppend a comma. On 2013/07/23 14:56:29, dsinclair wrote: > This is only for the array case? If so, can we move it up to just the array case > statement, then let it fall through? > > (Put a comment // Fall-through required.) https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:35: StringAppendF(out, ", "); On 2013/07/23 14:56:29, dsinclair wrote: > Isn't this, essentially AppendCommaInsideArray(out, op_stack.top(), > element_count_stack.top()? Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:40: StringAppendF(out, "{"); On 2013/07/23 14:56:29, dsinclair wrote: > An array in JSON is [] not {}. This would start a dictionary. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:46: op_stack.pop(); On 2013/07/23 14:56:29, dsinclair wrote: > nit: Move this down one line to keep with the element stack pop. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:53: AppendCommaInsideDictionary(out, element_count_stack.top()++); On 2013/07/23 14:56:29, dsinclair wrote: > Can we pull the AppendCommand methods outside of the switch statement, and just > have one method? Something like the following: > > AppendComma(out, op_code, element_count) { > if (op_code == END_DICTIONARY || op_code == END_ARRAY) > return; > > if (element_count == 0) > return; > > if (op_code == KEY_AS_RAW_POINTER || VALUE_AS_RAW_POINTER) { > append_comma > return; > } > > if (op_code != BEGIN_ARRAY) > return; > > append_comma > } Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:167: } On 2013/07/23 14:56:29, dsinclair wrote: > These are all the same and could be done with a macro inside the header: > > #define TRACE_PUSH(type) \ > void TraceValue::Push(const char* key, int size_of_key, ##type value) { \ > PushKey(key, size_of_key); \ > PushValueToPickle(value); \ > } > > TRACE_PUSH(const std::string&) > TRACE_PUSH(int) > TRACE_PUSH(int64) > etc. > #undef TRACE_PUSH Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:213: DCHECK_EQ(BEGIN_ARRAY, op_codes_.top()); On 2013/07/23 14:56:29, dsinclair wrote: > Can be macro'ized same as dictionary ones. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:257: // Private On 2013/07/23 14:56:29, dsinclair wrote: > nit: remove. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:276: pickle_.WriteString(value); On 2013/07/23 14:56:29, dsinclair wrote: > The single param versions could be macro'ized. I actually think doing this would decrease readability, for example, to macroize PushValueToPickle(int64), I'd need to pass: MACRO(int64, INT64_VALUE, WriteInt64) If you feel strongly about it, please let me know, and I'll give it a try. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:312: value.PushInto(this); On 2013/07/23 14:56:29, dsinclair wrote: > I feel like TracedObject shouldn't be needed. I was thinking that this could probably be used to avoid the problem of unnecessary instances, for example: RasterWorkerPoolTaskImpl's DataAsValue() which is later used to build a TracedValue, could change to the following structure: // RasterWorkerPoolTaskImpl would have to implement TracedObject of course. virtual void PushInto(base::debug::TracedValue* traced_value) { traced_value.BeginDictionary(); traced_value.Push("tile_id", TracedValueUtil::CreateIDRef(tile_id_)); // and CreateIDRef would convert to uintptr_t or something like that? traced_value.Push("is_tile_in_pending_tree_now_bin", is_tile_in_pending_tree_now_bin_); traced_value.Push("resolution", tile_resolution_); // TileResolution would also need to implement TracedObject. traced_value.Push("source_frame_number", source_frame_number_); traced_value.Push("layer_id", layer_id_); traced_value.EndDictionary(); } That way, we don't hammer the new operator, since we only work on data that's already instantiated. And since TracedObject does not define any behavior, this is in line with Chrome's C++ guidelines for multiple inheritance (in the cases where this would happen). WDYT?. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:336: StringAppendF(out, "\"%s\"", out_data); On 2013/07/23 14:56:29, dsinclair wrote: > This needs to handle strings with "'s in them. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:343: StringAppendF(out, "\"%s\"", out_string.c_str()); On 2013/07/23 14:56:29, dsinclair wrote: > This needs to deal with strings with "'s in them. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.cc#... base/trace_event_value.cc:403: } // namespace On 2013/07/23 14:56:29, dsinclair wrote: > nit: // namespace base Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h File base/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/07/23 14:56:29, dsinclair wrote: > These should probably be in base/debug with the other trace_event code. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:23: // JSON format without depending on base::Value objects. On 2013/07/23 14:56:29, dsinclair wrote: > Remove the JSON and just say trace format. We don't expose outside of our > implementation that it's JSON. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:25: public base::NonThreadSafe { On 2013/07/23 14:56:29, dsinclair wrote: > nit: indenting Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:33: void BeginDictionary(const char* key_in_parent_dict, int size_of_key); On 2013/07/23 04:54:04, nduca wrote: > any way we can make this not require key_in_parent_dict? that seems pretty > awkward I thought about this also, and toyed with the idea of exposing the PushKey(const char*), but it felt wrong, too easy to misuse I think. Would you prefer to do that?. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:48: void Push(const char* key, int size_of_key, const char* value, On 2013/07/23 04:54:04, nduca wrote: > i think you can simplify this using the following assertion: if a user passes a > const char* into the api, then they are sayign that the const char* will live > forever. meaning we only have to store the raw pointer in the pickle. if they > have something that may disappear, they would pass in a std::string, which we'd > push by value into the pickle. > > want to try making that change? Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:50: void Push(const char* key, int size_of_key, const std::string& value); On 2013/07/23 14:56:29, dsinclair wrote: > Why do you need size_of_key? Is this the sizeof(char) or is it the string > length? If it's the string lenght, why not just do strlen(key) inside the > methods? It was more like sizeof buffer, but Nat's hint above allowed me to get rid of this :) https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:74: void PushKey(const char* key, int size_of_key); On 2013/07/23 14:56:29, dsinclair wrote: > Any reason why this isn't PushkeyToPickle to be consistent with the Value > versions? Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:120: // in order to enforce concistency in the operations. On 2013/07/23 14:56:29, dsinclair wrote: > nit: in in > nit: consistency Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:121: std::stack<Opcode> op_codes_; On 2013/07/23 14:56:29, dsinclair wrote: > From reading the code, this isn't all the op_codes seen, but just the BEGIN > DICTIONARY and BEGIN ARRAY does? > > If that's correct, I think we need a better name/comment to go here. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:126: } // Closing namespace base. On 2013/07/23 14:56:29, dsinclair wrote: > nit: // namespace base Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value.h#n... base/trace_event_value.h:128: #endif // Endif for BASE_TRACE_EVENT_VALUE_H_. On 2013/07/23 14:56:29, dsinclair wrote: > nit: remove comment. Done. https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... File base/trace_event_value_unittest.cc (right): https://codereview.chromium.org/19642005/diff/5001/base/trace_event_value_uni... base/trace_event_value_unittest.cc:38: EXPECT_EQ("{{\"TestTracedObjectJson\"}}", out_str); On 2013/07/23 14:56:29, dsinclair wrote: > This should be [["TestTracedObjectJson"]] Done.
On 2013/07/23 18:17:55, nduca wrote:
> i think we may want to avoid the Dan's idea of returning tracedvaleus as a way
> to avoid traced_object.h's stuff --- we want to replace BlahBlah::AsValue
with
> at most one allocation, e.g. share the pickle between all the different data
> types.
>
> i do think we want to experiment with how to pickle these types a bit more.
One
> promising thing I've seen is that you define traits. E.g.
>
> template
> struct TraceEventValueTraits<int> {
> void PushToPickle(int value, TraceEventValuePusher pusher) {
> pusher.pushRaw(INT, value);
> }
> }
>
> Where Pusher is an a class that just gives the traits object access to the
> Push() internal method where you can push a typename and an opcode.
>
> And we do that for all our basic types.
>
> Then the push() macro (or template) just instantiates
> TraceEventvalueTraits<T> foo;
> foo.push(this.pusher);
>
> This causes the template system to go looking for value traits of the given
type
>
> So then when you have something like a rect, you just add to rect.h a traits
for
> this. Et voila, you are done.
Haven't had a chance to experiment with this idea, but I also put some
additional comments describing how I envisioned TracedObject doing something
like this, any opinions about that?. If that's a pattern we'd want to follow,
I'd also like to rename that interface to PushableToTracedValueInterface or
something along those lines. Anyhow, wdyt?.
https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... File base/debug/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:47: #define TRACE_PUSH(type) \ Linter think's we're using a C-style cast here, can we ignore?.
https://codereview.chromium.org/19642005/diff/21001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/19642005/diff/21001/base/base.gypi#newcode155 base/base.gypi:155: 'debug/traced_object.h', Can we rename this to debug/trace_event_object.h ? Or something, would let it match the OWNERS filter that is setup. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... File base/debug/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:100: NOTREACHED() << "Don't know what to read, opcode=" << opcode; nit: s/read/write https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:116: begin_opcodes_.push(BEGIN_DICTIONARY); DCHECK_NEQ(BEGIN_DICTIONARY, begin_opcodes_.top()) Should we verify we aren't currently in a dictionary before doing this? https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:136: begin_opcodes_.push(BEGIN_ARRAY); DCHECK_NEQ(BEGIN_DICTIONARY, begin_opcodes_.top()? https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:148: // blocks, it's the up to the publicly exposed caller to check the state. nit: s/publicly/publically https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:151: DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top()); The comment above says the caller checks the state, if so, why do we check the state here? https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:211: StringAppendF(out, "\"%s\":", key); Is it possible for key to have a " in it? If so, it needs to be escaped. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:218: StringAppendF(out, "\"%s\":", out_string.c_str()); out_string.c_str() needs to escape any " characters. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:270: false)); nit: fits above. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... File base/debug/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:5: #ifndef BASE_TRACE_EVENT_VALUE_H_ This should be BASE_DEBUG_TRACE_EVENT_VALUE_H_. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:32: void BeginDictionary(const char* key_in_parent_dict); What about calling the two begins that take a key: void PushDictionary(const char* key); void PushArray(const char* key); Keeps the name and meaning close to what we have below with the various Push methods. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:48: void Push(const char* key, type value) { \ nit: indent 2 https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:49: PushKeyToPickle(key); \ Should this have a: DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top()) to verify we have a dictionary open? https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:65: void Push(type value) { \ nit: indent 2 https://codereview.chromium.org/19642005/diff/21001/base/debug/traced_object.h File base/debug/traced_object.h (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/traced_object.... base/debug/traced_object.h:5: #ifndef BASE_TRACED_OBJECT_H_ BASE_DEBUG_TRACED_OBJECT_H_
https://codereview.chromium.org/19642005/diff/21001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/19642005/diff/21001/base/base.gypi#newcode155 base/base.gypi:155: 'debug/traced_object.h', On 2013/08/07 15:40:20, dsinclair wrote: > Can we rename this to debug/trace_event_object.h ? Or something, would let it > match the OWNERS filter that is setup. Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... File base/debug/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:100: NOTREACHED() << "Don't know what to read, opcode=" << opcode; On 2013/08/07 15:40:20, dsinclair wrote: > nit: s/read/write Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:116: begin_opcodes_.push(BEGIN_DICTIONARY); On 2013/08/07 15:40:20, dsinclair wrote: > DCHECK_NEQ(BEGIN_DICTIONARY, begin_opcodes_.top()) > > Should we verify we aren't currently in a dictionary before doing this? This would harm implementations of TracedObject, for example, if we had this check, and an implementation of TracedObject along these lines: PushInto(traced_obj) { traced_obj.BeginDictionary(); traced_obj.Push("Foo", 12); traced_obj.EndDictionary(); } We'd have a problem. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:136: begin_opcodes_.push(BEGIN_ARRAY); On 2013/08/07 15:40:20, dsinclair wrote: > DCHECK_NEQ(BEGIN_DICTIONARY, begin_opcodes_.top()? Similarly to the dictionary case above, except the problem would arise at: PushInto(traced_obj) { traced_obj.BeginArray(); ... } https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:148: // blocks, it's the up to the publicly exposed caller to check the state. On 2013/08/07 15:40:20, dsinclair wrote: > nit: s/publicly/publically Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:151: DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top()); On 2013/08/07 15:40:20, dsinclair wrote: > The comment above says the caller checks the state, if so, why do we check the > state here? Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:211: StringAppendF(out, "\"%s\":", key); On 2013/08/07 15:40:20, dsinclair wrote: > Is it possible for key to have a " in it? If so, it needs to be escaped. Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:218: StringAppendF(out, "\"%s\":", out_string.c_str()); On 2013/08/07 15:40:20, dsinclair wrote: > out_string.c_str() needs to escape any " characters. Done, actually, I removed this one, wasn't being used, not sure why I added it in the first place, we're heavily preferring statically allocated strings. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:270: false)); On 2013/08/07 15:40:20, dsinclair wrote: > nit: fits above. Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... File base/debug/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:5: #ifndef BASE_TRACE_EVENT_VALUE_H_ On 2013/08/07 15:40:20, dsinclair wrote: > This should be BASE_DEBUG_TRACE_EVENT_VALUE_H_. Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:32: void BeginDictionary(const char* key_in_parent_dict); On 2013/08/07 15:40:20, dsinclair wrote: > What about calling the two begins that take a key: > > void PushDictionary(const char* key); > void PushArray(const char* key); > > Keeps the name and meaning close to what we have below with the various Push > methods. Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:48: void Push(const char* key, type value) { \ On 2013/08/07 15:40:20, dsinclair wrote: > nit: indent 2 Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:49: PushKeyToPickle(key); \ On 2013/08/07 15:40:20, dsinclair wrote: > Should this have a: DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top()) to verify > we have a dictionary open? Yes, I had it in PushKeyToPickle() but I agree, makes more sense to put this here. Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.h:65: void Push(type value) { \ On 2013/08/07 15:40:20, dsinclair wrote: > nit: indent 2 Done. https://codereview.chromium.org/19642005/diff/21001/base/debug/traced_object.h File base/debug/traced_object.h (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/traced_object.... base/debug/traced_object.h:5: #ifndef BASE_TRACED_OBJECT_H_ On 2013/08/07 15:40:20, dsinclair wrote: > BASE_DEBUG_TRACED_OBJECT_H_ Done.
https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... File base/debug/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/21001/base/debug/trace_event_va... base/debug/trace_event_value.cc:116: begin_opcodes_.push(BEGIN_DICTIONARY); On 2013/08/08 06:21:30, rterrazas wrote: > On 2013/08/07 15:40:20, dsinclair wrote: > > DCHECK_NEQ(BEGIN_DICTIONARY, begin_opcodes_.top()) > > > > Should we verify we aren't currently in a dictionary before doing this? > > This would harm implementations of TracedObject, for example, if we had this > check, and an implementation of TracedObject along these lines: > > PushInto(traced_obj) { > traced_obj.BeginDictionary(); > traced_obj.Push("Foo", 12); > traced_obj.EndDictionary(); > } > > We'd have a problem. I think you're going to need a third type on your begin list. I should not be able to do: BeginDictionary() BeginDictionary() EndDictionary() EndDictionary() Without it raising some kind of DCHECK. In order to handle the PushInto case I think you should probably push the PUSH_INTO onto your begin_opcodes_ list. When you do a PushInto you don't have anything started, you're essentially starting fresh. We also don't want a user to accidentally do: BeginDictionary() PushInto() { EndDictionary) } EndDictionary() Which, currently is possible. https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... File base/debug/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... base/debug/trace_event_value.cc:113: // Named dictionaries are properties of dictionaries, the name is the key. The DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top()); seems to have been lost. https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... File base/debug/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... base/debug/trace_event_value.h:35: void PushDictionary(const char* key_in_parent_dict); nit: Move the two Push* methods down below the EndArray so they're grouped with the other Push methods.
> I think you're going to need a third type on your begin list. I should not be > able to do: > > BeginDictionary() > BeginDictionary() > EndDictionary() > EndDictionary() Yes, you are right, I see what you meant with the previous comment, sorry about the confusion!. I'll add that :)
https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... File base/debug/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... base/debug/trace_event_value.cc:113: // Named dictionaries are properties of dictionaries, the name is the key. On 2013/08/08 14:07:28, dsinclair wrote: > The DCHECK_EQ(BEGIN_DICTIONARY, begin_opcodes_.top()); seems to have been lost. Done. https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... File base/debug/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/37001/base/debug/trace_event_va... base/debug/trace_event_value.h:35: void PushDictionary(const char* key_in_parent_dict); On 2013/08/08 14:07:28, dsinclair wrote: > nit: Move the two Push* methods down below the EndArray so they're grouped with > the other Push methods. Done. https://codereview.chromium.org/19642005/diff/50001/base/debug/trace_event_va... File base/debug/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/50001/base/debug/trace_event_va... base/debug/trace_event_value.cc:20: DCHECK(begin_opcodes_.empty()) << "This TracedValue is not ready to be \ Also added this check, as I think it can be helpful to know if we missed an End*() call.
This is looking good to me. Did you get a chance to try out nduca's Trait idea? https://codereview.chromium.org/19642005/diff/50001/base/debug/trace_event_va... File base/debug/trace_event_value.cc (right): https://codereview.chromium.org/19642005/diff/50001/base/debug/trace_event_va... base/debug/trace_event_value.cc:290: if (!begin_opcodes_.empty()) if (begin_opcodes_.empty()) return; DCHECK_NE(BEGIN_DICTIONARY, begin_opcodes_.top()); https://codereview.chromium.org/19642005/diff/50001/base/debug/trace_event_va... File base/debug/trace_event_value.h (right): https://codereview.chromium.org/19642005/diff/50001/base/debug/trace_event_va... base/debug/trace_event_value.h:86: // value write away, but defer to some other implementation how to write the s/write/right
On 2013/08/12 13:34:40, dsinclair wrote: > This is looking good to me. Did you get a chance to try out nduca's Trait idea? > Nice!, I had a chance to read about that a bit yesterday, thought I still haven't experimented with it. This and allowing primitive values as sole values of TracedObjects (or whatever concept replaces it) is next on my list for this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
