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

Easy Performance Improvements (Reducing lodash filter and shallow clone in hot path) #134

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

winrid
Copy link

@winrid winrid commented Sep 27, 2022

  1. Lodash _.filter is about twice as slow as Array.filter, same with _.map if we need to just support taking a function as a filter.
  2. _.clone() is much slower than {...obj}.
  3. Tried to only change things in hot path and where risk seemed low, and limit logic changes.

…ne in hot path)

1. Lodash _.filter is about twice as slow as Array.filter, same with _.map.
2. _.clone() is much slower than {...obj}.
3. Tried to only change things in hot path and where risk seemed low, and limit logic changes.
@winrid
Copy link
Author

winrid commented Nov 29, 2022

Note that when I did this I didn't realize the API supported filters by lodash predicate objects - I didn't know people were doing that :)

So this would break the API in several places. Instead we should probably check the filter predicate type before calling into lodash, and otherwise use the native filter methods.

EDIT - fixed

@sparr
Copy link
Contributor

sparr commented Jun 17, 2023

Do you plan to add that check for filter predicate type?

@winrid
Copy link
Author

winrid commented Jun 17, 2023

@sparr I was going to, but then it sounded like it wasn't going to be merged. It folks want the change I will finish it.

@sparr
Copy link
Contributor

sparr commented Jun 17, 2023

The devs here haven't merged a lot of community contributions, especially lately, but they do take some. And PRs have value outside of being merged; you can find plenty of mention in other PRs of people using them on private servers despite them not being merged to the main repo.

@winrid
Copy link
Author

winrid commented Jun 17, 2023

Okay! I'll try to wrap it up tomorrow then if you want to use it :)

@sparr
Copy link
Contributor

sparr commented Jun 17, 2023

Full disclosure... I've run local servers for testing, but when I play online it will almost certainly be on the official servers. So don't implement this just for me, although I'd still appreciate seeing it available.

@winrid
Copy link
Author

winrid commented Jun 17, 2023

@sparr Done. I was able to move the optimization, and add safety checks, to utils. This also reduces the scope of changes from 18 files to 11 (10 + utils.js).

@winrid
Copy link
Author

winrid commented Jun 17, 2023

@artch This is done, if you are interested. However I am not able to run the tests yet due to node/python versions etc.

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