Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2215153003:
Add argument support for stand-alone WebRTC tracing. (Closed)
DescriptionAdd argument support for stand-alone WebRTC tracing.
BUG=webrtc:6186
Committed: https://crrev.com/f4433ef14e8d4643009b85063089ab6c05aa298b
Cr-Commit-Position: refs/heads/master@{#13670}
Patch Set 1 #Patch Set 2 : Comments. #Patch Set 3 : Array version of delete. #Patch Set 4 : Acquire lock later. #Patch Set 5 : Don't use stringstreams. #
Total comments: 22
Patch Set 6 : Changes according to codereview comments. #Patch Set 7 : Change output_length type to size_t like it should be now. #Patch Set 8 : Try to make code compile on Windows. #Patch Set 9 : Fix warning about print_length being uninitialized. #Messages
Total messages: 29 (19 generated)
Description was changed from ========== Add argument support for stand-alone WebRTC tracing. BUG=webrtc:6186 ========== to ========== Add argument support for stand-alone WebRTC tracing. BUG=webrtc:6186 ==========
sakal@webrtc.org changed reviewers: + tommi@webrtc.org
Tommi, PTAL.
https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.cc File webrtc/base/event_tracer.cc (right): https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:104: for (int i = 0; i < num_args; i++) { ++i https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:108: arg.value.as_uint = arg_values[i]; At first I wasn't sure if this would always copy all the bits since uint can be 32bit. I see that this is not a problem after seeing that as_uint, isn't necessarily the same as a uint. So I think a better name might be in order. intptr? https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:114: strcpy(str_copy, arg.value.as_string); nit: since you call strlen above, you could use that return value and call strncpy here. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:140: args_str += ", \"args\": {"; nit: since we know that args_str will always be at least of a certain size, we could start by reserving a few bytes to handle common cases. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:143: if (!is_first_argument) { no {} (see rest of code) https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:230: unsigned long long as_uint; 'as_uint' is a bit misleading. uint can be 32 bits, but this can never be that. I think it might actually be a good thing too to have a static_assert() that the size of this union member, equals the size of the union, since you use it to assign arbitrary types to the union. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:242: std::vector<TraceArg> args; is there a way for us to avoid allocating a vector for every TraceEvent? As is, it seems like all the other members are pod. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:256: output += "\""; nit: this does a "string append" and not a "char append". The difference is that a strlen() call will be made. Instead, you can avoid that strlen call + the extra terminating \0 by doing: output += '"'; https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:266: output += "\""; same here. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:272: print_length = snprintf(&output[0], kTraceArgBufferLength, "%s", you don't need snprintf here, just use strcpy()
https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.cc File webrtc/base/event_tracer.cc (right): https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:104: for (int i = 0; i < num_args; i++) { On 2016/08/05 10:58:42, tommi-webrtc wrote: > ++i Done. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:108: arg.value.as_uint = arg_values[i]; On 2016/08/05 10:58:41, tommi-webrtc wrote: > At first I wasn't sure if this would always copy all the bits since uint can be > 32bit. I see that this is not a problem after seeing that as_uint, isn't > necessarily the same as a uint. So I think a better name might be in order. > intptr? I copied the union from webrtc/base/trace_event.h TraceValueUnion. I would like to keep it identical to that one. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:114: strcpy(str_copy, arg.value.as_string); On 2016/08/05 10:58:41, tommi-webrtc wrote: > nit: since you call strlen above, you could use that return value and call > strncpy here. Changed this to memcpy. Should be safe and even more efficient. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:140: args_str += ", \"args\": {"; On 2016/08/05 10:58:41, tommi-webrtc wrote: > nit: since we know that args_str will always be at least of a certain size, we > could start by reserving a few bytes to handle common cases. I moved args_str to the outside of the loop and made it be initialized to 256 bytes. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:143: if (!is_first_argument) { On 2016/08/05 10:58:42, tommi-webrtc wrote: > no {} (see rest of code) Done. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:230: unsigned long long as_uint; On 2016/08/05 10:58:42, tommi-webrtc wrote: > 'as_uint' is a bit misleading. uint can be 32 bits, but this can never be that. > I think it might actually be a good thing too to have a static_assert() that > the size of this union member, equals the size of the union, since you use it to > assign arbitrary types to the union. I copied this union from webrtc/base/trace_event.h TraceValueUnion. I would like to keep it identical to that one. I added the static assert. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:242: std::vector<TraceArg> args; On 2016/08/05 10:58:42, tommi-webrtc wrote: > is there a way for us to avoid allocating a vector for every TraceEvent? As is, > it seems like all the other members are pod. Discussed this offline. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:256: output += "\""; On 2016/08/05 10:58:42, tommi-webrtc wrote: > nit: this does a "string append" and not a "char append". The difference is > that a strlen() call will be made. Instead, you can avoid that strlen call + > the extra terminating \0 by doing: > > output += '"'; Done. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:266: output += "\""; On 2016/08/05 10:58:42, tommi-webrtc wrote: > same here. Done. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:272: print_length = snprintf(&output[0], kTraceArgBufferLength, "%s", On 2016/08/05 10:58:41, tommi-webrtc wrote: > you don't need snprintf here, just use strcpy() Done.
lgtm https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.cc File webrtc/base/event_tracer.cc (right): https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:108: arg.value.as_uint = arg_values[i]; On 2016/08/05 12:21:07, sakal wrote: > On 2016/08/05 10:58:41, tommi-webrtc wrote: > > At first I wasn't sure if this would always copy all the bits since uint can > be > > 32bit. I see that this is not a problem after seeing that as_uint, isn't > > necessarily the same as a uint. So I think a better name might be in order. > > intptr? > > I copied the union from webrtc/base/trace_event.h TraceValueUnion. I would like > to keep it identical to that one. Acknowledged. https://codereview.webrtc.org/2215153003/diff/80001/webrtc/base/event_tracer.... webrtc/base/event_tracer.cc:230: unsigned long long as_uint; On 2016/08/05 12:21:07, sakal wrote: > On 2016/08/05 10:58:42, tommi-webrtc wrote: > > 'as_uint' is a bit misleading. uint can be 32 bits, but this can never be > that. > > I think it might actually be a good thing too to have a static_assert() that > > the size of this union member, equals the size of the union, since you use it > to > > assign arbitrary types to the union. > > I copied this union from webrtc/base/trace_event.h TraceValueUnion. I would like > to keep it identical to that one. I added the static assert. Acknowledged. Thanks for the static_assert.
The CQ bit was checked by sakal@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/16539)
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2215153003/#ps120001 (title: "Change output_length type to size_t right it should be now.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2215153003/#ps160001 (title: "git cl format")
The CQ bit was unchecked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by sakal@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add argument support for stand-alone WebRTC tracing. BUG=webrtc:6186 ========== to ========== Add argument support for stand-alone WebRTC tracing. BUG=webrtc:6186 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add argument support for stand-alone WebRTC tracing. BUG=webrtc:6186 ========== to ========== Add argument support for stand-alone WebRTC tracing. BUG=webrtc:6186 Committed: https://crrev.com/f4433ef14e8d4643009b85063089ab6c05aa298b Cr-Commit-Position: refs/heads/master@{#13670} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f4433ef14e8d4643009b85063089ab6c05aa298b Cr-Commit-Position: refs/heads/master@{#13670} |
