This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Implement support for explicit specification of JIT snapshots #34244
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 21 commits
June 22, 2022 16:56
iskakaushik
reviewed
Jun 23, 2022
added 6 commits
June 23, 2022 17:34
Member
|
@iskakaushik All open queries seem resolved. Can we merge this please? |
bdero
approved these changes
Jul 7, 2022
Member
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
This still LGTM and looks like it should solve flutter/flutter#100640. @akbiggs could you take a look and confirm?
akbiggs
reviewed
Jul 7, 2022
Contributor
|
Also really sorry for the slow review, I was out sick. |
akbiggs
approved these changes
Jul 7, 2022
| TEST_F(EmbedderTest, CanSuccessfullySpecifyJITSnapshotLocations) { | ||
| #if defined(OS_FUCHSIA) | ||
| GTEST_SKIP() << "Inconsistent paths in Fuchsia."; | ||
| #endif // OS_FUCHSIA |
Contributor
There was a problem hiding this comment.
Please resolve this comment before submission (either by fixing or filing a bug) - I'm curious to know more about this issue.
iskakaushik
added
the
autosubmit
jason-simmons
added a commit
to jason-simmons/flutter_engine
that referenced
this pull request
Jul 7, 2022
…flutter#34244)" This reverts commit f3dd927.
jason-simmons
added a commit
that referenced
this pull request
Jul 7, 2022
Contributor
|
This appears to have broken some Dart/Flutter HHH benchmarking builds, see |
Contributor
|
Reverted in 177d1ad. |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 8, 2022
betrevisan
pushed a commit
to betrevisan/engine
that referenced
this pull request
Jul 8, 2022
…ts (flutter#34244)" This reverts commit 177d1ad.
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changes proposed in this PR will enable users to explicitly specify JIT snapshots. Currently, the engine relies on the asset resolver to identify the snapshot asset locations. There have been requests for increased flexibility when determining such locations, especially in cases in which the asset resolver fails to find a valid snapshot. Additionally, this PR was conceived with the goal of making the JIT mode consistent with the existing AOT mode, which allows users to explicitly describe such asset paths.
As such, I built the FlutterEngineSetupJITSnapshots function within embedder.cc, which will set the snapshots in the embedder ProjectArgs to the user defined locations. At initialization, the engine will call the PopulateJITSnapshotMappingCallbacks function, which will set the snapshots to their corresponding mapping callbacks as defined by the user and add them to the Settings. In the cases in which no snapshots are explicitly defined, the engine will resolve the assets as before since the snapshots will be set to nullptr in the Settings.
This PR stems from the discussion raised in flutter/flutter#100640.