chore: Replace view descriptors set with view descriptors map #8324
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| 'use strict'; | ||
| import type { SharedValue, StyleUpdaterContainer } from './commonTypes'; | ||
| import { makeMutable } from './core'; | ||
| import type { Descriptor, ViewTag } from './hook/commonTypes'; | ||
|
|
||
| export type ShareableViewDescriptors = SharedValue< | ||
| ReadonlyMap<ViewTag, Descriptor> | ||
| > & { | ||
| toArray: () => Descriptor[]; | ||
| }; | ||
|
|
||
| export interface ViewDescriptorsMap { | ||
| shareableViewDescriptors: ShareableViewDescriptors; | ||
|
|
||
| /** | ||
| * Adds a new view descriptor to the set | ||
| * | ||
| * @param item - The descriptor to add | ||
| * @param updaterContainer - Optional container with style updater function | ||
| */ | ||
| add: (item: Descriptor, updaterContainer?: StyleUpdaterContainer) => void; | ||
|
|
||
| /** | ||
| * Removes a view descriptor from the set by its tag | ||
| * | ||
| * @param viewTag - The tag of the view descriptor to remove | ||
| */ | ||
| remove: (viewTag: ViewTag) => void; | ||
| } | ||
|
|
||
| export function makeViewDescriptorsMap(): ViewDescriptorsMap { | ||
| const sharedDescriptors = makeMutable<Map<ViewTag, Descriptor>>(new Map()); | ||
| const cachedArray = makeMutable<Descriptor[] | null>(null); | ||
|
There was a problem hiding this comment. I'm curious about the performance of this new solution with a cacheable array. Usually, the view descriptors container doesn't contain more than 3 elements (mostly just one) So, finding the index with linear complexity isn't too bad in this context. There was a problem hiding this comment. What if we have a FlatList with hundreds of elements and each of them share the same animated style? It would register/unregister the style quite often and doing it in There was a problem hiding this comment. I agree, it could be problematic in this cases, but it's not the primary use case. I just want to ensure that this doesn't introduce a performance regression when we have many components, each with its own style (not shared). There was a problem hiding this comment. Ok, I see. The map is used only during renders, so the performance difference might be noticeable only during renders, not on every animations frame, so I think it is not that bad. Animations still use the cached array, which is invalidated only when new animated styles are added or old are removed (very rare cases). Wrapping up, it shouldn't affect the performance of animations but may affect the performance of renders if we have multiple views with separate animated styles each (still I feel that this performance loss shouldn't be that much noticeable but I haven't measured the impact). There was a problem hiding this comment. There's another case to consider. Let's imagine a long FlatList where every component uses the same animated style. The animation is playing (for example skeleton animation). As we scroll down, we'll create new animated components, and each time we have to recreate the whole array. Maybe we can replace the recreation of an array with simply pushing new descriptor to it? There was a problem hiding this comment. I think that it usually works in such a way that new elements are rendered but old ones are removed at the same time when we scroll the list. In this case, pushing won't give us any benefit as we won't be able to perform a fast removal of elements, so the array would have to still be re-created. The old implementation re-created it on each If we consider only additions, then yes, we could optimize it a bit by pushing new descriptors to the array but I think it is a very rare case to have more elements added without old ones being removed. |
||
|
|
||
| return { | ||
| shareableViewDescriptors: { | ||
| ...(sharedDescriptors as SharedValue<ReadonlyMap<ViewTag, Descriptor>>), | ||
|
There was a problem hiding this comment. Why do you spread There was a problem hiding this comment. Because I want to add an additional field to it - the And, since the Mutable is a JS object, extending it in this way seems to be fine. There was a problem hiding this comment. I don't think it's safe. Mutables use the reference from There was a problem hiding this comment. Both implementations of I can change it to There was a problem hiding this comment. We call There was a problem hiding this comment. I agree with Tomek, it is safer to avoid a spread operator in that case. |
||
| toArray: () => { | ||
| 'worklet'; | ||
| if (!cachedArray.value) { | ||
| cachedArray.value = Array.from(sharedDescriptors.value.values()); | ||
| } | ||
| return cachedArray.value; | ||
|
Comment on lines
+40
to
+43
There was a problem hiding this comment. Not sure if I should use the |
||
| }, | ||
| }, | ||
|
|
||
| add: (item: Descriptor, updaterContainer?: StyleUpdaterContainer) => { | ||
| const updater = updaterContainer?.current; | ||
| sharedDescriptors.modify((descriptors) => { | ||
| 'worklet'; | ||
| descriptors.set(item.tag, item); | ||
| cachedArray.value = null; | ||
|
There was a problem hiding this comment. Why we can just push to an array instead of create new one? |
||
| updater?.(true); | ||
| return descriptors; | ||
| }, false); | ||
| }, | ||
|
|
||
| remove: (viewTag: ViewTag) => { | ||
| sharedDescriptors.modify((descriptors) => { | ||
| 'worklet'; | ||
| descriptors.delete(viewTag); | ||
| cachedArray.value = null; | ||
| return descriptors; | ||
| }, false); | ||
| }, | ||
| }; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changed when I installed pods. Not sure if it'd better to add these changes in a separate PR instead.