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

perf: use map instead of weak map #2084

Conversation

BrianHung
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@BrianHung BrianHung force-pushed the use-map-instead-of-weakmap-running-actions branch from 604a397 to eda30bb Compare September 18, 2023 19:14
@coolsoftwaretyler
Copy link
Collaborator

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
Copy link
Collaborator

@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!

@coolsoftwaretyler coolsoftwaretyler merged commit 5b13686 into mobxjs:master Oct 4, 2023
1 check passed
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.

2 participants