search

mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.99k stars 641 forks source link

perf: use map instead of weak map #2084

Closed BrianHung closed 1 year ago

BrianHung commented 1 year ago

What does this PR do and why?

Uses map instead of weakmap for running actions. Each call has a unique id, so the memory overhead of a weakmap is not needed.

coolsoftwaretyler commented 1 year ago

Hey @BrianHung - I'm sorry I didn't see this before. I've been really busy at work + I don't seem to get notifications for PRs in this repo? I'm going to turn on my notifications + get around to reviewing this and many of the other open PRs. Sorry to leave you hanging, and thanks for your patience!

BrianHung commented 1 year ago

No worries! I always use patch package to use my PRs locally so don’t rush.

coolsoftwaretyler commented 1 year ago

Hey @BrianHung - as with the other PRs, would you mind adding a few tests around this, even if we already have some? Looks like we have a test file in packages/mobx-state-tree/__tests__/core/actionTrackingMiddleware2.test.ts - it would be great to add coverage or some redundant tests that make sense or are related to this change.

Thanks!

coolsoftwaretyler commented 1 year ago

@BrianHung - I went ahead and added some more tests here. It was fun to play around with here, and I appreciate you taking a detailed look at how we can improve the overhead on this one.

I'm going to merge this, but when you get a chance, will you please update this discussion post with any details about how you found this problem? Did you notice memory overhead of maps in a particular scenario? How much improvement did you get? I want to take whatever was the cause of your PR and add it to some of our measurement work around performance.

Thanks!