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

BUG: GOOGLE PIXEL(Android 13) hang on scrolling horizontal. #259

Closed
tomwotton opened this issue Sep 3, 2022 · 50 comments · Fixed by #297
Closed

BUG: GOOGLE PIXEL(Android 13) hang on scrolling horizontal. #259

tomwotton opened this issue Sep 3, 2022 · 50 comments · Fixed by #297

Comments

@tomwotton
Copy link

Describe the bug
On Android 13 in all google pixel devices. If I scroll the diary horizontally then after few seconds the app got hanging. if I move to other screens then came back to diary screen it works for few seconds and then hangs again.
I face this hanging issue on numberOfDays is set as 7 or 5. If 5 days is set then it hang after 10 seconds but id select it as 7 days then it hangs in 1 or 2 seconds.

Visual demo

screen-20220901-135507.mov

Steps To Reproduce

  1. Open the calendar in Google Pixel 6,6a on android 13
  2. Scroll the calendar horizontally in 7 days view then you face the issue.

Expected behaviour
I expect it to scroll smother like other devices.

Environment:

  • react-native-week-view: 0.16.0
  • react-native: 0.66.4
  • react:17.0.2
  • OS: android 13 on Google Pixel devices
@pdpino
Copy link
Collaborator

pdpino commented Sep 3, 2022

@Jack-Alfie thanks for the detailed description

Can you try with the latest react-native-week-view version?

@pdpino pdpino added type: bug status: waiting reply Waiting a reply from the issue's author labels Sep 3, 2022
@tomwotton
Copy link
Author

@pdpino I have updated the library version to 0.25.1 and I have updated the react native version to 0.68.2 as in your example project but I face the same issue and I have attached the video for it

screen-20221007-135916.mov

@greenboots88
Copy link

@pdpino I also have this problem, it's affecting all my users running on Android13, so Google Pixel phones and other Android devices. Is there a fix?

@pdpino pdpino removed the status: waiting reply Waiting a reply from the issue's author label Oct 7, 2022
@pdpino
Copy link
Collaborator

pdpino commented Oct 7, 2022

I'll check this out. Do u know if it occurs in iOS as well?

@greenboots88
Copy link

iOS works fine. It’s only happening in Android13 devices. Something might have added to stop the scrolling?

@jethro69er
Copy link

Also facing this issue on Androids running OS v13. v12 fine, iOS fine.

@greenboots88
Copy link

I'll check this out. Do u know if it occurs in iOS as well?
@pdpino Have you checked this? Is there a fix please??

@greenboots88
Copy link

@hoangnm @pdpino This is a major problem that will affect all users when Android13 is shortly rolled out to all devices. The issue causes the diary to lag then crash if the scrolling is done vertically or horizontally

@pdpino
Copy link
Collaborator

pdpino commented Oct 11, 2022

Hi @greenboots88, sorry for the late reply, I've been out of office the last days

I agree this is an urgent issue. I'll check it out this week

@greenboots88
Copy link

@pdpino Testing different Pixel models, they both have the same problem. The diary lags and then crashes after 8 swipes. Swiping forwards or backwards has the same problem

@pdpino
Copy link
Collaborator

pdpino commented Oct 12, 2022

(just in case u can check this)
Is the behavior the same with many events (e.g. ~100 or more) vs with few events (e.g. ~10 or less)?

@greenboots88
Copy link

@pdpino the behaviour is worse with more events. 10 events has no lag. 100+ starts lagging after 20-30 swipes. 1000+ events starts lagging 8-10 swipes

@tomwotton
Copy link
Author

@pdpino I have tried your old version of library which build with scrollview and the app works faster I didn't face hang issue for more than 1000 events. So I think the issue is ViitualizedList which stop scrolling in android 13. This may helps you to find the reason.

@pdpino
Copy link
Collaborator

pdpino commented Oct 13, 2022

@Jack-Alfie thanks, very good to know! I already reproduced this bug in android 13, and I'm trying to find the culprit

Can you confirm which version/commit did you use? The VirtualizedList was introduced after 0.0.19

@pdpino
Copy link
Collaborator

pdpino commented Oct 13, 2022

