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.

Sign up for GitHub

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

Hold weak reference only #3157

Closed
wants to merge 4 commits into from
Closed

Hold weak reference only #3157

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b246fa9
Hold weak refference only
piaskowyk Apr 8, 2022
a2acad6
Save only exitin
piaskowyk Apr 9, 2022
e8391f8
Exiting animation registry
piaskowyk Apr 11, 2022
fc47ffe
Improvements after review
piaskowyk Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 36 additions & 23 deletions ios/LayoutReanimation/REAAnimationsManager.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ @implementation REAAnimationsManager {
RCTUIManager *_uiManager;
REAUIManager *_reaUiManager;
NSMutableDictionary<NSNumber *, NSNumber *> *_states;
NSMutableDictionary<NSNumber *, UIView *> *_viewForTag;
NSMutableSet<NSNumber *> *_toRemove;
NSMutableDictionary<NSNumber *, UIView *> *_viewsToRemove;
NSMutableDictionary<NSNumber *, UIView *> *_viewsWithExitingAnimation;
NSMutableArray<NSString *> *_targetKeys;
NSMutableArray<NSString *> *_currentKeys;
BOOL _cleaningScheduled;
Expand All @@ -45,8 +45,8 @@ - (instancetype)initWithUIManager:(RCTUIManager *)uiManager
_uiManager = uiManager;
_reaUiManager = (REAUIManager *)uiManager;
_states = [NSMutableDictionary new];
_viewForTag = [NSMutableDictionary new];
_toRemove = [NSMutableSet new];
_viewsToRemove = [NSMutableDictionary new];
_viewsWithExitingAnimation = [NSMutableDictionary new];
_cleaningScheduled = false;

_targetKeys = [NSMutableArray new];
Expand All @@ -65,8 +65,8 @@ - (void)invalidate
_removeConfigForTag = nil;
_uiManager = nil;
_states = nil;
_viewForTag = nil;
_toRemove = nil;
_viewsToRemove = nil;
_viewsWithExitingAnimation = nil;
_cleaningScheduled = false;
_targetKeys = nil;
_currentKeys = nil;
Expand Down Expand Up @@ -119,47 +119,56 @@ - (void)findRoot:(UIView *)view roots:(NSMutableSet<NSNumber *> *)roots
}
}

