-
Notifications
You must be signed in to change notification settings - Fork 641
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
perf: make model creation faster #2113
Conversation
FWIW, I don't really trust the memory usage measurement, so I wouldn't read too much into that, but it's nice that they are overall trending downwards. I would be curious to know if the trendline looks the same for other folks. |
I also published a release candidate tag for |
This is not very rigorously measured, but in my production app (React Native using Hermes) we load about 8,000 MST objects into memory. In the past, this has taken ~3-4 seconds (we do some other operations on it that are expensive during a loading sequence). With this change, I'm seeing 0.8-1.2 seconds. We've made some other changes since I last measured it, so I don't think that's all because of this PR, but I'm starting to feel good about these improvements being real. |
51b801c
to
01594bd
Compare
Since we've had a little review, talked about it in Slack, and have a release candidate out for this, I'm going to merge this into |
I was also messing around with VS Code debugger and fixed a path that is no longer correct. |
this is nice, sorry missed the PR |
What does this PR do and why?
In the
reduce
function insidetoPropertiesObject
, we've been creating a new object and merging it withprops
for every iteration. This might be a bottleneck ifprops
has a lot of keys. Instead, we can directly assign the new property toprops
and avoid the object creation and merging.We've also been using an object to check for duplicate keys. This requires converting the keys to strings. Using a
Set
should avoid that issue, and IMO is a little nicer to read.I believe this improves the speed of model creation above the margin of error of my benchmarks. I used this performance testing tool to check the performance of MST
5.3.0
and this changeset running in node. I haven't yet checked in web or bun. Here's a few relevant test results:5.3.0
This branch
Comparison
benchmark.js
does something under the hood to prevent the engine from making optimizations, so individual model creation never gets optimized test-to-test.Steps to validate locally
results/
directory.npm run build
, thennpm link
inmobx-state-tree
.mst-performance-testing
directory and runnpm link mobx-state-tree
.5.3.0
as well because of the way npm linking happens.Minor note
I changed around some comments with regard to personal preference. I do not feel strongly about those changes.