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: add isSimpleValidator #11412

Merged
merged 6 commits into from
Mar 7, 2022
Merged

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Feb 17, 2022

This PR adds a helper method called isSimpleValidator. Basically it is a function to determine if a validator is just a simple object or if it is a more complex one. To avoid the usage in other places, it is not named isFlatObject. It uses a WeakMap to cache the result and gain as such speed.

With isSimpleValidator we determine if we should make a deep clone of the validator or a shallow copy of it with Object.assign.

Before:

MONGOOSE_DEV=1 node validate
invalid x 23,461 ops/sec ±0.78% (89 runs sampled)
valid x 75,657 ops/sec ±0.67% (90 runs sampled)

after

MONGOOSE_DEV=1 node validate
invalid x 29,047 ops/sec ±0.37% (95 runs sampled)
valid x 148,950 ops/sec ±0.29% (93 runs sampled)

So we basically double the performance on the valid path.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 17, 2022

If we combine this with #11408 we get super performance speed for validation :D

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 17, 2022

With the improved benchmark from #11415
before:

before:

MONGOOSE_DEV=1 node validate
invalid async x 21,179 ops/sec ±0.71% (84 runs sampled)
valid async x 41,408 ops/sec ±1.34% (83 runs sampled)
invalid sync x 24,553 ops/sec ±0.76% (93 runs sampled)
valid sync x 76,212 ops/sec ±0.67% (92 runs sampled)

after:

 MONGOOSE_DEV=1 node validate
invalid async x 23,496 ops/sec ±1.08% (82 runs sampled)
valid async x 60,767 ops/sec ±1.08% (80 runs sampled)
invalid sync x 27,526 ops/sec ±1.27% (92 runs sampled)
valid sync x 142,613 ops/sec ±0.36% (96 runs sampled)

Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

*/
const isSimpleValidatorCache = new WeakMap();

module.exports = function isSimpleValidator(obj) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only request would be to get rid of the caching. I know that it should be faster and the risk of WeakMap() causing any sort of memory issue is extremely low, but I'd rather write off that risk altogether and just keep this as a simple loop.

@vkarpov15 vkarpov15 added this to the 6.2.5 milestone Mar 7, 2022
@vkarpov15
Copy link
Collaborator

I'll merge this and remove the WeakMap caching locally

@vkarpov15 vkarpov15 merged commit 029b786 into Automattic:master Mar 7, 2022
vkarpov15 added a commit that referenced this pull request Mar 7, 2022
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Mar 7, 2022

Seems like the performance is still very good. Strange. Expected a higher performance penalty. But hey, i dont complain.

@Uzlopak Uzlopak deleted the isSimpleValidator branch April 19, 2022 12:31
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.

None yet

3 participants