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

Optimize runtime initialization for native components with memoization #44243

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

admirsaheta
Copy link
Contributor

Summary:

This pull request introduces a memoization optimization in the getNativeComponentAttributes function to avoid redundant runtime initialization for native components. It introduces a memoization cache (configCache) to store previously computed configurations, which are then utilized if available before computing a new one.

Changelog:

[INTERNAL|GENERAL] [ADDED] - Introduced memoization optimization to getNativeComponentAttributes function.

Test Plan:

No changes to user interface. The code has been tested locally to ensure that previously computed configurations are utilized when available, reducing redundant computations.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 24, 2024
Copy link

Fails
🚫

📋 Verify Changelog Format - See Changelog format

Generated by 🚫 dangerJS against 9e766f5

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Apr 24, 2024
@NickGerleman
Copy link
Contributor

@dmytrorykun does this change make sense?

Copy link
Contributor

@dmytrorykun dmytrorykun left a comment

Choose a reason for hiding this comment

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

@admirsaheta it is very likely that this optimization is already happening on a lower level of the framework. E.g. the only caller of getNativeComponentAttributes already uses cache internally. Are you seeing any reduction of the number of calls to UIManager.getViewManagerConfig with your changes?
cc @NickGerleman

@admirsaheta
Copy link
Contributor Author

admirsaheta commented Apr 28, 2024

Good point @dmytrorykun, not really sure however I did fix my issue with patch-package locally in a project this way, see ref. below
image
image

@dmytrorykun
Copy link
Contributor

@admirsaheta this error means that the LEGACY_RNCViewPager native component is not exported from native. Assuming you are on the old architecture, you should have either of the two:

  1. LEGACY_RNCViewPager, a subclass of RCTViewManager, that has RCT_EXPORT_MODULE() in its implementation.
  2. Some other RCTViewManager subclass that has RCT_EXPORT_MODULE(LEGACY_RNCViewPager) in its implementation.

I don't see how extra caching can fix this. Could that be some other change?

@efstathiosntonas
Copy link

@admirsaheta you must pass useNext={false} to the pager view component, latest release of pager-view is a mess…

@admirsaheta
Copy link
Contributor Author

@admirsaheta you must pass useNext={false} to the pager view component, latest release of pager-view is a mess…

This fixes it as well! Thank you 👍
While I did observe improvements locally using this approach, your question about the reduction of calls to UIManager.getViewManagerConfig raises valid concerns about the effectiveness of this optimization in real-world scenarios, still would consider it good practice to avoid unnecassary re-inits each time it's called 👍

@dmytrorykun
Copy link
Contributor

dmytrorykun commented Apr 29, 2024

@admirsaheta

While I did observe improvements locally using this approach

Could you describe the improvements in more details?

still would consider it good practice to avoid unnecassary re-inits each time it's called

Re-inits of what exactly? Are you seeing getNativeComponentAttributes being called for the same component multiple times?

@admirsaheta
Copy link
Contributor Author

Yes!

@dmytrorykun
Copy link
Contributor

@admirsaheta could you please add more details, describe how you test this, and what is the improvement that you're seeing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants