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

Conversation

bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Aug 6, 2018

Updated DartCallbackCache to write callback cache to disk which is restored on engine startup

@@ -44,7 +45,8 @@ void FlutterMain::Init(JNIEnv* env,
jclass clazz,
jobject context,
jobjectArray jargs,
jstring bundlePath) {
jstring bundlePath,
jstring appRootPath) {
Copy link
Member

Choose a reason for hiding this comment

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

Who has write access to appRootPath? Could application code accidentally clobber the callback cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This parameter is only visible to the internals of FlutterMain.java which only sets it to the result of PathUtils.getFilesDir(applicationContext); so it shouldn't be possible for the application to clobber the cache unless it's really trying to (e.g., by deleting the cache file directly).

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm but also get a review from chinmay and/or jason.

@@ -229,7 +230,7 @@ public static void ensureInitializationComplete(Context applicationContext, Stri
}
}

private static native void nativeInit(Context context, String[] args, String bundlePath);
private static native void nativeInit(Context context, String[] args, String bundlePath, String appRootPath);
Copy link
Member

Choose a reason for hiding this comment

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

Change appRootPath to something reflecting that this path is writable (e.g. "storagePath" or "writablePath"?)

imho appRootPath sounds like a path where the app's own assets would live, inviting confusion with bundlePath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM, will do.

@@ -34,6 +56,7 @@ int64_t DartCallbackCache::GetCallbackHandle(const std::string& name,

if (cache_.find(hash) == cache_.end()) {
cache_[hash] = {name, class_name, library_path};
SaveCacheToDisk();
Copy link
Member

Choose a reason for hiding this comment

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

What is the lifecycle of the cache file? Is the cache file or any of its entries ever deleted?

Would anything within the cache become stale if the application's code is updated but the cache is not purged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the cache is persistent and entries aren't deleted, although we could verify all the callbacks in the cache actually exist when we restore and remove the stale entries.

@bkonyi bkonyi merged commit 3cbb5e2 into master Aug 7, 2018
@bkonyi bkonyi deleted the callback_cache_disk branch August 7, 2018 21:37
liyuqian added a commit that referenced this pull request Aug 7, 2018
liyuqian added a commit to liyuqian/flutter that referenced this pull request Aug 7, 2018
b3e866e Call drawPath without clip if possible (flutter/engine#5952)
7e0bb3b Allow freezing a texture. (flutter/engine#5938)
3cbb5e2 Persist DartCallbackCache contents across launches (flutter/engine#5947)
953570a libtxt: truncate paragraph width to an integer in order to match Blink's behavior (flutter/engine#5962)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants