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

Improving merge strategy (3х performance increase, and 5x memory allocate decrease) #346

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

weglov
Copy link
Contributor

@weglov weglov commented Oct 28, 2021

Hi all, let me introduce my idea and improvement of the current merge logic.

Reason:
I noticed in my application had a performance problem and memory leaks. It was because the merge logic was doing double work, long story short on each iteration of the loop, we created a model in mst and then converted the newly created model into the graphql response structure.
For example:
We requested 100 entities, and we got this as an array response:

   entites: [{entity1}, {entity2}, {entity3}, ..., etc]

Then we would create each such entity in the mst.map structure, and from there convert it to an array.
Which is the bottleneck of mst as we have created a copy of the reactive data (array of entites in our case)

it starts to be noticeable when we have an array with 1000 entities in the response

It seems that the author of this issue also has the same problem and also works with mst-gql.

My solution

  1. I used Proxy to get the mobx-model while reading and not while writing (merge), this saves a lot of memory and is the main idea. (it's apply only for arrays, everything else computed as before.)

  2. I rewrote the recursive merge logic to avoid frequent recalculations of reactive models, and left it only for root data, which also makes sense since root data if linked to mst-tree, will be automaticle reactive.

This has been working for a few days in my project and seems to be working fine, but please test it on your projects and post here any issues, I think it can significantly increase the performance of your applications. I would be happy to get feedback.

PS: I will prepare a comparison of the two approaches later.

@weglov weglov changed the title Improving merge strategy (3х performance increase, and 10x memory allocate decrease) Improving merge strategy (3х performance increase, and 5x memory allocate decrease) Oct 29, 2021
@jesse-savary
Copy link
Member

Will take a look at this and release it as a beta if it works well 👍

@ryskin
Copy link

ryskin commented Nov 4, 2021

This will be great! Waiting for this update! Hope to see it asap )

@weglov
Copy link
Contributor Author

weglov commented Nov 6, 2021

@jesse-savary Hi, I have corrected several problems found in my project. Seems, PR is now stable. Are there any plans for merge?

@jesse-savary
Copy link
Member

Merged, apologies for the delay @weglov. Will have this published along with other fixes by the end of the week.

jesse-savary added a commit that referenced this pull request Apr 19, 2022
@jesse-savary
Copy link
Member

@weglov planned to release this as part of 0.15.0 but the changes to mergeHelper.ts are causing issues with a test. I'm not entirely sure how your new merge code works; is there any way you could take a look and see what the issue is?

@weglov
Copy link
Contributor Author

weglov commented Apr 20, 2022

@jesse-savary I'll take a look.

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

Successfully merging this pull request may close these issues.

3 participants