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

Add new FlutterEngineAOTData argument to FlutterProjectArgs #18146

Merged
stuartmorgan-g merged 14 commits into from
May 10, 2020
Merged

Add new FlutterEngineAOTData argument to FlutterProjectArgs #18146

stuartmorgan-g merged 14 commits into from
May 10, 2020

Conversation

@MarcusTomlinson
Copy link
Contributor

@MarcusTomlinson MarcusTomlinson commented May 5, 2020

Added a new FlutterEngineAOTData argument to FlutterProjectArgs. Embedders can instantiate and destroy this object via the new FlutterEngineCreateAOTData and FlutterEngineCollectAOTData methods provided.

If an embedder provides more than one source of AOT data to FlutterEngineInitialize or FlutterEngineRun (e.g. snapshots as well as FlutterEngineAOTData), the engine will error out.

Resolves: flutter/flutter#50778

@auto-assign auto-assign bot requested a review from flar May 5, 2020 14:12
@MarcusTomlinson
Copy link
Contributor Author

MarcusTomlinson commented May 5, 2020 •
edited
Loading

For a real world example, here's the GLFW embedder updated to use the new API:

https://github.com/MarcusTomlinson/glfw

(see: README.md)

@MarcusTomlinson MarcusTomlinson changed the title Add new aot_data argument to FlutterProjectArgs Add new FlutterEngineAOTData argument to FlutterProjectArgs May 5, 2020
@flar flar removed their request for review May 5, 2020 20:21
stuartmorgan-g
stuartmorgan-g suggested changes May 5, 2020
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

I'll defer to Chinmay on the substantive part of the review; just some style nits from me. Looks great!

chinmaygarde
chinmaygarde reviewed May 5, 2020
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Apart from the pointer to AOT data in FluttereProjectArgs, everything else is minor nits. This is looking great. Thanks.

@MarcusTomlinson
Copy link
Contributor Author

MarcusTomlinson commented May 7, 2020

Thanks for the reviews guys! Learnt a lot from them!

@MarcusTomlinson
Copy link
Contributor Author

MarcusTomlinson commented May 7, 2020

@chinmaygarde, I just remembered, the additional kSnapshotsInitialize and kAOTDataInitialize initialisation preferences for the config builder are actually needed. If I default to populating just aot_data in AOT mode, we lose all coverage of the snapshots approach in the unit tests. I've put this back again.

stuartmorgan-g
stuartmorgan-g approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g 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; please wait for Chinmay's approval as well though.

chinmaygarde
chinmaygarde approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

One last comment about the lifecycle of the AOTData and when it may be collected. Other than that, lgtm. Thanks for the patch and the followups. This is great.

@MarcusTomlinson
Copy link
Contributor Author

MarcusTomlinson commented May 10, 2020

@chinmaygarde, @stuartmorgan: I think that covers it :) Please could you merge when you get the chance. Thanks!

@stuartmorgan-g stuartmorgan-g merged commit d96f962 into flutter:master May 10, 2020
@provokateurin provokateurin mentioned this pull request May 10, 2020
Merged
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 11, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2020
@engine-flutter-autoroll
d96f962 Add new FlutterEngineAOTData argument to FlutterProjectArgs (f…
20e9da9 
…lutter/engine#18146)
wandyers pushed a commit to wandyers/engine that referenced this pull request May 23, 2020
@MarcusTomlinson @wandyers
Add new FlutterEngineAOTData argument to FlutterProjectArgs (flutter#…
d9e3206 
…18146)

Added a new `FlutterEngineAOTData` argument to `FlutterProjectArgs`. Embedders can instantiate and destroy this object via the new `FlutterEngineCreateAOTData` and `FlutterEngineCollectAOTData` methods provided.

If an embedder provides more than one source of AOT data to `FlutterEngineInitialize` or `FlutterEngineRun` (e.g. snapshots as well as `FlutterEngineAOTData`), the engine will error out.

Resolves: flutter/flutter#50778
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose embedder API affordances for working with AOT buffers from disparate sources.

4 participants

@MarcusTomlinson @chinmaygarde @stuartmorgan-g @googlebot