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(common): lazy load class validator and class transformer #11676

Closed
wants to merge 4 commits into from
Closed

perf(common): lazy load class validator and class transformer #11676

wants to merge 4 commits into from

Conversation

HGRon
Copy link

@HGRon HGRon commented May 27, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Currently the default behavior of the ValidatorPipe is to load the ClassValidator and the ClassTransformer as soon as the application starts, this took 35ms on my machine:

image

What is the new behavior?

Now the behavior has been modified so that it loads when receiving the first request.

Does this PR introduce a breaking change?

  • Yes
  • No

@coveralls
Copy link

coveralls commented May 27, 2023

Pull Request Test Coverage Report for Build 8ca218e2-352d-4c44-9118-20921fe4ab8e

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 92.641%

Totals Coverage Status
Change from base Build ca6b0cd6-9a68-4f95-9278-bca8f4b87731: 0.007%
Covered Lines: 6534
Relevant Lines: 7053

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

Are we sure that it is better to speed up the bootstrap time by 35ms while slowing down the first request response?

@HGRon
Copy link
Author

HGRon commented May 29, 2023

Yes @kamilmysliwiec, it's the same idea in this PR nestjs/swagger#2392. Basically benefits those who run serverless.

@kamilmysliwiec
Copy link
Member

And what about the majority of users that don't run apps on serverless? Lazily initializing a document is a slightly different optimization - it wouldn't affect application's end-users but just developers (which is fine)

@HGRon
Copy link
Author

HGRon commented May 29, 2023

Regardless of where you deploy a NestJS application, it's ideal to have it initialize as swiftly as possible to ensure prompt request handling.

Although a slower initial request doesn't heavily impact NestJS performance, a slightly delayed startup can be somewhat inconvenient.

Furthermore, not all routes necessitate the use of ValidationPipe. It's possible that the API's first incoming request is for a public route, causing the entire API to incur a penalty for that solitary route.

@kamilmysliwiec
Copy link
Member

it's ideal to have it initialize as swiftly as possible to ensure prompt request handling.

I'd rather say it's ideal to have a fully functioning application - not a partial one that requires loading basic features (like validation) on demand. We don't really defer loading anything, that's the philosophy & a reason why features like async providers were introduced in the first place.

@H4ad
Copy link
Contributor

H4ad commented May 29, 2023

@kamilmysliwiec Don't it worth at least having an option for the user, something like lazyInitialize? By default false to keep the default behavior and serverless users can enable to get a little bit better performance on startup.

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

4 participants