-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Add unregister feature to ReactNativePropRegistry #11188
Conversation
This allows objects registered with ReactNativePropRegistry to be later unregistered to free up memory, so StyleSheet can support "semi-static" styles that are mostly static but have situations they become known to be unused. It is still possible for this to work should native code starts caching props from the registry. In that case ReactNativePropRegistry.unregister would just keep a temporary blacklist of unregistered ids that would be passed to native code and the blacklist would be cleared as native removes things from its cache. The motivation for this and rationale for "semi-static" props is explained in facebook/react-native/issues/15899
I think we'll want to go in the opposite direction and instead just have StyleSheet.create return an object. So when it gets garbage collected it is freed. I doubt we will actually move to a model where these are cached on the native side since it is really hard to do diffing that way. But even if we did we could use the object reference as the key and clean it up with a finalizer (React will likely rely on finalizers anyway). Want to open a pr for turning them into objects? |
@sebmarkbage What kind of object are you thinking of? A pass thru of the registered object, an new empty object, a Symbol, or some sort of I presume if it's one of the last three |
I was thinking just passing through the original. This can be problematic if we ever want to attach a finalizer to it since it would have to create a custom object for that. So if we do that then this will be another subtle change. Symbol + WeakMap would work but I think we still support JSC engines that don't support it. Seems like a simple pass through is the simplest and most efficient for now. |
If you don't want symbol then dummy/custom class would work. The dummy class would just contain an integer id and a toString. The id would not be used as a reference, just used so that in dev mode we still have a reference (or we could use the original style in a _var). And the class can be used to validate input to getByID. Then that can be the id for the WeakMap and we discourage anyone from trying to do |
Closing as there wasn't a consensus. |
This allows objects registered with ReactNativePropRegistry to be later unregistered to free up memory, so StyleSheet can support "semi-static" styles that are mostly static but have situations they become known to be unused.
It is still possible for this to work should native code starts caching props from the registry. In that case ReactNativePropRegistry.unregister would just keep a temporary blacklist of unregistered ids that would be passed to native code and the blacklist would be cleared as native removes things from its cache.
The motivation for this and rationale for "semi-static" props is explained in facebook/react-native/issues/15899