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

Hold weak reference only #3157

Closed
wants to merge 4 commits into from
Closed

Conversation

piaskowyk
Copy link
Member

@piaskowyk piaskowyk commented Apr 8, 2022

Description

We held in the _viewForTag registry strong references for views even without exiting animation. When an object hasn't had exiting animation they weren't removed from this registry. In effect, the object was unmounted from the tree of views but still was in memory.

Fixes #3124

Test

code
import Animated, {
  FadeIn,
  FadeOut
} from 'react-native-reanimated';
import { View, Button } from 'react-native';
import React, { useState } from 'react';

const style = {
  width: 100, height: 50, backgroundColor: 'black', margin: 10
}
export default function MountingUnmountingTest() {
  const [toogle, setToggle] = useState(false);

  return (
    <View
      style={{
        flex: 1,
        flexDirection: 'column',
        marginTop: 60
      }}>
      <Button title="toggle" onPress={() => setToggle(!toogle)} />
      { 
        toogle && 
        <Animated.View 
          entering={FadeIn}
          exiting={FadeOut}
          style={style} 
        /> 
      }
      { 
        toogle && 
        <View>
          <Animated.View 
            entering={FadeIn}
            exiting={FadeOut}
            style={style} 
          /> 
        </View>
      }
      { 
        toogle && 
        <Animated.View
          entering={FadeIn}
          exiting={FadeOut}
        >
          <Animated.View
            style={style} 
          /> 
        </Animated.View>
      }
      { 
        toogle && 
        <Animated.View
          entering={FadeIn}
          exiting={FadeOut}
        >
          <Animated.View 
            entering={FadeIn}
            exiting={FadeOut}
            style={style} 
          /> 
        </Animated.View>
      }
      { 
        toogle && 
        <Animated.View
          entering={FadeIn}
          exiting={FadeOut.duration(2000)}
        >
          <Animated.View 
            entering={FadeIn}
            exiting={FadeOut}
            style={style} 
          /> 
        </Animated.View>
      }
      { 
        toogle && 
        <Animated.View
          entering={FadeIn}
          exiting={FadeOut}
        >
          <Animated.View 
            entering={FadeIn}
            exiting={FadeOut.duration(2000)}
            style={style} 
          /> 
        </Animated.View>
      }
    </View>
  );
}

@hirbod
Copy link
Contributor

hirbod commented Apr 8, 2022

@piaskowyk ping me whenever you think this is ready for test if you need my feedback here.

@piaskowyk piaskowyk self-assigned this Apr 9, 2022
@piaskowyk piaskowyk requested a review from tomekzaw April 11, 2022 10:28
@piaskowyk piaskowyk marked this pull request as ready for review April 11, 2022 10:39
@piaskowyk piaskowyk requested a review from kmagiera April 11, 2022 11:01
@piaskowyk
Copy link
Member Author

@hirbod It's ready for testing, let me know if you have some time to check if it works.

@hirbod
Copy link
Contributor

hirbod commented Apr 11, 2022

@piaskowyk thanks, I will test this today, EOD

[_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

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?

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.

@hirbod
Copy link
Contributor

hirbod commented Apr 12, 2022

@piaskowyk I haven't tested the PR yet since there have been requests for changes by @kmagiera, so I will wait until they are sorted out. I got a confirmation though, that it did fix most of the issues for @mateioprea but not all
#3157 (comment)

@LeviWilliams
Copy link

Any eta of when this could be patched in? Specifically including the changes requested by @kmagiera. Really appreciate all the work here as this is something we're fully blocked by. Thank you!

@hirbod
Copy link
Contributor

hirbod commented Apr 13, 2022

Any eta of when this could be patched in? Specifically including the changes requested by @kmagiera. Really appreciate all the work here as this is something we're fully blocked by. Thank you!

I am pretty sure soon, since Expo SDK 45 is around the corner.

@LeviWilliams
Copy link

Forgive me for the bump, @piaskowyk any progress here? Thanks again for all the help

@davidskaarup
Copy link

Also following this, we had to remove all LayoutAnimation from our project, so would be really cool to get the fix out.

@mateioprea
Copy link

^^ you can always use patch package to get the fix asap into your project. I did that and it's live since last week :)

@LeviWilliams
Copy link

@mateioprea I'm still worried with your comment regarding the increased number of rendered views even including this fix, have you not had any issues with this in prod?? We have a lot of navigation resets in our app and increasing the rendered views over time will greatly affect our performance

@mateioprea
Copy link

Yup, but memory management will release those views eventually. don't worry

@AlexanderEggers
Copy link
Contributor

@piaskowyk What is your plan to unblock this PR?

@hirbod
Copy link
Contributor

hirbod commented Apr 25, 2022

It looks like they've decided to rewrite LayoutAnimation instead:
https://github.com/software-mansion/react-native-reanimated/commits/layoutanim_rewrite

0fa7649

@piaskowyk
Copy link
Member Author

I can confirm that we rewriting the layout animation code and this issue will be fixed with these changes.

@catalinmiron
Copy link
Contributor

@piaskowyk @kmagiera is the new approach going to support reparenting?

@ziyoshams
Copy link

Do you guys have an ETA for when this fix will make to the release pipeline?

@LeviWilliams
Copy link

Are layout animations still being rewritten?

@hirbod
Copy link
Contributor

hirbod commented Jun 23, 2022

No commits since 4th of May unfortunately

@piaskowyk
Copy link
Member Author

I am closing this PR because we decided to rewrite the layout animation, and this issue will be resolve here #3332

@piaskowyk piaskowyk closed this Jun 29, 2022
@hkar19
Copy link

hkar19 commented Aug 2, 2022

i made this into a patch if anyone seeking:

but it this didn't solve my problem which might be similar to #3286

react-native-reanimated+2.9.1.patch

--- a/node_modules/react-native-reanimated/ios/LayoutReanimation/REAAnimationsManager.m
+++ b/node_modules/react-native-reanimated/ios/LayoutReanimation/REAAnimationsManager.m
@@ -22,8 +22,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   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;
@@ -45,8 +45,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
     _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];
