Migrate ML Downloader to DataStore#8418
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates SharedPreferencesUtil from Android's SharedPreferences to Jetpack DataStore-backed JavaDataStorage. The review feedback highlights several performance and consistency issues arising from the migration. Specifically, the reviewer recommends retrieving all preferences in a single snapshot using dataStore.getAllSync() to avoid multiple sequential, non-atomic blocking reads in getCustomModelDetails and getDownloadingCustomModelDetails. Additionally, they suggest compiling regex patterns outside of loops and optimizing getCustomModelStatsCollectionFlag to avoid redundant sequential blocking calls.
| public CustomModel getCustomModelDetails(@NonNull String modelName) { | ||
| String modelHash = dataStore.getSync(localModelHashKey(modelName), null); | ||
|
|
||
| if (modelHash == null || modelHash.isEmpty()) { | ||
| // no model downloaded - check if model is being downloaded. | ||
| return getDownloadingCustomModelDetails(modelName); | ||
| } | ||
|
|
||
| String filePath = | ||
| getSharedPreferences() | ||
| .getString(String.format(LOCAL_MODEL_FILE_PATH_PATTERN, persistenceKey, modelName), ""); | ||
| String filePath = dataStore.getSync(localModelFilePathKey(modelName), ""); | ||
|
|
||
| long fileSize = | ||
| getSharedPreferences() | ||
| .getLong(String.format(LOCAL_MODEL_FILE_SIZE_PATTERN, persistenceKey, modelName), 0); | ||
| long fileSize = dataStore.getSync(localModelFileSizeKey(modelName), 0L); | ||
|
|
||
| // if no-zero - local model is present and new model being downloaded | ||
| long id = | ||
| getSharedPreferences() | ||
| .getLong(String.format(DOWNLOADING_MODEL_ID_PATTERN, persistenceKey, modelName), 0); | ||
| long id = dataStore.getSync(downloadingModelIdKey(modelName), 0L); |
There was a problem hiding this comment.
Calling dataStore.getSync(...) multiple times sequentially performs multiple blocking flow collections, which is inefficient. Furthermore, because these reads are not atomic, a concurrent write could modify the preferences between reads, leading to an inconsistent CustomModel state.
We can retrieve all preferences in a single consistent snapshot using dataStore.getAllSync() and extract the values from the map.
| public CustomModel getCustomModelDetails(@NonNull String modelName) { | |
| String modelHash = dataStore.getSync(localModelHashKey(modelName), null); | |
| if (modelHash == null || modelHash.isEmpty()) { | |
| // no model downloaded - check if model is being downloaded. | |
| return getDownloadingCustomModelDetails(modelName); | |
| } | |
| String filePath = | |
| getSharedPreferences() | |
| .getString(String.format(LOCAL_MODEL_FILE_PATH_PATTERN, persistenceKey, modelName), ""); | |
| String filePath = dataStore.getSync(localModelFilePathKey(modelName), ""); | |
| long fileSize = | |
| getSharedPreferences() | |
| .getLong(String.format(LOCAL_MODEL_FILE_SIZE_PATTERN, persistenceKey, modelName), 0); | |
| long fileSize = dataStore.getSync(localModelFileSizeKey(modelName), 0L); | |
| // if no-zero - local model is present and new model being downloaded | |
| long id = | |
| getSharedPreferences() | |
| .getLong(String.format(DOWNLOADING_MODEL_ID_PATTERN, persistenceKey, modelName), 0); | |
| long id = dataStore.getSync(downloadingModelIdKey(modelName), 0L); | |
| public CustomModel getCustomModelDetails(@NonNull String modelName) { | |
| Map<Preferences.Key<?>, Object> allPrefs = dataStore.getAllSync(); | |
| String modelHash = (String) allPrefs.get(localModelHashKey(modelName)); | |
| if (modelHash == null || modelHash.isEmpty()) { | |
| // no model downloaded - check if model is being downloaded. | |
| return getDownloadingCustomModelDetails(modelName); | |
| } | |
| String filePath = (String) allPrefs.getOrDefault(localModelFilePathKey(modelName), ""); | |
| long fileSize = (Long) allPrefs.getOrDefault(localModelFileSizeKey(modelName), 0L); | |
| // if no-zero - local model is present and new model being downloaded | |
| long id = (Long) allPrefs.getOrDefault(downloadingModelIdKey(modelName), 0L); |
| public CustomModel getDownloadingCustomModelDetails(@NonNull String modelName) { | ||
| String modelHash = dataStore.getSync(downloadingModelHashKey(modelName), null); | ||
|
|
||
| if (modelHash == null || modelHash.isEmpty()) { | ||
| // no model hash means no download in progress | ||
| return null; | ||
| } | ||
|
|
||
| long fileSize = | ||
| getSharedPreferences() | ||
| .getLong(String.format(DOWNLOADING_MODEL_SIZE_PATTERN, persistenceKey, modelName), 0); | ||
| long fileSize = dataStore.getSync(downloadingModelSizeKey(modelName), 0L); | ||
|
|
||
| long id = | ||
| getSharedPreferences() | ||
| .getLong(String.format(DOWNLOADING_MODEL_ID_PATTERN, persistenceKey, modelName), 0); | ||
| long id = dataStore.getSync(downloadingModelIdKey(modelName), 0L); |
There was a problem hiding this comment.
Calling dataStore.getSync(...) multiple times sequentially performs multiple blocking flow collections, which is inefficient. Furthermore, because these reads are not atomic, a concurrent write could modify the preferences between reads, leading to an inconsistent CustomModel state.
We can retrieve all preferences in a single consistent snapshot using dataStore.getAllSync() and extract the values from the map.
| public CustomModel getDownloadingCustomModelDetails(@NonNull String modelName) { | |
| String modelHash = dataStore.getSync(downloadingModelHashKey(modelName), null); | |
| if (modelHash == null || modelHash.isEmpty()) { | |
| // no model hash means no download in progress | |
| return null; | |
| } | |
| long fileSize = | |
| getSharedPreferences() | |
| .getLong(String.format(DOWNLOADING_MODEL_SIZE_PATTERN, persistenceKey, modelName), 0); | |
| long fileSize = dataStore.getSync(downloadingModelSizeKey(modelName), 0L); | |
| long id = | |
| getSharedPreferences() | |
| .getLong(String.format(DOWNLOADING_MODEL_ID_PATTERN, persistenceKey, modelName), 0); | |
| long id = dataStore.getSync(downloadingModelIdKey(modelName), 0L); | |
| public CustomModel getDownloadingCustomModelDetails(@NonNull String modelName) { | |
| Map<Preferences.Key<?>, Object> allPrefs = dataStore.getAllSync(); | |
| String modelHash = (String) allPrefs.get(downloadingModelHashKey(modelName)); | |
| if (modelHash == null || modelHash.isEmpty()) { | |
| // no model hash means no download in progress | |
| return null; | |
| } | |
| long fileSize = (Long) allPrefs.getOrDefault(downloadingModelSizeKey(modelName), 0L); | |
| long id = (Long) allPrefs.getOrDefault(downloadingModelIdKey(modelName), 0L); |
| public Set<CustomModel> listDownloadedModels() { | ||
| Set<CustomModel> customModels = new HashSet<>(); | ||
| Set<String> keySet = getSharedPreferences().getAll().keySet(); | ||
| Set<Preferences.Key<?>> keySet = dataStore.getAllSync().keySet(); | ||
|
|
||
| for (String key : keySet) { | ||
| for (Preferences.Key<?> key : keySet) { | ||
| String keyName = key.getName(); | ||
| // if a local file path is present - get model details. | ||
| Matcher matcher = Pattern.compile(LOCAL_MODEL_FILE_PATH_MATCHER).matcher(key); | ||
| Matcher matcher = Pattern.compile(LOCAL_MODEL_FILE_PATH_MATCHER).matcher(keyName); |
There was a problem hiding this comment.
Compiling the Pattern inside the loop for every key is inefficient. Additionally, calling getCustomModelDetails(modelName) inside the loop performs multiple sequential blocking reads (dataStore.getSync(...)) for each model. For
We can optimize this by compiling the pattern once outside the loop, and retrieving all preferences in a single consistent snapshot using dataStore.getAllSync().
public Set<CustomModel> listDownloadedModels() {
Set<CustomModel> customModels = new HashSet<>();
Map<Preferences.Key<?>, Object> allPrefs = dataStore.getAllSync();
Pattern pathPattern = Pattern.compile(LOCAL_MODEL_FILE_PATH_MATCHER);
for (Preferences.Key<?> key : allPrefs.keySet()) {
String keyName = key.getName();
// if a local file path is present - get model details.
Matcher matcher = pathPattern.matcher(keyName);| public boolean getCustomModelStatsCollectionFlag() { | ||
| if (dataStore.contains(eventLoggingEnabledKey())) { | ||
| return dataStore.getSync(eventLoggingEnabledKey(), true); | ||
| } |
There was a problem hiding this comment.
Calling dataStore.contains(...) followed by dataStore.getSync(...) results in two sequential blocking reads. We can optimize this to a single read by calling getSync with a default value of null and checking if the result is non-null.
| public boolean getCustomModelStatsCollectionFlag() { | |
| if (dataStore.contains(eventLoggingEnabledKey())) { | |
| return dataStore.getSync(eventLoggingEnabledKey(), true); | |
| } | |
| public boolean getCustomModelStatsCollectionFlag() { | |
| Boolean enabled = dataStore.getSync(eventLoggingEnabledKey(), null); | |
| if (enabled != null) { | |
| return enabled; | |
| } |
Following previous PRs, updates ML Model Downloader's Shared Preference utility class to use DataStore instead. This includes removing synchronized on a lot of methods since this is handled by our DataStore wrapper innately. Additionally, added helper methods for several keys with duplicate code.