- (BOOL)dfs:(UIView *)root view:(UIView *)view cands:(NSMutableSet<NSNumber *> *)cands
- (BOOL)dfs:(UIView *)root view:(UIView *)view
{
NSNumber *tag = view.reactTag;
if (tag == nil) {
return true;
}
if (![cands containsObject:tag] && _states[tag] != nil) {
if (_viewsToRemove[tag] == nil && _states[tag] != nil) {
return true;
}
BOOL cannotStripe = false;
NSArray<UIView *> *toRemoveCopy = [view.reactSubviews copy];
for (UIView *child in toRemoveCopy) {
if (![view isKindOfClass:[RCTTextView class]]) {
cannotStripe |= [self dfs:root view:child cands:cands];
cannotStripe |= [self dfs:root view:child];
}
}
if (!cannotStripe) {
if (view.reactSuperview != nil) {
[_reaUiManager unregisterView:view];
}
[_states removeObjectForKey:tag];
[_viewForTag removeObjectForKey:tag];
[_toRemove removeObject:tag];
[_viewsToRemove removeObjectForKey:tag];
[_viewsWithExitingAnimation removeObjectForKey:tag];
Copy link
Member

Choose a reason for hiding this comment

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

It still seem like there are cases where views can be kept in those dicts. Specifically, it appears like the only codepath that leads to the removal is when the given view is returned from findRoot. But findRoot can in some cases return the parent of a given view and not the view itself. It'd feel safer if we have a cleaner contract as to when given view needs to be removed from an array. Also it'd be good to add a few lines of documentation for each of these two new dicts. I can understand the purpose of viewWithExitingAnimations fine by its name but not so much with viewToRemove – i.e. I don't know why we need to keep track of all views we remove.

As to "a cleaner contract" an example would be to always remove object from viewWithExitingAnimations after animation is finished (have a callback of some sort ?). With viewsToRemove we should ideally have something similar. Now it is very difficult to track the codepath that leads to deletion and it can be altered to easily.

Copy link
Contributor

@hirbod hirbod Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@mateioprea is that maybe your case, when calling reset on navigator, which leaves views mounted even after applying this fix?

Copy link

Choose a reason for hiding this comment

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

yes, in the perf monitor i can see that the total number of rendered view increase over time.
My app's lifecycle is:
Mount 1st screen -> navigate to 2nd screen -> navigate to 3rd screen -> navigate to last screen -> Reset navigation to 1st screen and then start again the same cycle.

Previously I've seen custom UI views not being unmounted but this PR fixes that issue. But i can see an increase of total number of rendered views with each new session.

}
return cannotStripe;
}

- (UIView *)findView:(NSNumber *)tag
{
UIView *view = _viewsToRemove[tag];
if (view != nil) {
return view;
}
view = [_reaUiManager viewForReactTag:tag];
if (view != nil) {
return view;
}
return _viewsWithExitingAnimation[tag];
}

- (void)removeLeftovers
{
NSMutableSet<NSNumber *> *roots = [NSMutableSet new];
for (NSNumber *viewTag in _toRemove) {
UIView *view = _viewForTag[viewTag];
if (view == nil) {
view = [_reaUiManager viewForReactTag:viewTag];
_viewForTag[viewTag] = view;
}
for (NSNumber *key in _viewsToRemove) {
UIView *view = _viewsToRemove[key];
[self findRoot:view roots:roots];
}
for (NSNumber *viewTag in roots) {
UIView *view = _viewForTag[viewTag];
[self dfs:view view:view cands:_toRemove];
UIView *view = [self findView:viewTag];
[self dfs:view view:view];
}
}

Expand All @@ -173,7 +182,10 @@ - (void)notifyAboutEnd:(NSNumber *)tag cancelled:(BOOL)cancelled
if (state == Disappearing) {
_states[tag] = [NSNumber numberWithInt:ToRemove];
if (tag != nil) {
[_toRemove addObject:tag];
UIView *view = [self findView:tag];
if (view != nil) {
[_viewsToRemove setObject:view forKey:tag];
}
}
[self scheduleCleaning];
}
Expand All @@ -192,7 +204,8 @@ - (void)notifyAboutProgress:(NSDictionary *)newStyle tag:(NSNumber *)tag

NSMutableDictionary *dataComponenetsByName = [_uiManager valueForKey:@"_componentDataByName"];
RCTComponentData *componentData = dataComponenetsByName[@"RCTView"];
[self setNewProps:[newStyle mutableCopy] forView:_viewForTag[tag] withComponentData:componentData];
UIView *view = [self findView:tag];
[self setNewProps:[newStyle mutableCopy] forView:view withComponentData:componentData];
}

- (double)getDoubleOrZero:(NSNumber *)number
Expand Down Expand Up @@ -299,12 +312,13 @@ - (void)onViewRemoval:(UIView *)view before:(REASnapshot *)before
if (state == Inactive) {
if (startValues != nil) {
_states[tag] = [NSNumber numberWithInt:ToRemove];
[_toRemove addObject:tag];
[_viewsToRemove setObject:view forKey:tag];
[self scheduleCleaning];
}
return;
}
_states[tag] = [NSNumber numberWithInt:Disappearing];
[_viewsWithExitingAnimation setObject:view forKey:tag];
NSDictionary *preparedValues = [self prepareDataForAnimatingWorklet:startValues frameConfig:ExitingFrame];
_startAnimationForTag(tag, @"exiting", preparedValues, @(0));
}
Expand All @@ -315,7 +329,6 @@ - (void)onViewCreate:(UIView *)view after:(REASnapshot *)after
NSNumber *tag = view.reactTag;
if (_states[tag] == nil) {
_states[tag] = [NSNumber numberWithInt:Inactive];
_viewForTag[tag] = view;
}
NSMutableDictionary *targetValues = after.values;
ViewState state = [_states[tag] intValue];
Expand Down