@@ -65,8 +65,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   _removeConfigForTag = nil;
   _uiManager = nil;
   _states = nil;
-  _viewForTag = nil;
-  _toRemove = nil;
+  _viewsToRemove = nil;
+  _viewsWithExitingAnimation = nil;
   _cleaningScheduled = false;
   _targetKeys = nil;
   _currentKeys = nil;
@@ -119,20 +119,20 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   }
 }
 
-- (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) {
@@ -140,26 +140,35 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
       [_reaUiManager unregisterView:view];
     }
     [_states removeObjectForKey:tag];
-    [_viewForTag removeObjectForKey:tag];
-    [_toRemove removeObject:tag];
+    [_viewsToRemove removeObjectForKey:tag];
+    [_viewsWithExitingAnimation removeObjectForKey:tag];
   }
   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];
   }
 }
 
@@ -173,7 +182,10 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
     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];
     }
@@ -192,7 +204,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
 
   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
@@ -299,12 +312,13 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   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));
 }
@@ -315,7 +329,6 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   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];

@hkar19
Copy link

hkar19 commented Aug 2, 2022

interestingly, after i combined this #3157 with #3298, i solved my problem which is similar to #3286.

this is my patch: react-native-reanimated+2.9.1.patch

you can use patch-package to apply this patch

--- a/node_modules/react-native-reanimated/ios/LayoutReanimation/REAAnimationsManager.m
+++ b/node_modules/react-native-reanimated/ios/LayoutReanimation/REAAnimationsManager.m
@@ -22,8 +22,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   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;
@@ -45,8 +45,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
     _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];
@@ -65,8 +65,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   _removeConfigForTag = nil;
   _uiManager = nil;
   _states = nil;
-  _viewForTag = nil;
-  _toRemove = nil;
+  _viewsToRemove = nil;
+  _viewsWithExitingAnimation = nil;
   _cleaningScheduled = false;
   _targetKeys = nil;
   _currentKeys = nil;
@@ -119,20 +119,20 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   }
 }
 
-- (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) {
@@ -140,26 +140,35 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
       [_reaUiManager unregisterView:view];
     }
     [_states removeObjectForKey:tag];
-    [_viewForTag removeObjectForKey:tag];
-    [_toRemove removeObject:tag];
+    [_viewsToRemove removeObjectForKey:tag];
+    [_viewsWithExitingAnimation removeObjectForKey:tag];
   }
   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];
   }
 }
 
@@ -173,7 +182,10 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
     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];
     }
@@ -192,7 +204,8 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
 
   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
@@ -299,12 +312,13 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   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));
 }
@@ -315,7 +329,6 @@ typedef NS_ENUM(NSInteger, FrameConfigType) { EnteringFrame, ExitingFrame };
   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];
diff --git a/node_modules/react-native-reanimated/ios/LayoutReanimation/REAUIManager.mm b/node_modules/react-native-reanimated/ios/LayoutReanimation/REAUIManager.mm
index 16ed327..2695d7d 100644
--- a/node_modules/react-native-reanimated/ios/LayoutReanimation/REAUIManager.mm
+++ b/node_modules/react-native-reanimated/ios/LayoutReanimation/REAUIManager.mm
@@ -117,6 +117,11 @@ std::weak_ptr<reanimated::Scheduler> _scheduler;
   // Reanimated changes /start
   if (isUIViewRegistry) {
     NSMutableDictionary<NSNumber *, id<RCTComponent>> *viewRegistry = [self valueForKey:@"_viewRegistry"];
+      NSMutableDictionary<NSNumber *, NSMutableSet<id<RCTComponent>> *> *toBeRemovedRegisterCopy =
+              [NSMutableDictionary dictionaryWithDictionary:_toBeRemovedRegister];
+          for (NSNumber *key in _toBeRemovedRegister) {
+            toBeRemovedRegisterCopy[key] = [NSMutableSet setWithSet:_toBeRemovedRegister[key]];
+          }
     for (id<RCTComponent> toRemoveChild in _toBeRemovedRegister[containerTag]) {
       NSInteger lastIndex = [container reactSubviews].count - 1;
       if (lastIndex < 0) {
@@ -129,7 +134,7 @@ std::weak_ptr<reanimated::Scheduler> _scheduler;
       ) {
         // we don't want layout animations when removing modals or Screens of native-stack since it brings buggy
         // behavior
-        [_toBeRemovedRegister[container.reactTag] removeObject:toRemoveChild];
+        [toBeRemovedRegisterCopy[container.reactTag] removeObject:toRemoveChild];
         [permanentlyRemovedChildren removeObject:toRemoveChild];
 
       } else {
@@ -137,6 +142,7 @@ std::weak_ptr<reanimated::Scheduler> _scheduler;
         viewRegistry[toRemoveChild.reactTag] = toRemoveChild;
       }
     }
+      _toBeRemovedRegister = toBeRemovedRegisterCopy;
       
     for (UIView *removedChild in permanentlyRemovedChildren) {
       [self callAnimationForTree:removedChild parentTag:containerTag];

@piaskowyk piaskowyk deleted the @piaskowyk/hold-weak-refference-only branch April 3, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet