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

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Aug 4, 2021

Fix implicit casts and implicit dynamics, and update analysis_options.yaml.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Aug 4, 2021
@google-cla google-cla bot added the cla: yes label Aug 4, 2021
@yjbanov yjbanov requested review from ferhatb and mdebbar August 4, 2021 17:21
/// keys are `dynamic` because when JSON is deserialized from method channels
/// it arrives as `Map<dynamic, dynamic>`.
// TODO(yjbanov): use Json typedef when type aliases are shipped
extension JsonExtensions on Map<dynamic, dynamic> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it generating same js code under O4 optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an overall increase of 0.03% (or 286 bytes) in code size for flutter_gallery when I compare flutter build web --release vs flutter --local-engine=host_debug_unopt build web --release with this PR. Some of these do become runtime casts (maybe something we can ask the Darat team to optimize for O4), but nearly all of these are not in the rendering pipeline. Most are I/O and platform messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking. Suggestion: if you see anything worth optimizing here for rendering pipeline, you can use unsafeCast. Something like:
@pragma('dart2js:tryInline') // Required for next pragma to be effective.
@pragma('dart2js:as:trust') // Omits as T.
T unsafeCast(dynamic any) => any as T;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. We should try that pragma on the framework side for the most expensive as casts.

@yjbanov yjbanov force-pushed the remove-implicit-dynamic branch from 5da7ce3 to 6b54c26 Compare August 4, 2021 22:55
@yjbanov
Copy link
Contributor Author

yjbanov commented Aug 5, 2021

The luci failure looks like a flake. Going to merge to kick the bots, but happy to revert is this causes trouble.

}
if (EngineSemanticsOwner.instance.receiveGlobalEvent(event)) {
return handler(event);
return handler(event) as html.EventListener?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there’s something wrong here.

loggedHandler *is* an event listener, it doesn’t return an event listener. And the same goes for handler. I reviewed the PR that introduced this mistake but I completely missed it 😔

I’ll fix it tomorrow morning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I guess this lint is useful after all :)

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants