Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
357703002:
Introduce builders for tracing event arguments (Closed)
|
Created:
6 years, 6 months ago by yurys Modified:
6 years, 5 months ago CC:
blink-reviews, eae+blinkwatch, apavlov+blink_chromium.org, aandrey+blink_chromium.org, rwlbuis, caseq+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, jchaffraix+rendering, devtools-reviews_chromium.org, pdr., rune+blink, loislo+blink_chromium.org, zoltan1, sof, lushnikov+blink_chromium.org, eustas+blink_chromium.org, paulirish+reviews_chromium.org, blink-reviews-rendering, leviw+renderwatch, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionIntroduce builders for tracing event arguments
The builders provide interface for constructing JSON-like structure. Current implementation uses JSONValue as backing store but it may be replaced later with something more efficient without requiring to update client code.
Current implementation ensures that we create isolated copies of all strings as they may be destroyed on a different thread.
JSCallStack in InspectorTraceEvents.cpp now serializes call stack to string in the constructor instead of doing it later during tracing events serialization. Alternatively we could clone ScriptCallStack so that it contained isolated copies of all strings and do the serialization later but that seemed equally costly.
BUG=375242, 361045
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177086
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 14
Patch Set 3 : #Patch Set 4 : Rebase #Patch Set 5 : Removed PLATFORM_EXPORT from template classes #
Messages
Total messages: 48 (0 generated)
This seems like a lot of duplication over what the JSON types already gives us. We will always have to output a JSON format as that's what everything expects the trace file to be. Are there plans in the works to store this as a different type then JSON internally? Is this just on the Blink side, or are you planning on making these changes on the Chromium side as well?
On 2014/06/25 13:49:57, dsinclair wrote: > This seems like a lot of duplication over what the JSON types already gives us. > We will always have to output a JSON format as that's what everything expects > the trace file to be. > > Are there plans in the works to store this as a different type then JSON > internally? It depends on whether we can come up with a fast enough implementation that writes directly into a JSON string. If not we can use some binary format and do expensive escaping later. > Is this just on the Blink side, or are you planning on making these > changes on the Chromium side as well? Nat requested for similar builder in Chrome (see https://codereview.chromium.org/263653002/#msg9) so that we could switch on a more efficient implementation. The plan is to have similar facilities on the Chrome side too.
lgtm The only concern is if these templates get broader usage it might start affecting the binary size. https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.cpp File Source/platform/TracedValue.cpp (right): https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.... Source/platform/TracedValue.cpp:30: String threadSafeCopy(const String& string) static? https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.... Source/platform/TracedValue.cpp:97: ASSERT(!m_stack.size() > 1); something is wrong here https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.... Source/platform/TracedValue.cpp:138: ASSERT(!m_stack.size() > 1); something is wrong here
https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.cpp File Source/platform/TracedValue.cpp (right): https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.... Source/platform/TracedValue.cpp:30: String threadSafeCopy(const String& string) On 2014/06/25 14:07:25, alph wrote: > static? It is in anonymous namespace. https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.... Source/platform/TracedValue.cpp:97: ASSERT(!m_stack.size() > 1); On 2014/06/25 14:07:25, alph wrote: > something is wrong here Already fixed. https://codereview.chromium.org/357703002/diff/1/Source/platform/TracedValue.... Source/platform/TracedValue.cpp:138: ASSERT(!m_stack.size() > 1); On 2014/06/25 14:07:26, alph wrote: > something is wrong here Also fixed.
lgtm. Thanks for the explanation. https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:65: nit: extra blank line. https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:74: static void createQuad(TracedValue* value, const char* name, const FloatQuad& quad) Can the TraceValue* be passed by reference so we know it can never be null by accident?
https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:38: if (!m_serialized.isSafeToSendToAnotherThread()) Do we expect to ever hit this? https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... File Source/platform/TracedValue.cpp (right): https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.cpp:39: bool isJSONObject(JSONValue* value) I think explicitly comparing against value->type() is more readable and can be conveniently inlined. https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... File Source/platform/TracedValue.h (right): https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.h:25: void setInteger(const char* name, int value); Can we put these methods on TracedDictionary... https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.h:33: void pushInteger(int); ... and these to TracedArray instead? https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.h:139: class PLATFORM_EXPORT TracedValue : public TracedValueBase { Can we inherit from TracedDictionary instead?
On 2014/06/25 14:07:26, alph wrote: > lgtm > > The only concern is if these templates get broader usage it might start > affecting the binary size. > Size w/o the patch: -rwxrwxrwx 1 yurys eng 150380952 Jun 25 18:36 out/Release/chrome Size with the patch: -rwxrwxrwx 1 yurys eng 150378760 Jun 25 18:06 out/Release/chrome So the binary size decreased a little bit, though the difference is negligible.
https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:38: if (!m_serialized.isSafeToSendToAnotherThread()) On 2014/06/25 14:27:25, caseq wrote: > Do we expect to ever hit this? I believe not, replaced with ASSERT. https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:65: On 2014/06/25 14:12:50, dsinclair wrote: > nit: extra blank line. Done. https://codereview.chromium.org/357703002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:74: static void createQuad(TracedValue* value, const char* name, const FloatQuad& quad) On 2014/06/25 14:12:50, dsinclair wrote: > Can the TraceValue* be passed by reference so we know it can never be null by > accident? Done. https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... File Source/platform/TracedValue.cpp (right): https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.cpp:39: bool isJSONObject(JSONValue* value) On 2014/06/25 14:27:25, caseq wrote: > I think explicitly comparing against value->type() is more readable and can be > conveniently inlined. Good point. I forgot that we have type getter. https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... File Source/platform/TracedValue.h (right): https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.h:25: void setInteger(const char* name, int value); On 2014/06/25 14:27:25, caseq wrote: > Can we put these methods on TracedDictionary... TracedDictionary is a template which means that these methods would be duplicated for each instantiation. https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.h:33: void pushInteger(int); On 2014/06/25 14:27:25, caseq wrote: > ... and these to TracedArray instead? I'd like to avoid this for the same reason as above. Alternatively we could have intermediate non-template TracedDictionaryBase and TracedArrayBase that would contain these methods. It looks like unnecessary complication though and since this is an implementation detail I'd rather keep things as is. https://codereview.chromium.org/357703002/diff/20001/Source/platform/TracedVa... Source/platform/TracedValue.h:139: class PLATFORM_EXPORT TracedValue : public TracedValueBase { On 2014/06/25 14:27:25, caseq wrote: > Can we inherit from TracedDictionary instead? I don't think so it has different implementation of beginDictionary, beginArray and provides endDictionary.
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8572) linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8578)
lgtm
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/357703002/80001
Message was sent while issue was closed.
Change committed as 177086 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
