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.

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

Enable leak tracking for release mode. #25

Open
1 of 2 tasks
polina-c opened this issue Dec 13, 2022 · 15 comments
Open
1 of 2 tasks

Enable leak tracking for release mode. #25

polina-c opened this issue Dec 13, 2022 · 15 comments
Assignees
Labels
P2 A bug or feature request we're likely to work on Size:M weeks

Comments

@polina-c
Copy link
Contributor

polina-c commented Dec 13, 2022

Subtasks:

If you are interested in leak tracking in release mode, please, upvote this issue and justify your case.

@marandaneto
Copy link

I believe this would be very helpful for release builds as well, so people can upload their leaks to error reporting tools such as sentry.io and fix them.
Similar to https://square.github.io/leakcanary/uploading/

@noga-dev
Copy link

noga-dev commented Apr 1, 2023

Could be useful for edge cases with special releases on beta channel. Would be good to add an enableInRelease flag.

@polina-c
Copy link
Contributor Author

polina-c commented Apr 3, 2023

@a-siva and @bkonyi , thoughts?

@a-siva
Copy link

a-siva commented Apr 4, 2023

@a-siva and @bkonyi , thoughts

This is for flutter release builds I presume or is this request for command line Dart ?

@bkonyi
Copy link
Contributor

bkonyi commented Apr 4, 2023

I assume this is for PRODUCT mode builds, which includes Flutter release. Given that this is basically a counter that we increment (even in PRODUCT mode, although it's not accessible) I think it's low-risk to expose this in PRODUCT mode. However, if this feature requires any sort of VM service connection to work properly then this won't be something we can do.

@polina-c
Copy link
Contributor Author

polina-c commented Apr 4, 2023

Thanks. Yes, it is just counter. VM service connection is not needed as the diagnostics will be saved and analyzed offline. Can we plan implementing this? Should I transfer the issue to VM repo?

@fzyzcjy
Copy link
Contributor

fzyzcjy commented May 31, 2023

I vote for it, if the performance drop is minimal in release mode. More sanity checks never do any harm (as long as they do not affect performance)!

@a-siva
Copy link

a-siva commented Jul 24, 2023

https://dart-review.googlesource.com/c/sdk/+/315981

@a-siva a-siva self-assigned this Jul 24, 2023
@a-siva a-siva closed this as completed Jul 24, 2023
@ZhouC125
Copy link

I vote for it, if the performance drop is minimal in release mode. More sanity checks never do any harm (as long as they do not affect performance)!

I agree with this comment.

@polina-c
Copy link
Contributor Author

The feature is unblocked, but not implemented yet, and not part of leak_tracker MVP.
Reopening.

@polina-c polina-c reopened this Aug 14, 2023
@polina-c polina-c added P2 A bug or feature request we're likely to work on Size:M weeks labels Aug 14, 2023
@a-siva
Copy link

a-siva commented Aug 14, 2023

The feature is unblocked, but not implemented yet, and not part of leak_tracker MVP. Reopening.

We have turned the reachability barrier on in product mode, why do you say it is not implemented yet?

@polina-c polina-c assigned polina-c and unassigned a-siva Aug 15, 2023
@polina-c
Copy link
Contributor Author

Issue title is 'Enable leak_tracker...'. I need to update leak_tracker

@polina-c
Copy link
Contributor Author

polina-c commented Aug 27, 2023

@itsjustkevin, I am reopening the issue: while reachability barrier is now available in release mode (thank you!!!), leak tracker is still off for release mode.

@polina-c polina-c reopened this Aug 27, 2023
@dnfield
Copy link

dnfield commented Jan 18, 2024

I do not think we should make this an option unless we have significant profiling data to support it.

In particular, we should have profiling data from a mid-tier Android device that shows this has little to no impact on frame/raster times. The profiling data we do have so far from macOS desktop applications suggests that it would.

@polina-c
Copy link
Contributor Author

I do not think we should make this an option unless we have significant profiling data to support it.

In particular, we should have profiling data from a mid-tier Android device that shows this has little to no impact on frame/raster times. The profiling data we do have so far from macOS desktop applications suggests that it would.

I agree. Most likely in release mode leak analysis and reporting should happen not on timer, but on request from application, when application is known to be in stale state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on Size:M weeks
Projects
None yet
Development

No branches or pull requests

8 participants