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

Provide APIs for the PlatformDispatcher to set the performance mode #35614

Merged
merged 5 commits into from
Aug 29, 2022

Conversation

iskakaushik
Copy link
Contributor

No description provided.

/// Returns the old performance mode of the Dart VM.
///
/// See [DartPerformanceMode] for more information on individual performance modes.
DartPerformanceMode requestDartPerformanceMode(DartPerformanceMode mode) {
Copy link
Member

Choose a reason for hiding this comment

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

I would not necessarily return the current performance mode. What is the plan for this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was mostly going to use it when adding tests on the framework side, makes it easy to assert instead of having to add GetCurrentPerformanceMode. Let me know if you think this is a bad idea, happy to make it a void function.

Copy link
Member

Choose a reason for hiding this comment

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

I would consider removing it, and the asserts in the framework should track whether or not the requested GC modes are consistent.

Consider: we may wish to disable this functionality on certain platforms, or tune other changes in the engine that overrwrite the requested mode. If we tell people what the resulting mode is we're going to make that part of the contract of this API - which folks will depend on, eventually.

Also consider: in the event the framework is making bad requests, but the engine is ignoring those requests for an unrelated reason, we'd like the framework asset/test to still fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are valid points, will make it not return anything for now.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

You'll also need a web impl, but since this is just a request it can no-op

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM with nits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
2 participants