perf: fix Android AAB cleanup and mapping cache - #653
Conversation
Size Analysis1 component analyzed, 1 component processing iOS Builds
Android Builds
|
📲 Install BuildsiOS
Android
|
jamieQ
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| dex_mapping_file = dex_mapping_files[0] | ||
| with open(dex_mapping_file, "rb") as f: | ||
| dex_mapping_buffer = f.read() | ||
| return DexMapping(dex_mapping_buffer) | ||
| self._dex_mapping = DexMapping(dex_mapping_buffer) | ||
| return self._dex_mapping | ||
|
|
||
| @sentry_sdk.trace | ||
| def get_app_icon(self) -> bytes | None: |
There was a problem hiding this comment.
Bug: The caching in get_dex_mapping is incomplete. When no proguard.map file is found, the None result is not stored, causing repeated expensive filesystem searches on subsequent calls.
Severity: MEDIUM
Suggested Fix
To ensure the None result is cached, use a sentinel value. Initialize self._dex_mapping to a unique sentinel object. In get_dex_mapping, if no mapping file is found, explicitly set self._dex_mapping = None before returning. This ensures the filesystem search runs only once, and subsequent calls will immediately return the cached None value.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/launchpad/artifacts/android/aab.py#L149-L156
Potential issue: The `get_dex_mapping` method is designed to cache the result of a
filesystem search for a `proguard.map` file. However, when no mapping file is found, the
method returns `None` but fails to update the `self._dex_mapping` instance variable. As
a result, if the method is called multiple times, such as within the `get_primary_apks`
loop, the expensive `rglob` filesystem search is re-executed on every call. This defeats
the purpose of caching for the common scenario where an AAB does not contain a Proguard
mapping file, leading to unnecessary performance degradation.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
I don't yet have any definitive empirical evidence the filesystem searches are particularly expensive. This might be a reasonable thing to do, but this change was intentionally kept pretty minimal.
2 participants
The
AABclass contained a_dex_mappingfield that was presumably intended to cache the results of computing the dex mapping. However, since it was never written to, when there were multiple APKs, the mapping would be re-computed for each one. This appears to in some cases waste a lot of time (e.g. see this case where we spent ~30s computing the mapping once, then did it 5 more times).Also fixed an independent closure capture bug with APK cleanup logic.