Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@polina-c
Copy link
Contributor

@polina-c polina-c commented Aug 9, 2022

See go/dart-leaktracker-productization.

Before merging to https://github.com/flutter/engine/tree/main/lib:

@polina-c
Copy link
Contributor Author

polina-c commented Aug 9, 2022

@yjbanov , can you advise, please - is it right understanding that I need to add instrumentation both in lib/ui and in lib/web_ui?

@yjbanov
Copy link
Contributor

yjbanov commented Aug 9, 2022

@yjbanov , can you advise, please - is it right understanding that I need to add instrumentation both in lib/ui and in lib/web_ui?

The API should be identical, but if leak tracking a VM-only feature that doesn't have an equivalent in dart2js/DDC, then the web version can remain a noop.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Aug 10, 2022
@polina-c polina-c marked this pull request as ready for review August 11, 2022 20:21
@skia-gold
Copy link

Gold has detected about 37 new digest(s) on patchset 21.
View them at https://flutter-engine-gold.skia.org/cl/github/35274

@polina-c polina-c mentioned this pull request Aug 29, 2022
@polina-c polina-c marked this pull request as ready for review September 1, 2022 20:30

ui.Image.onCreate = null;
// TODO(polina-c): unskip the test when bug is fixed:
// https://github.com/flutter/engine/pull/35791
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#35791 is now merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was wrong URL. Replaced with right one. The issue is not fixed yet.


ui.Image.onDispose = null;
// TODO(polina-c): unskip the test when bug is fixed:
// https://github.com/flutter/engine/pull/35791
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

/// useful when trying to determine what parts of the program are keeping an
/// image resident in memory.
void dispose() {
onDispose?.call(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere: can we guard calling these so it's compiled out of release mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I know there is no compile time flags (like bool.fromEnvironment('flutter.memory_allocations')) in engine.
And 'normal' flags will not be more efficient both from performance perspective and code size perspective, than comparison to null. So there is no point in such guarding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline - any flag we put in here would be at best a runtime check that's probably no faster than the null check here.

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants