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

Slowness with big model #432

Open
hersentino opened this issue Mar 11, 2022 · 9 comments
Open

Slowness with big model #432

hersentino opened this issue Mar 11, 2022 · 9 comments
Labels
🍗 enhancement New feature or request

Comments

@hersentino
Copy link

hersentino commented Mar 11, 2022

Hello,

I use mobx-keystone in a project where i'm managing big amount of data that I get from api call.
The model that I receive are not mobx-keystone snapshot but the api models match my mobx-keystone models, except for the $modeltype. When I instance my mobx-keystone models from the api data, it can be very long like more than 1 second.
I've seen your fix on v0.67.0 version but in my case instantiations are still long with big models.

I've made a reproducible example here: https://github.com/hersentino/mobx-keystone-issue
In this example you can find a big amount of data (that i really get from an api).

  • Clicking to the button "instance main model" you will be able to see the time taken by the instantiation of the model.
  • I also made a button that set a primitive variable in the store, this is to show that the heavier is the store, the longer the set will be (5ms with empty store, 25ms with big store), even if it just set a primitive variable.

I change all tProp by prop in models to save type checking time.
Are I doing something wrong ?
Thanks

@xaviergonz
Copy link
Owner

is your app going to mutate the models?
if not, did you consider using frozen? https://mobx-keystone.js.org/frozen-data
if you do, did you try transforming the data into a snapshot (and then using fromSnapshot instead of creating all the models)?
or alternatively, if you use tProps you may ignore $modelType in snapshot in most cases since v0.64.0

@xaviergonz
Copy link
Owner

xaviergonz commented Mar 11, 2022

Btw, the times of time of setting are greatly reduced if you don't use the redux middleware.

With redux middleware:
time of setting in the store : 187.5 ms
time of change a primitive value in the store : 32.80000001192093 ms

Without redux middleware:
time of setting in the store : 28.69999998807907 ms
time of change a primitive value in the store : 0.699999988079071 ms

@xaviergonz
Copy link
Owner

xaviergonz commented Mar 11, 2022

or alternatively, if you use tProps you may ignore $modelType in snapshot in most cases since v0.64.0

Ok, I just tried that option and it is actually slower (1.4s vs 1.2s) due to the extra type checking involved to automatically detect the type.

@xaviergonz
Copy link
Owner

That being said I just published v0.67.1, which should make model creation around 25% faster when used with mobx 6. (In my case your example went down from 1000ms to around 750ms)

@xaviergonz xaviergonz added the 🍗 enhancement New feature or request label Mar 11, 2022
@hersentino
Copy link
Author

Thanks for the quick responses !

I've just try without the reduxtools and with the fromSnapshot, like that :

const mainModel = fromSnapshot<MainModel>(data);

end the results are much quicker !

time of instanciation with fromGrpc : 289 ms
time of setting in the store : 5.699999999953434 ms

if not, did you consider using frozen?

The problem is that I want to have models because I have modelAction and if I understand frozen are made for immutable data and the objects are not keystone models.

@xaviergonz
Copy link
Owner

xaviergonz commented Mar 11, 2022

const mainModel = fromSnapshot(data);

for that test to be accurate you'll need to either inject $modelType to your whole snapshot object tree or to make sure all model properties that reference models use a tProp and then use fromSnapshot(MainModel, data);

if you don't do that then you will just get a "simple" object without actions/etc.

The problem is that I want to have models because I have modelAction and if I understand frozen are made for immutable data and the objects are not keystone models.

Yes, that's not a good case for frozen then.

@hersentino
Copy link
Author

hersentino commented Mar 18, 2022

if you don't do that then you will just get a "simple" object without actions/etc.

yep I did try and you're right I can't access to the modelAction.

for that test to be accurate you'll need to either inject $modelType

So I did try to modify my fromGrpc functions to inject the $modelType to the data and then use fromSnapshot(data) and the time of instantiation is pretty much the same (about 800ms), you can find a reproducible example in the branch "fromSnapshot" (https://github.com/hersentino/mobx-keystone-issue/tree/fromSnapshot).

Is it expected to have similar time with fromSnapshot ?

@xaviergonz
Copy link
Owner

I don't think fromSnapshot would help much, it basically does the same thing you do manually by creating sub-models.

the best way I could think of improving the speed is to either:
a) use frozen for the stuff that doesn't need actions / is immutable
b) use data models instead of actual "class" models (they should give you around the same speed as the from snapshot without $modelType, but they are a bit trickier to use)

@hersentino
Copy link
Author

I'm trying the b solution and I start to have some good result, but i'm struggling to create reference with DataModel, can you confirm that is possible ?

Also I'm a chrome user so didn't notice yet but the example that I provide upside is about 2 times slower with firefox (without any extension, v98.0.2).

Result with chrome :
time of instanciation : 765.4000000953674 ms

Result with firefox :
time of instanciation : 2103 ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants