Performance Improvements

[appden]

Thanks for this awesome state management library and its excellent documentation! I found a few small areas for performance improvement. Some might seem like micro-optimizations, but anything helps in a library like this, especially with React Native apps running on mobile devices without JIT. 😀

Commit messages copied below for ease of review:

Avoid creating new objects for no-op updates

When updating a focused atom with the same value, nothing should change, otherwise we'll get unnecessary re-rendering.

Avoid unnecessary clone+shift in getIn/setIn calls

By passing the index recursively, we can avoid cloning and shifting path arrays, which puts is more costly and puts more pressure on the GC.

Avoid duplicate calls to get() for focused atoms

Use slice() to clone arrays (~10x faster)

Micro-optimization, but anything helps in a low-level library like this, especially in React Native apps on mobile devices where the JS engine cannot JIT.

See:

• https://tinyurl.com/2p889sun
• https://www.codefellows.org/blog/speed-alley-the-fastest-way-to-copy-an-array/

[onurkerimov]

Hi @appden . Thank you for contributing! Also thanks for the kind words about the library.

Some might seem like micro-optimizations, but anything helps in a library like this, especially with React Native apps running on mobile devices without JIT.

Totally understandable. I just observed the PR and I think what you did here is so valuable. I'm really happy that getIn and setIn no longer rely on cloning arrays. Also I didn't know that slice was 10x more performant. Beautiful additions. Thank you.

I want to merge this soon, but I want to kindly explain that I'll not include the following 2 improvements at this time:

• Avoid duplicate calls to get() for focused atoms
  • internal.get call is pretty cheap, and I want to keep the bundle size small unless the advancement is essential
• Avoid creating new objects for no-op updates
  • This one if I'm not mistaken, doesn't really stop parent objects from being cloned. It only avoids 1 cloning operation when we reach the leaf object. I believe the same kind of optimization can be implemented in internal.set level, instead of inside setIn. (This still has trade-offs)

Thank you for the awesome work!

[appden]

Thanks for your response @onurkerimov!

I'd kindly like to address your concerns, especially for the "avoid creating new objects for no-op updates" change, which is the most essential and was the original motivator for me to dig into the code.

Avoid duplicate calls to get() for focused atoms

This one is pretty minor optimization, but in deeply focused trees, it nearly halves the number of recursive function calls and object lookups when calling set(). That isn't a huge deal so it's not critical, but nice to have, and only costs a few bytes in the final build. I think we can find much larger build size savings if these extra bytes are critical to you, but ultimately I don't feel that strongly about this change. 😀

Avoid creating new objects for no-op updates

I had hoped the test I added better showed the problem this solved, so I apologize for my lack of explanation on this one. I just adapted "use-items-abstraction" example (which is currently broken btw) from the repo to better illustrate the issue.

This is the sandbox without the fix: https://codesandbox.io/s/upbeat-jennings-rph5wl

As you can see, clicking the clear button on items that are already zero results in the entire app still re-rendering...

https://user-images.githubusercontent.com/70904/206926915-0304c18c-2ac6-484a-aefc-695ead9d7bd6.mov

And here's the sandbox with the fix: https://codesandbox.io/s/awesome-euler-zlform

Now, clicking the clear button when already zero results in no change to the data structure, which means there's no need to re-render the app...

https://user-images.githubusercontent.com/70904/206927036-6962dc5e-813d-43a2-8978-50eb42fbec6f.mov

I hope that clears up why this change is so important! 😀

[appden]

This one if I'm not mistaken, doesn't really stop parent objects from being cloned. It only avoids 1 cloning operation when we reach the leaf object.

Following up on this concern for extra clarification. This change does in fact prevent parent objects from being cloned as well due to its recursive nature. As you can see in the test with the deeply nested structure ({ deeply: { nested: { number: 5 } } }), setting number to 5 again results in the whole object tree remaining the same because nextValue === currentValue will be true up the entire recursive call stack.

[appden]

Hi @onurkerimov, kindly following up on this. Thanks for your time and consideration as always!

[onurkerimov]

Thank you for the extra clarification @appden ! First time I looked at this I didn't notice that the setIn function's call order is changed that much, so I thought it doesn't prevent recursive clone calls. Looks good, I just checked this locally as well. I'm merging it now. Thank you, as always, for your contribution!