Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Subscriptions Block UI thread #75

Closed
da314pc opened this issue Sep 22, 2021 · 12 comments
Closed

Subscriptions Block UI thread #75

da314pc opened this issue Sep 22, 2021 · 12 comments

Comments

@da314pc
Copy link

da314pc commented Sep 22, 2021

I was have been using the old repo before migrating to yours. This is an important issue worth reviewing

inProgress-team/react-native-meteor#301

The main benefit of this library is subscriptions. For large list or complex components, subscriptions can cause weird rendering problems.

Some content can be partially rendered.

I've tried the InteractionManage, not very helpful. React native is single threaded. Not sure if we can use a background process for this. Just an idea

@ToyboxZach
Copy link
Contributor

I was having the similar problems.

I ended up having to do pretty hefty changes to make sure we aren't oversubscribing and that we only get change notifications when we want. I'm not sure how much of my changes ended up being unique to my experience, but if you are still looking for a solution this is a good starting point:

https://github.com/ToyboxZach/meteor-react-native

My app got significantly better after these changes

@TheRealNate
Copy link
Collaborator

@ToyboxZach, would you be open to possibly merging your changes into this repo?

@ToyboxZach
Copy link
Contributor

I don't have the time to thoroughly validate it across all use cases or maintain it outside of times my product has a bug, but I can definitely open up a PR

@ToyboxZach
Copy link
Contributor

Also do you have resources explaining how to run the tests? "npx mocha" doesn't seem to work for me like I would expect

@da314pc
Copy link
Author

da314pc commented Jun 7, 2022

I was having the similar problems.

I ended up having to do pretty hefty changes to make sure we aren't oversubscribing and that we only get change notifications when we want. I'm not sure how much of my changes ended up being unique to my experience, but if you are still looking for a solution this is a good starting point:

https://github.com/ToyboxZach/meteor-react-native

My app got significantly better after these changes

I think react native's new fabric architecture should solve this problem.
https://medium.com/coox-tech/deep-dive-into-react-natives-new-architecture-fb67ae615ccd

From my experience, if you have a large list with lots of nested components and you try to have subscriptions like so:

const MyAppContainer = withTracker(() => {

const myTodoTasks = Todos.find({completed:false}).fetch();
const handle = Meteor.subscribe("myTodos");

return {
    myTodoTasks,
    loading:!handle.ready()
};

})(MyApp);

The UI thread will be blocked, the only work around for me was using meteor methods. But hopefully this will be solved.

@TheRealNate
Copy link
Collaborator

@jankapunkt could you provide info regarding testing?

@jankapunkt
Copy link
Member

If I have an example component that shows how to reproduce these oversubscription then I can fast test/debug/provide a fix if possible

@ToyboxZach
Copy link
Contributor

Merging in master resolved this.

For clarification on what the problem is in master:

Every time you register a callback it gets added to a generic "changed" callback, that means that every single callback needs to be called on every single change to a collection which causes a huge hold up on the UI thread.

To make it even worst the subscriptions don't get cleaned up when you resubscribe so every time a "withTracker" or "useTracker" was called you would get stacked subscriptions. This then would lead to 100 plus subscriptions for a single component after some time which leads to bigger and bigger slow downs.

My PR separates that out as much as possible and makes sure that you only have as many subscriptions as you actually need. It is a pretty hefty change and I don't personally use and local collections in my app so those are going to be pretty poorly tested, but if other people want to take my change as a jumping off point I think its a pretty good point for server only subscriptions.

@da314pc
Copy link
Author

da314pc commented Jun 7, 2022

If I have an example component that shows how to reproduce these oversubscription then I can fast test/debug/provide a fix if possible

I'll try to get an example I had into a demo repo. This was only a problem on android If I remember correctly. But give me a couple days to get the repo back up.

@ToyboxZach
Copy link
Contributor

The part that causes over subscriptions is in useTracker

const meteorDataDep = new Tracker.Dependency();

will be called every render loop. Which means you are just stacking dependencies.

My PR
https://github.com/TheRealNate/meteor-react-native/pull/93/files#diff-2633dadb453d450392035d1d503a0b4cc6122e23781a792b0af576dfa38e5707

fixes that part in useTracker.js

@da314pc
Copy link
Author

da314pc commented Jun 9, 2022

If I have an example component that shows how to reproduce these oversubscription then I can fast test/debug/provide a fix if possible

Here is the best reproduction I could get.
https://github.com/da314pc/meteor_react_perf

One file, main.js
These are the only subs that matter:
var handler1 = Meteor.subscribe('myuserFields')
var handler2 = Meteor.subscribe('SubThread', subId, messageCount);
var handler3 = Meteor.subscribe('SubThreadDescription', subId)

subthread is just a document of post, you can create a sample subscription:
the model is just in this format:

        var message = {
          _id: obj._id,
          text: obj.body,
          createdAt: obj.createdAt,
          user:{
            _id: obj.userId,
            name: obj.username,
          },
          avatar:avatar,
          image: image,
          video: video,
          attachments: attachments,
          videoImage: obj.videoPosterImage,
          reply: obj.reply,
          replyBody: obj.replyBody,
          replyattachments: obj.replyattachments,   
          replyUsername: obj.replyUsername,
          replyUserId: obj.replyUserId,
          likedByUsername: obj.likedByUsername,
          _likeCount: obj._likeCount,
          replyId: obj.replyId
        }

hope this helps

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

Closing this issue due to no activity. Feel free to reopen.

@jankapunkt jankapunkt reopened this Dec 10, 2022
jankapunkt added a commit that referenced this issue Mar 17, 2023
…y better

Merge pull request #111 from ToyboxZach/Merge

Restructure how we handle the Tracker, instead of a global listen to any change make the listens happen on specific queries and collections. Also fixes some bugs where we would generate way too many instances of subscriptions.

This also fixes the log in flow so the series of loggingIn -> LoggedIN is consistent and doesn't allow the package to ever auto log you out as that should be handled by the owning app just like in base meteor.

Every time you register a callback it gets added to a generic "changed" callback, that means that every single callback needs to be called on every single change to a collection which causes a huge hold up on the UI thread.

To make it even worst the subscriptions don't get cleaned up when you resubscribe so every time a "withTracker" or "useTracker" was called you would get stacked subscriptions. This then would lead to 100 plus subscriptions for a single component after some time which leads to bigger and bigger slow downs.

My PR separates that out as much as possible and makes sure that you only have as many subscriptions as you actually need. It is a pretty hefty change and I don't personally use and local collections in my app so those are going to be pretty poorly tested, but if other people want to take my change as a jumping off point I think its a pretty good point for server only subscriptions.

This also fixes up weirdness with the logging in and order of events where you would have an invalid user state or it would force you to logout because of bad internet

Likely fixes these erorrs:
#75
#58

and maybe
#79
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

4 participants