small update:

  • I'm also able to reproduce the ANR bug in android 11
    • the logs are slightly different, but it seems to be the same bug
    • I guess some android versions may take longer to throw the ANR error than other versions (--> explaining why is more observed in android 13)
  • The ANR bug culprit seems to be Event.js, that has too many useSharedValue()s
    • if all interactions and gestures are disabled --> the ANR bug disappears

@tomwotton
Copy link
Author

@pdpino Thanks for the update. I didn't remember that I have download your src in one of my old sample project. Now I tried that and it's working on that sample app. May be its a 0.0.14 version.

@tomwotton
Copy link
Author

@pdpino In our app we need to add some more design in the events and we cant use the description key to show the event name, we use different keys in our api. so we have copied your src and use it in our project and in Event.js, we didn't use useSharedValue()s but we have the Virtulizedlist in Weekview.js file and we face the same issue in your lates build too but the old build which build without Virtulizedlist is working fine with Scrollview.

@pdpino
Copy link
Collaborator

pdpino commented Oct 14, 2022

we didn't use useSharedValue()s but we have the Virtulizedlist in Weekview.js file and we face the same issue in your lates build too but the old build which build without Virtulizedlist is working fine with Scrollview.

🤔 interesting, thanks for the follow-up @Jack-Alfie

  1. Are you using react-native-reanimated at all? reanimated has some similar issues (e.g. ANR : Input dispatching timed out  software-mansion/react-native-reanimated#2812, Large Flatlist with PanGestureHandler and useAnimatedReaction - ANR software-mansion/react-native-reanimated#3397), that's why I'm suspecting from the useSharedValue()s

    • Can I see the rn-week-view code you are using?
  2. VirtualizedList or FlatList should work better than ScrollView in our case when the user has visited many pages.

    • In their docs: "ScrollView renders all its react child components at once, [...] FlatList renders items lazily, when they are about to appear, and removes items that scroll way off screen to save memory and processing time."
    • I'll try with ScrollView, and will dig more into this

@pdpino
Copy link
Collaborator

pdpino commented Oct 14, 2022

In our app we need to add some more design in the events and we cant use the description key to show the event name, we use different keys in our api

Btw, the EventComponent should cover this use case. Is this not enough for your use case?

@tomwotton
Copy link
Author

we didn't use useSharedValue()s but we have the Virtulizedlist in Weekview.js file and we face the same issue in your lates build too but the old build which build without Virtulizedlist is working fine with Scrollview.

🤔 interesting, thanks for the follow-up @Jack-Alfie

  1. Are you using react-native-reanimated at all? reanimated has some similar issues (e.g. ANR : Input dispatching timed out  software-mansion/react-native-reanimated#2812, Large Flatlist with PanGestureHandler and useAnimatedReaction - ANR software-mansion/react-native-reanimated#3397), that's why I'm suspecting from the useSharedValue()s

    • Can I see the rn-week-view code you are using?
  2. VirtualizedList or FlatList should work better than ScrollView in our case when the user has visited many pages.

    • In their docs: "ScrollView renders all its react child components at once, [...] FlatList renders items lazily, when they are about to appear, and removes items that scroll way off screen to save memory and processing time."
    • I'll try with ScrollView, and will dig more into this
  1. We didn't use the react-native-reanimated and useSharedValue() in our live app but the app hangs on android 13 on scroll. In our development app we use your latest code with react-native-reanimated and useSharedValue() and face the same issue
  2. Yes VirtualizedList or FlatList is better than Scrollview. We also using that in our app.

Kindly find the Demo code for your reference. You can test this code by creating the sample project and add this files to that.
demo code.zip

@tomwotton
Copy link
Author

tomwotton commented Oct 17, 2022

In our app we need to add some more design in the events and we cant use the description key to show the event name, we use different keys in our api

Btw, the EventComponent should cover this use case. Is this not enough for your use case?

Yes our diary design looks different. so we customised your code. We need to show 4 fields for title, start time, end time and status like this
Simulator Screen Shot - iPhone 13 - 2022-10-17 at 14 52 49

@greenboots88
Copy link

@pdpino is there any update on fixing this. If the ANR bug is happening in android 11 and 12, it is significantly worse and more noticeable in Android 13. Throwing the errors very quickly. I cannot disable all interactions and bugs because it makes your library unusable.
Do you have a solution?

@pdpino
Copy link
Collaborator

pdpino commented Oct 18, 2022

@Jack-Alfie since version >=0.0.17, you can achieve your design using EventComponent prop:

const exampleEvents = [{
  id: 1,
  startDate: new Date(...),
  endDate: new Date(...),
  color: 'red',
  title: 'my title',
  start_time: '12:00', // start_time can be provided here...

const EventComponent = ({event}) => {
  // or can be computed on the fly, e.g.
  const start_time = moment(event.startDate).format('HH:mm')
  return (
    <>
      <Text>{event.title}</Text>
      <Text>{event.start_time}</Text>
      <Text>{event.end_time}</Text>
      <Text>{status}</Text>
    </>
  );
};

// then
<WeekView
  EventComponent={EventComponent}
  eventContainerStyle={moreStyles for the container} // since version v0.0.18
/>

@tomwotton
Copy link
Author

@pdpino Thanks I will check that and let you know

@greenboots88
Copy link

@pdpino Is there an update on this? I see there is a Hotfix update which is a temporary solution, but do we have a complete fix because the Hotfix just reduces the error, and it is still laggy

@pdpino
Copy link
Collaborator

pdpino commented Oct 24, 2022

@greenboots88 I'm still debugging this. I've found some memory leaks and bottlenecks that I'm trying to solve, but I haven't been able to send an isolated solution right now. I'm doing my best to have a solution this or next week.

the Hotfix just reduces the error

So the ANR is still triggered after a while? That's very useful to know

@tomwotton
Copy link
Author

@pdpino I run your demo app with my data of more than 1000 events and I still face the hang issue after adding your hot fix.

@tomwotton
Copy link
Author

@pdpino Kindly find the video of your example project. I Have added both option but on scroll after some time app hangs.

hangingExample.mp4

@pdpino
Copy link
Collaborator

pdpino commented Oct 27, 2022

Thanks @Jack-Alfie

I'm still debugging and I'm not able to isolate the problem.

  • I've tried improving the logic involving the infinite scroll (i.e. load more pages when you reach the end)
  • I've tried migrating to hooks
  • I've tried reducing the size of <Events/> and/or <Event /> components
  • I know there is a memory leak in the VirtualizedList: if you scroll forward and back between two pages, the memory increases indefinitely. I've searched for closure and other leaks

Overall, I think each page (<Events />) is too heavy for a VirtualizedList / FlatList. I will try other options

@greenboots88
Copy link

@pdpino yes if the problem is related to the animation of the VirtualizedList which is causing the hang, another option is needed

@jethro69er
Copy link

is there an update on this? I also have problems with my customers chasing me for this to be fixed, I’ve been keeping an eye on this thread but nothing more to add than has already been posted. Update would be appreciated.

@pdpino
Copy link
Collaborator

pdpino commented Nov 3, 2022

I've been busy last weeks, so I don't have a fix yet :(

I'm aware this issue is very relevant. I'll try to make some time this week, will let you know of any updates

@jethro69er
Copy link

@pdpino thanks for your efforts. If you can find a fix for this I'd be happy to pay you a handsome reward :)

@greenboots88
Copy link

@pdpino Yes, I would be happy to pay you to get this resolved asap please. Message me on wottonearth@gmail.com to arrange a payment

@pdpino
Copy link
Collaborator

pdpino commented Nov 4, 2022

@jethro69er , @greenboots88 that'd be awesome! I'm trying to solve it this weekend. Hope to have an update soon.

@greenboots88
Copy link

I'm trying to solve it this weekend. Hope to have an update soon.
How did you get on @pdpino. I was being serious about the payment.. drop me an email ;)

@pdpino
Copy link
Collaborator

pdpino commented Nov 7, 2022

Update: I've located some mistakes and bottlenecks in the code that might be causing this, I've done some refactors and improved performance, though the ANR issue still appears at some point.

My computer crashed during the weekend and it doesn't boot right now, so I'll be a bit delayed by this 😞

@greenboots88 I sent you an email 🙂

@tomwotton
Copy link
Author

@hoangnm can you look at this please, it should be the main priority because it makes the week view library completely unusable in Android 13

@greenboots88
Copy link

@hoangnm do you have an update on this? Your library is not usable in Android13.

@greenboots88
Copy link

@pdpino @hoangnm This library does not work on Android13 is you have more than 500 entries, it is very slow and laggy.

@hoangnm
Copy link
Owner

hoangnm commented Jan 3, 2023

Hi @greenboots88 , I'm checking

@pdpino
Copy link
Collaborator

pdpino commented Jan 5, 2023

Hi, I'm also checking this now. I was unavailable some weeks

Anyone with the ANR problem on android, can you send the logcat please? adb logcat -d > logs.txt. That would be helpful

@hoangnm
Copy link
Owner

hoangnm commented Jan 15, 2023

These issues may be related to our problem:
facebook/react-native#13413
https://stackoverflow.com/questions/58243680/react-native-another-virtualizedlist-backed-container

In the old approach, we used ScrollView for horizontal scrolling had better performance because it only rendered 5 pages at a time. However, it was laggy when it scrolled to the center, so we applied VirtualizedList for better animation. Thinking about it, it's not good at all to render thousands of events, so we should filter the events to be rendered, as when we used ScrollView, maybe rendering events for most 5 pages at any time, @pdpino.

@hoangnm
Copy link
Owner

hoangnm commented Jan 23, 2023

Hi guys, I just published the version https://www.npmjs.com/package/react-native-week-view/v/0.29.0-rc.0 to fix this issue. Could you try? cc @greenboots88 @pdpino

@pdpino
Copy link
Collaborator

pdpino commented Jan 28, 2023

I tried v0.29.0-rc.0 with 1000 events and it hangs after a few swipes. Also it takes a moment to load the events for the current screen.

2023-01-28-hangs-1000events-v0.29.rc-0.mp4

@pdpino
Copy link
Collaborator

pdpino commented Feb 2, 2023

These issues may be related to our problem:
https://stackoverflow.com/questions/58243680/react-native-another-virtualizedlist-backed-container

This question is regarding nested lists with the same orientation, but we use an horizontal list inside a vertical scrollview.

Thinking about it, it's not good at all to render thousands of events, so we should filter the events to be rendered, as when we used ScrollView, maybe rendering events for most 5 pages at any time

I tried this, rendering 5 to 10 pages at the time. The app still crashes with 1000 events.


I'm trying a bunch of things now, will give an update soon.

@pdpino
Copy link
Collaborator

pdpino commented Feb 2, 2023

I tested with 5000 events, and the crash disappears if the Event component is simplified like this:

const SimpleEvent = ({
  event,
  containerStyle,
  top,
  left,
  height,
  width,
  onPress,
}) => {
  const onPressWrapper = () => onPress();

  const someValue = useSharedValue(3);
  const pressGesture = Gesture.Tap()
    .onEnd((evt, success) => {
      // using a sharedValue does not trigger the crash:
      someValue.value = 7;
      console.log('Reference to shared value: ', someValue.value);

      // but with either of these lines it crashes:
      console.log('Reference to onPress', onPress);
      console.log('Reference to wrapper', onPressWrapper);
    });

  return (
    <GestureDetector gesture={pressGesture}>
      <View
        style={[
          styles.container,
          {
            backgroundColor: event.color || DEFAULT_COLOR,
            top,
            left,
            height,
            width,
          },
          containerStyle,
          event.style,
        ]}
      >
        <Text style={styles.description}>{event.id}</Text>
      </View>
    </GestureDetector>
  );
};

(This implementation removes support for interactions: press, long press, drag, edit)

In other words, if the function inside onEnd references a JS variable, it crashes after some swipes. Referencing a shared value does not make it crash.

I think it's a bug in how rn-gesture-handler / rn-reanimated handle JS closure in worklets, and that only gets triggered with large lists (it only crashes after you scroll a few times).

@pdpino
Copy link
Collaborator

pdpino commented Feb 8, 2023

Update: I just sent a PR with a fix 😄

Setting the new prop runOnJS={true} makes the crash dissapear (or at least I'm not able to reproduce it 😁 )

Notice this sets .runOnJS(true) in RNGH, i.e. animations are run in the JS thread instead of the UI thread (read about these in rn-reanimated worklets).

Apparently this is a (really specific) bug in RNGH, I reported it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants