Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache local images into memory #1279

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

christophpurrer
Copy link

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

This introduces a cache for local image on macOS. There are no predefined limits set here (at least yet) since presumably we are loading a finite number of resources and we have the benefit of virtual memory swap on desktop so we don't really need to sweat a few extra megabytes of in-memory cache for images we know we'll need to repeatedly load.

Local images don't use the shared RN image cache, and on macOS there's no automatic in-memory cache for local resources (other than the OS-level disk cache) so the RCTImageFromLocalBundleAssetURL function will synchronously access the disk on the main thread many times when switching throw an app's UI or when reloading the app.

Changelog

[macOS] [Added] - Cache local images into memory

Added logging to confirmed cache works as expected in rn-tester, and instrumentation also confirms it.

localImageCache.mov

@christophpurrer christophpurrer requested a review from a team as a code owner July 22, 2022 22:50
This introduces a cache for local image on macOS. There are no predefined limits set here (at least yet) since presumably we are loading a finite number of resources and we have the benefit of virtual memory swap on desktop so we don't really need to sweat a few extra megabytes of in-memory cache for images we know we'll need to repeatedly load.

Local images don't use the shared RN image cache, and on macOS there's no automatic in-memory cache for local resources (other than the OS-level disk cache) so the `RCTImageFromLocalBundleAssetURL` function will synchronously access the disk on the main thread many times over during thread switching in Messenger Desktop.

Added logging to confirmed cache works as expected in rn-tester, and instrumentation also confirms it.
React/Base/RCTUtils.m Show resolved Hide resolved
@Saadnajmi
Copy link
Collaborator

My general question with this change (and other related changes) is:

  • Is this something RN on iOS is already doing, and the change brings parity to the macOS implementation?

or

  • Is this something extra we're adding to the macOS implementation?

The former is easy to merge and justify, since presumably the React Native core team thought through the performance implications of caches / whatever change is being introduced. If it's the latter, I think we might have to think harder about upstreaming it and it's implications across other products.

@christophpurrer
Copy link
Author

@Saadnajmi > It is nothing iOS is doing atm and I don't have all the insights to say whether the would benefit from it or not.

We added this macOS only change to Messenger Desktop as we are loading quite a few assets from the application's mainBundle and this was one of the I/O accesses which profiling with Instruments marked as a potential performance bottleneck.

What's your recommendation with this change? Change and upstream or abandon and keep only in our internal repo?

@Saadnajmi
Copy link
Collaborator

Could we use the cache referenced in #1272 instead of making a new one?

@christophpurrer
Copy link
Author

That would be ideal. It will require some refactoring.
I'll explore and share the results here (I hope we don't have to move too many files/imports around)

@ghost ghost removed the Needs: Author Feedback label Aug 9, 2022
@christophpurrer christophpurrer changed the title Cache local images into memory Cache local images into memory [WIP] Aug 9, 2022
@christophpurrer christophpurrer changed the title Cache local images into memory [WIP] Cache local images into memory Aug 10, 2022
@christophpurrer
Copy link
Author

@Saadnajmi > I don't think there is a clear/easy/legit path to share the cache implementation with RCTImageCache.
Its API is just too different
https://github.com/microsoft/react-native-macos/blob/main/Libraries/Image/RCTImageCache.h
and we don't have an (easy) way to get the actual image size here.

I still think this change is legit, I just added some tracing to Messenger Desktop and it seems we are loading assets from the application quite a lot > https://gist.github.com/christophpurrer/ec00f0a4daf68a6328cf61a6d40b49b5

W/o this in-memory cache we would always make a blocking I/O operation ....

@Saadnajmi Saadnajmi added the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Aug 10, 2022
@Saadnajmi Saadnajmi merged commit 2190b3b into microsoft:main Aug 10, 2022
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
This introduces a cache for local image on macOS. There are no predefined limits set here (at least yet) since presumably we are loading a finite number of resources and we have the benefit of virtual memory swap on desktop so we don't really need to sweat a few extra megabytes of in-memory cache for images we know we'll need to repeatedly load.

Local images don't use the shared RN image cache, and on macOS there's no automatic in-memory cache for local resources (other than the OS-level disk cache) so the `RCTImageFromLocalBundleAssetURL` function will synchronously access the disk on the main thread many times over during thread switching in Messenger Desktop.

Added logging to confirmed cache works as expected in rn-tester, and instrumentation also confirms it.

Co-authored-by: Scott Kyle <skyle@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) Partner: Facebook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants