base: Simplify trace arguments passing |
|||
Issue descriptionTRACE_EVENT() macros can take 0, 1 or 2 named arguments, with liberal values like integers, enums, floating points, strings or even scoped pointers to ConvertableToTraceFormat instances. The way these arguments are actually recorded by the macros is complicated and results in generated machine code that is more complex than necessary. One way to simplify that is to use a small dedicated data structure to store the argument names, types and values more efficiently. I have already experimented with a new approach, with the CL at [1] which shows that this new approach works (and reduces the size of the generated code for libmonochrome.so on 32-bit ARM by 12 kiB). This entry is to track the issue, and modify the Chromium sources in several steps to move to the new model, i.e.: 1) Introduce the new TraceArguments data structure, and add new AddTraceEvent() and AddMetaDataEvent() methods that accepts one instance as a parameter (in addition to the existing ones). 2) Modify the TRACE_EVENTXX() macros to use the TraceArguments based methods by default. 3) Modify other explicit callers of the AddTraceEvent() or AddMetaDataEvent() methods to use the TraceArguments based version of the methods. 4) Once all callers have been updated, remove the old methods that still accept |num_args|, |arg_types|, |arg_names|, |arg_values|, and |convertables| parameters instead. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1269728
,
Oct 25
,
Oct 25
No, there is absolutely no change to the macros, or the call sites (except a few places that call TraceLog::AddTraceEvent() directly, but mostly to convert their own trace event format for the Chromium one, e.g. v8 or skia).
,
Oct 25
And you can have a look at the CL referenced in #1 that contains all the changes involved, if you want to look at the nitty gritty details. The main point is simply reducing the generated machine code at each call site, without impacting the callers.
,
Oct 25
Thanks for the link to the tracing-unbundling doc. I don't think this change should impact anything about the refactoring, since it only modifies how the TRACE_EVENTXX() macros will call the TraceLog::AddTraceEvent() methods. Everything else after that is essentially the same, with few subtle differences. It should be possible to port the same changes to Skia/WebRTC/V8 in the future, but there are no blocking dependencies here, as far as I understand.
,
Oct 26
Another thing that I've discovered is that the trace macros end up generating machine code that does something like:
TraceLog::GetInstance()->AddTraceEvent(<arguments>).
This translates to machine code that performs two machine calls, i.e.
1) One direct machine call to TraceLog::GetInstance()
2) One direct machine call to the TraceLog::AddTraceEvent() methods
(using the result of the first call as a first parameter).
Given that this is repeated thousands of time in the Chromium code base, it might be beneficial to provide a single static method that implements the same thing, i.e. something like:
--- trace_event_impl.h
static TraceEventHandle TraceLogHelper::AddTraceEvent(<arguments>);
--- trace_event_impl.cc or even trace_log.cc
// static
TraceEventHandle TraceLogHelper::AddTraceEvent(<arguments>) {
return TraceLog::GetInstance()->AddTraceEvent(<arguments>);
}
I have a working prototype that does just that, and reduces the size of libmonochrome.so by yet another 12 kiB. I'll try to clean it up for this bug too.
,
Oct 26
Re #2-#5, ah okay, sorry I misread your initial intention then. skyostil@/oysteine@, wdyt? Re #6: You are definitely right, I'd hope that if we put that into trace_log.cc, the compiler might be able to avoid the 2nd call and just inline AddTraceEvent? Is there an other way we could ensure that we avoid this to become 2 nested calls (e.g. marking TraceLog::AddTraceEvent always_inline ?)?
,
Oct 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87 commit 8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87 Author: David 'Digit' Turner <digit@google.com> Date: Mon Oct 29 15:33:57 2018 Introduce base::trace_event::TraceArguments helper class. This CL introduces the TraceArguments helper class to simplify the way TRACE_EVENTXXX macro named arguments are passed to the trace log. By using a small dedicated data structure, it is possible to generate less code at each trace call site, compared to the old way that relies on passing 5 specially crafted arguments to TraceLog methods. For example, this optimization can save about 12 kiB in libmonochrome.so for the 32-bit ARM build of Chrome on Android. For more details, see corresponding bug entry. This CL does the following: - Introduce the new class in base/trace_event/trace_arguments.h - Move the definition of TraceValue to trace_arguments.h and augment the union with templated methods to support easy initialization from a large set of C++ value types. - Add new TraceLog methods to add trace events using an optional TraceArguments parameter, instead of a bag of 5 ones (count + names + types + values + convertables). - Simplify internal templates using C++11 universal references and std::forward<> to remove special-casing convertable arguments in trace event calls. - Remove the now obsolete TraceValueUnion type from trace_event.h - Make the TraceEvent class fully C++11 movable, to simplify its usage (this removes the Initialize() and MoveFrom() methods). - Update base/trace_event/ code to use TraceArguments and the new TraceEvent move operations. - Note that this keeps a few forwarding data types, values and methods to avoid other direct callers from the Chromium tree, these will be updated in future CLs. One Blink header needs to be modified though because it specializes a template defined in trace_arguments.h, to support sending WTF::CString references as trace arguments. BUG= 898794 R=oysteine@chromium.org,chiniforooshan@chromium.org,alexiln@chromium.org TBR=primiano@chromium.org,torne@chromium.org Change-Id: I8fe2225974f7044515a2f64bdc67f6d36abfe004 Reviewed-on: https://chromium-review.googlesource.com/c/1298997 Commit-Queue: David Turner <digit@chromium.org> Reviewed-by: Richard Coles <torne@chromium.org> Reviewed-by: oysteine <oysteine@chromium.org> Cr-Commit-Position: refs/heads/master@{#603500} [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/BUILD.gn [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/blame_context.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/event_name_filter_unittest.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/process_memory_dump.cc [add] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_arguments.cc [add] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_arguments.h [add] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_arguments_unittest.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event.h [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event_android.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event_argument.h [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event_etw_export_win.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event_etw_export_win.h [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event_impl.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event_impl.h [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_event_unittest.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_log.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/trace_log.h [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/base/trace_event/traced_value.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/services/tracing/public/cpp/perfetto/trace_event_data_source.cc [modify] https://crrev.com/8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87/third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h
,
Oct 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d commit 2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d Author: Dominic Mazzoni <dmazzoni@chromium.org> Date: Mon Oct 29 18:11:51 2018 Revert "Introduce base::trace_event::TraceArguments helper class." This reverts commit 8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87. Reason for revert: Suspecting that it's causing these tests to fail: services_unittests MemoryTracingIntegrationTest.GenerationChangeDoesntReenterMDM content_browsertests MemoryTracingTest.BrowserInitiatedDump See bug 899813 Original change's description: > Introduce base::trace_event::TraceArguments helper class. > > This CL introduces the TraceArguments helper class to simplify > the way TRACE_EVENTXXX macro named arguments are passed to the > trace log. By using a small dedicated data structure, it is > possible to generate less code at each trace call site, > compared to the old way that relies on passing 5 specially > crafted arguments to TraceLog methods. > > For example, this optimization can save about 12 kiB in > libmonochrome.so for the 32-bit ARM build of Chrome on Android. > For more details, see corresponding bug entry. > > This CL does the following: > > - Introduce the new class in base/trace_event/trace_arguments.h > > - Move the definition of TraceValue to trace_arguments.h and > augment the union with templated methods to support easy > initialization from a large set of C++ value types. > > - Add new TraceLog methods to add trace events using an > optional TraceArguments parameter, instead of a bag of > 5 ones (count + names + types + values + convertables). > > - Simplify internal templates using C++11 universal references > and std::forward<> to remove special-casing convertable > arguments in trace event calls. > > - Remove the now obsolete TraceValueUnion type from trace_event.h > > - Make the TraceEvent class fully C++11 movable, to simplify > its usage (this removes the Initialize() and MoveFrom() methods). > > - Update base/trace_event/ code to use TraceArguments and the > new TraceEvent move operations. > > - Note that this keeps a few forwarding data types, values and > methods to avoid other direct callers from the Chromium tree, > these will be updated in future CLs. One Blink header needs > to be modified though because it specializes a template > defined in trace_arguments.h, to support sending WTF::CString > references as trace arguments. > > BUG= 898794 > R=oysteine@chromium.org,chiniforooshan@chromium.org,alexiln@chromium.org > TBR=primiano@chromium.org,torne@chromium.org > > Change-Id: I8fe2225974f7044515a2f64bdc67f6d36abfe004 > Reviewed-on: https://chromium-review.googlesource.com/c/1298997 > Commit-Queue: David Turner <digit@chromium.org> > Reviewed-by: Richard Coles <torne@chromium.org> > Reviewed-by: oysteine <oysteine@chromium.org> > Cr-Commit-Position: refs/heads/master@{#603500} TBR=digit@chromium.org,primiano@chromium.org,torne@chromium.org,chiniforooshan@chromium.org,oysteine@chromium.org,alexiln@chromium.org Change-Id: I165b96fde68d927f966bdb4ed8649dd7bb4dd0f5 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 898794 , 899813 Reviewed-on: https://chromium-review.googlesource.com/c/1305250 Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Commit-Position: refs/heads/master@{#603564} [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/BUILD.gn [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/blame_context.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/event_name_filter_unittest.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/process_memory_dump.cc [delete] https://crrev.com/595fa04ecb30e9a034e982d260fa321e8ac6646a/base/trace_event/trace_arguments.cc [delete] https://crrev.com/595fa04ecb30e9a034e982d260fa321e8ac6646a/base/trace_event/trace_arguments.h [delete] https://crrev.com/595fa04ecb30e9a034e982d260fa321e8ac6646a/base/trace_event/trace_arguments_unittest.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event.h [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_android.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_argument.h [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_etw_export_win.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_etw_export_win.h [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_impl.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_impl.h [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_unittest.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_log.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_log.h [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/traced_value.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/services/tracing/public/cpp/perfetto/trace_event_data_source.cc [modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fc57a1780b06d0444fde693de8c733f8247ece83 commit fc57a1780b06d0444fde693de8c733f8247ece83 Author: David 'Digit' Turner <digit@google.com> Date: Thu Nov 15 14:37:46 2018 base/trace_event: Make TRACE_EVENTXX call sites smaller. Replaces two function calls by one at each trace macro call site. This should save about 12 kiB in 32-bit ARM libmonochrome.so BUG= 898794 R=primiano@chromium.org,oysteine@chromium.org,alexiln@chromium.org Change-Id: Icf64644355b584dd5ff9910920df02930344f837 Reviewed-on: https://chromium-review.googlesource.com/c/1301913 Commit-Queue: David Turner <digit@chromium.org> Reviewed-by: oysteine <oysteine@chromium.org> Cr-Commit-Position: refs/heads/master@{#608362} [modify] https://crrev.com/fc57a1780b06d0444fde693de8c733f8247ece83/base/trace_event/trace_event.h [modify] https://crrev.com/fc57a1780b06d0444fde693de8c733f8247ece83/base/trace_event/trace_log.cc [modify] https://crrev.com/fc57a1780b06d0444fde693de8c733f8247ece83/base/trace_event/trace_log.h
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03982f120083225b52db5385953f95fcf24d75b0 commit 03982f120083225b52db5385953f95fcf24d75b0 Author: David 'Digit' Turner <digit@google.com> Date: Tue Nov 27 18:05:34 2018 base: Make TraceEvent a movable class. This is a small cleanup of the TraceEvent class performed which is part of a larget bug allowing cleaning up and reducing the generated machine code for TRACE_EVENTXXX() macro calls (see related bug). A first CL to perform this refactor was submitted as [1], but later reverted because it made some tests fail mysteriously (see http://crbug.com/899813). So the original CL was split into several independent ones. A first CL was submitted as [2], which actually fixed some potential dangling pointer issues that were created from the Initialize() and MoveFrom() methods. This second CL removes this methods by making TraceEvent a proper C++11 movable type, which should prevent (or at least make it more difficult) introducing invalid states for its instances. The goal is to see if this introduces new unexpected test failures (which would indicate that there are still invalid instance states used in the code base). The third CL is [3] and re-introduces the TraceArguments helper class on top of this one. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1318919 [2] https://chromium-review.googlesource.com/c/chromium/src/+/1340308 [3] https://chromium-review.googlesource.com/c/chromium/src/+/1318919 BUG= 898794 R=primiano@chromium.org,oysteine@chromium.org,alexilin@chromium.org,chiniforooshan@chromium.org Change-Id: I2b36885e2485d23cca199c48b2bd07d5745b00c5 Reviewed-on: https://chromium-review.googlesource.com/c/1346305 Commit-Queue: David Turner <digit@chromium.org> Reviewed-by: Ehsan Chiniforooshan <chiniforooshan@chromium.org> Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Reviewed-by: oysteine <oysteine@chromium.org> Cr-Commit-Position: refs/heads/master@{#611185} [modify] https://crrev.com/03982f120083225b52db5385953f95fcf24d75b0/base/trace_event/event_name_filter_unittest.cc [modify] https://crrev.com/03982f120083225b52db5385953f95fcf24d75b0/base/trace_event/trace_event_impl.cc [modify] https://crrev.com/03982f120083225b52db5385953f95fcf24d75b0/base/trace_event/trace_event_impl.h [modify] https://crrev.com/03982f120083225b52db5385953f95fcf24d75b0/base/trace_event/trace_log.cc
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9db27e0d6107eae10975165294a25d3e5c9f4f24 commit 9db27e0d6107eae10975165294a25d3e5c9f4f24 Author: Shuotao Gao <stgao@chromium.org> Date: Tue Nov 27 19:44:25 2018 Revert "base: Make TraceEvent a movable class." This reverts commit 03982f120083225b52db5385953f95fcf24d75b0. Reason for revert: Caused compile failure on code coverage bot https://bugs.chromium.org/p/chromium/issues/detail?id=908937 Original change's description: > base: Make TraceEvent a movable class. > > This is a small cleanup of the TraceEvent class performed > which is part of a larget bug allowing cleaning up and > reducing the generated machine code for TRACE_EVENTXXX() > macro calls (see related bug). > > A first CL to perform this refactor was submitted as [1], but > later reverted because it made some tests fail mysteriously > (see http://crbug.com/899813). So the original CL was split into > several independent ones. > > A first CL was submitted as [2], which actually fixed some > potential dangling pointer issues that were created from the > Initialize() and MoveFrom() methods. > > This second CL removes this methods by making TraceEvent a > proper C++11 movable type, which should prevent (or at least > make it more difficult) introducing invalid states for its > instances. The goal is to see if this introduces new unexpected > test failures (which would indicate that there are still invalid > instance states used in the code base). > > The third CL is [3] and re-introduces the TraceArguments helper > class on top of this one. > > [1] https://chromium-review.googlesource.com/c/chromium/src/+/1318919 > [2] https://chromium-review.googlesource.com/c/chromium/src/+/1340308 > [3] https://chromium-review.googlesource.com/c/chromium/src/+/1318919 > > BUG= 898794 > R=primiano@chromium.org,oysteine@chromium.org,alexilin@chromium.org,chiniforooshan@chromium.org > > Change-Id: I2b36885e2485d23cca199c48b2bd07d5745b00c5 > Reviewed-on: https://chromium-review.googlesource.com/c/1346305 > Commit-Queue: David Turner <digit@chromium.org> > Reviewed-by: Ehsan Chiniforooshan <chiniforooshan@chromium.org> > Reviewed-by: Alexandr Ilin <alexilin@chromium.org> > Reviewed-by: oysteine <oysteine@chromium.org> > Cr-Commit-Position: refs/heads/master@{#611185} TBR=digit@chromium.org,primiano@chromium.org,chiniforooshan@chromium.org,oysteine@chromium.org,alexilin@chromium.org Change-Id: I73d85745e3b67cbcecddd25cb78a8e22ba031965 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 898794 Reviewed-on: https://chromium-review.googlesource.com/c/1351557 Reviewed-by: Shuotao Gao <stgao@chromium.org> Commit-Queue: Shuotao Gao <stgao@chromium.org> Cr-Commit-Position: refs/heads/master@{#611259} [modify] https://crrev.com/9db27e0d6107eae10975165294a25d3e5c9f4f24/base/trace_event/event_name_filter_unittest.cc [modify] https://crrev.com/9db27e0d6107eae10975165294a25d3e5c9f4f24/base/trace_event/trace_event_impl.cc [modify] https://crrev.com/9db27e0d6107eae10975165294a25d3e5c9f4f24/base/trace_event/trace_event_impl.h [modify] https://crrev.com/9db27e0d6107eae10975165294a25d3e5c9f4f24/base/trace_event/trace_log.cc
,
Nov 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d84875638d699d0110af1f958c8157cb11f4d4db commit d84875638d699d0110af1f958c8157cb11f4d4db Author: David 'Digit' Turner <digit@google.com> Date: Wed Nov 28 19:25:03 2018 base: Make TraceEvent a movable class. This is a re-land of [1] that has been fixed to not trigger the Clang crash when coverage instrumentation is enabled. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1346305 ---------------- original commit message ---------------------- This is a small cleanup of the TraceEvent class performed which is part of a larget bug allowing cleaning up and reducing the generated machine code for TRACE_EVENTXXX() macro calls (see related bug). A first CL to perform this refactor was submitted as [1], but later reverted because it made some tests fail mysteriously (see http://crbug.com/899813). So the original CL was split into several independent ones. A first CL was submitted as [2], which actually fixed some potential dangling pointer issues that were created from the Initialize() and MoveFrom() methods. This second CL removes this methods by making TraceEvent a proper C++11 movable type, which should prevent (or at least make it more difficult) introducing invalid states for its instances. The goal is to see if this introduces new unexpected test failures (which would indicate that there are still invalid instance states used in the code base). The third CL is [3] and re-introduces the TraceArguments helper class on top of this one. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1318919 [2] https://chromium-review.googlesource.com/c/chromium/src/+/1340308 [3] https://chromium-review.googlesource.com/c/chromium/src/+/1318919 ------------------ end of original commit message ------------ BUG= 898794 R=primiano@chromium.org,oysteine@chromium.org,alexilin@chromium.org,chiniforooshan@chromium.org Change-Id: Icb332f91dd9724a2acae9cb1f99a580541e6cc42 Reviewed-on: https://chromium-review.googlesource.com/c/1353929 Reviewed-by: Ehsan Chiniforooshan <chiniforooshan@chromium.org> Commit-Queue: David Turner <digit@chromium.org> Cr-Commit-Position: refs/heads/master@{#611817} [modify] https://crrev.com/d84875638d699d0110af1f958c8157cb11f4d4db/base/trace_event/event_name_filter_unittest.cc [modify] https://crrev.com/d84875638d699d0110af1f958c8157cb11f4d4db/base/trace_event/trace_event_impl.cc [modify] https://crrev.com/d84875638d699d0110af1f958c8157cb11f4d4db/base/trace_event/trace_event_impl.h [modify] https://crrev.com/d84875638d699d0110af1f958c8157cb11f4d4db/base/trace_event/trace_log.cc
,
Dec 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eebbd7f63c58e92d8b7081b3287b28f07706b95f commit eebbd7f63c58e92d8b7081b3287b28f07706b95f Author: David 'Digit' Turner <digit@google.com> Date: Mon Dec 10 12:49:11 2018 Introduce base::trace_event::TraceArguments helper class. NOTE: This a reland of [1] which got reverted in [2]. This CL introduces the TraceArguments helper class to simplify the way TRACE_EVENTXXX macro named arguments are passed to the trace log. By using a small dedicated data structure, it is possible to generate less code at each trace call site, compared to the old way that relies on passing 5 specially crafted arguments to TraceLog methods. For example, this optimization can save about 12 kiB in libmonochrome.so for the 32-bit ARM build of Chrome on Android. For more details, see corresponding bug entry. This CL does the following: - Introduce the new class in base/trace_event/trace_arguments.h - Move the definition of TraceValue to trace_arguments.h and augment the union with templated methods to support easy initialization from a large set of C++ value types. - Add new TraceLog methods to add trace events using an optional TraceArguments parameter, instead of a bag of 5 ones (count + names + types + values + convertables). - Simplify internal templates using C++11 universal references and std::forward<> to remove special-casing convertable arguments in trace event calls. - Remove the now obsolete TraceValueUnion type from trace_event.h - Update base/trace_event/ code to use TraceArguments and the new TraceEvent move operations. - Note that this keeps a few forwarding data types, values and methods to avoid other direct callers from the Chromium tree, these will be updated in future CLs. One Blink header needs to be modified though because it specializes a template defined in trace_arguments.h, to support sending WTF::CString references as trace arguments. BUG= 898794 R=oysteine@chromium.org,chiniforooshan@chromium.org,alexiln@chromium.org,primiano@chromium.org,torne@chromium.org TBR=torne@chromium.org,primiano@chromium.org [1] https://chromium-review.googlesource.com/c/1298997 [2] https://chromium-review.googlesource.com/c/1305250 Change-Id: I67fa434ab1c29b65abd0f327d1c9d27973f54f6a Reviewed-on: https://chromium-review.googlesource.com/c/1318919 Commit-Queue: David Turner <digit@chromium.org> Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Reviewed-by: oysteine <oysteine@chromium.org> Reviewed-by: David Turner <digit@chromium.org> Cr-Commit-Position: refs/heads/master@{#615099} [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/BUILD.gn [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/blame_context.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/event_name_filter_unittest.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/process_memory_dump.cc [add] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_arguments.cc [add] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_arguments.h [add] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_arguments_unittest.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_event.h [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_event_android.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_event_etw_export_win.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_event_etw_export_win.h [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_event_impl.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_event_impl.h [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_event_unittest.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_log.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/base/trace_event/trace_log.h [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/services/tracing/public/cpp/perfetto/trace_event_data_source.cc [modify] https://crrev.com/eebbd7f63c58e92d8b7081b3287b28f07706b95f/third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h
,
Jan 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c1b9c28bfe1b5439f61e27148acd079e843d2129 commit c1b9c28bfe1b5439f61e27148acd079e843d2129 Author: David 'Digit' Turner <digit@google.com> Date: Tue Jan 08 15:13:11 2019 gin,ppapi,skia: Use base::trace_event::TraceArguments BUG= 898794 R=oyesteine@chromium.org,primiano@chromium.org,jochen@chromium.org,djsollen@chromium.org,thakis@chromium.org,bbudge@chromium.org Change-Id: Ibceb95855c382430919a3bd6b0c366df4aa92ac9 Reviewed-on: https://chromium-review.googlesource.com/c/1392182 Reviewed-by: Bill Budge <bbudge@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: David Turner <digit@chromium.org> Cr-Commit-Position: refs/heads/master@{#620725} [modify] https://crrev.com/c1b9c28bfe1b5439f61e27148acd079e843d2129/gin/v8_platform.cc [modify] https://crrev.com/c1b9c28bfe1b5439f61e27148acd079e843d2129/ppapi/shared_impl/ppb_trace_event_impl.cc [modify] https://crrev.com/c1b9c28bfe1b5439f61e27148acd079e843d2129/skia/ext/event_tracer_impl.cc
,
Jan 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e commit fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e Author: David 'Digit' Turner <digit@google.com> Date: Wed Jan 16 01:01:56 2019 Remove legacy trace macro entry points Adjust all callers of the legacy entry points to use base::trace_event::Arguments instead, and remove the code from TraceLog class. R=oysteine@chromium.org,primiano@chromium.org Bug: 898794 Change-Id: Ie47d731f4a4904f2beea83eb52785d9a7ad895be Reviewed-on: https://chromium-review.googlesource.com/c/1409517 Reviewed-by: Primiano Tucci <primiano@chromium.org> Reviewed-by: Henrik Grunell <grunell@chromium.org> Reviewed-by: Jonathan Backer <backer@chromium.org> Reviewed-by: Tommi <tommi@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: David Turner <digit@chromium.org> Cr-Commit-Position: refs/heads/master@{#622956} [modify] https://crrev.com/fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e/base/trace_event/trace_log.cc [modify] https://crrev.com/fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e/base/trace_event/trace_log.h [modify] https://crrev.com/fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc [modify] https://crrev.com/fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_observer.cc [modify] https://crrev.com/fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e/third_party/webrtc_overrides/init_webrtc.cc [modify] https://crrev.com/fd0d2d18ae70c83daa081fbc9ae5de0b0aad791e/ui/gl/angle_platform_impl.cc
,
Jan 16
(6 days ago)
Finally closing this as Fixed and Verified :) It's been an interesting journey! |
|||
►
Sign in to add a comment |
|||
Comment 1 by primiano@chromium.org
, Oct 25