Do not produce timeline events in release mode #15894
Conversation
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Are we sure Fuchsia isn't using this?
|
Yes, Fuchsia benchmarks depend on having these events enabled even on release builds. On Fuchsia, we try to configure tracing such that it's very cheap if tracing is not currently in progress, and still cheap if tracing is in progress but the category for a particular event was disabled. Since this change will result in immediate build issues for Fuchsia, I ask that it not be submitted right now (at least for the Fuchsia platform). If it's shown to be causing performance issues on Fuchsia, then we can have a followup discussion to investigate further, and decide how to proceed from there. |
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Keeping the signatures in sync between the release and non-release variants might get complicated. It's still ugly, but, can we just guard each call to Dart_TimelineEvent in this file using this macro?
Also, maybe make the macro TIMELINE_ENABLED or something similar so we can set if on in all modes for Fuchsia.
jason-simmons
force-pushed
the
trace_event_release
branch
from
e50d913 to
8627668
Compare
|
Changed this to disable the timeline in release mode only on non-Fuchsia targets @chinmaygarde The goal of duplicating the signatures in TIMELINE_ENABLED and non-TIMELINE_ENABLED is to prevent accidentally adding a new timeline function and forgetting to #ifdef out the Dart_TimelineEvent call within that function. |
Calling Dart_TimelineEvent has a performance cost, and the APIs for collecting timeline events are not available in release mode.
jason-simmons
force-pushed
the
trace_event_release
branch
from
8627668 to
780d39a
Compare
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
The goal of duplicating the signatures in TIMELINE_ENABLED and non-TIMELINE_ENABLED is to prevent accidentally adding a new timeline function and forgetting to #ifdef out the Dart_TimelineEvent call within that function.
I was more concerned about adding new methods in one path but finding that the build fails on the other (usually found via a failure on a bot somewhere). But, either way, don't feel too strongly about it.
flutter/engine@439a218...f10f03a git log 439a218..f10f03a --first-parent --oneline 2020-01-24 [email protected] Fix data race in DartIsolateGroupData. (flutter/engine#15949) 2020-01-23 [email protected] [fuchsia] Add LogSink to flutter_[jit & aot]_product_runner (flutter/engine#15697) 2020-01-23 [email protected] Do not produce timeline events in release mode (flutter/engine#15894) 2020-01-23 [email protected] Roll src/third_party/dart e84bea25df23..3eaae5405d37 (17 commits) (flutter/engine#15946) 2020-01-23 [email protected] Roll src/third_party/skia e4ddb8a7cddc..c88a3bc3f561 (24 commits) (flutter/engine#15945) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Calling Dart_TimelineEvent has a performance cost, and the APIs for collecting timeline events are not available in release mode.
This reverts commit 2c26488.
5 participants
Calling Dart_TimelineEvent has a performance cost, and the APIs for collecting
timeline events are not available in release mode.