Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support the timeline on Android release #47264

Closed
dnfield opened this issue Sep 21, 2021 · 8 comments
Closed

Support the timeline on Android release #47264

dnfield opened this issue Sep 21, 2021 · 8 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. customer-flutter os-android type-performance Issue relates to performance or code size

Comments

@dnfield
Copy link
Contributor

dnfield commented Sep 21, 2021

Related:
#45143
flutter/flutter#90424

Systrace can be useful in release applications. Dart should support at least the systrace recorder in release mode.

It's not clear to me how much of an impact tracing has on performance though, and whether there are benchmarks that could show the cost of this even if it's turned off.

/cc @rmacnak-google @zanderso
@dnfield dnfield added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. type-performance Issue relates to performance or code size os-android customer-flutter labels Sep 21, 2021
@a-siva
Copy link
Contributor

a-siva commented Sep 21, 2021

//cc @bkonyi

@rmacnak-google
Copy link
Contributor

For comparison, we do this on Fuchsia by defining SUPPORT_TIMELINE.

@dnfield
Copy link
Contributor Author

dnfield commented Sep 22, 2021

sdk/runtime/vm/globals.h

Lines 113 to 116 in 8aa9716

#if !defined(PRODUCT) || defined(DART_HOST_OS_FUCHSIA) || \
defined(DART_TARGET_OS_FUCHSIA)
#define SUPPORT_TIMELINE 1
#endif
adding Android to this list would probably do it.

@a-siva
Copy link
Contributor

a-siva commented Sep 22, 2021

This could potentially cause some perf regressions on Android, are we fine with that ?

@dnfield
Copy link
Contributor Author

dnfield commented Sep 22, 2021

It'd be helpful to understand the cost (e.g. change in binary size, any runtime performance implication). I suspect the Golem benchmarks would catch these, and if not the devicelab right?

@a-siva
Copy link
Contributor

a-siva commented Sep 22, 2021

Yes, not sure how to do devicelab runs on a local change, maybe we should make the change and monitor for perf regressions when it rolls into Flutter?

@dnfield
Copy link
Contributor Author

dnfield commented Sep 22, 2021

SGTM. If it's not used it should be a pretty minimal cost, maybe one time if anything...

@a-siva
Copy link
Contributor

a-siva commented Sep 22, 2021

https://dart-review.googlesource.com/c/sdk/+/214241 is being submitted and we should monitor it as it rolls into Flutter for regressions.

copybara-service bot pushed a commit that referenced this issue Sep 22, 2021
@a-siva
[Runtime/VM - Enable SUPPORT_TIMELINE even in product mode for Android
b7de66e 
Please see #47264

The Flutter team would like to turn this on in product mode for being
able to systrace release applications.
This change could have potential size and performance regressions and
will be monitored as it rolls into Flutter. If the impact is not within
acceptable ranges it can be rolled back.

TEST=ci

Change-Id: Iad6f9b831eaf60d15f03e368702fb25bc9ebc76c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/214241
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Dan Field <dnfield@google.com>
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
@dnfield dnfield closed this as completed Sep 23, 2021
copybara-service bot pushed a commit that referenced this issue Mar 21, 2023
@jiahaog
[Runtime/VM] Enable SUPPORT_TIMELINE in product mode on iOS
5921f83 
This is useful to systrace release mode applications on iOS. With this change and with systrace disabled, the `TimelineEventNopRecorder` is used so there should not be any performance impact.

See also the following bug where this was done for Android.

TEST=ci

Bug: #47264
Change-Id: If3d127054d6345213fe0ade827dd60f7904a99a1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/289925
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Auto-Submit: Jia Hao Goh <jiahaog@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. customer-flutter os-android type-performance Issue relates to performance or code size
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rmacnak-google @dnfield @a-siva