perf: prefer use factory instead of use value when possible #12753

H4ad · 2023-11-15T21:58:26Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Because of the nature of the ModuleTokenFactory, heavy objects will slow down the initialization, especially when using .proto files.

Issue Number: #12719
Closes #12719

What is the new behavior?

Now, we defer the initialization of the .proto files by using useFactory, this will fix any initialization issue.

The second run is with the fix.

I also updated almost all the references for useValue, so this can help avoid the same issue in other places.

Does this PR introduce a breaking change?

Yes
No

jmcdo29 · 2023-11-15T22:04:22Z

Very interesting, but nice to see a solution for this. Great job!

Tony133 · 2023-11-15T22:07:57Z

Let's see what Kamil thinks, but ha me I really like this modification. 😉👍

Tony133 · 2023-11-15T22:16:44Z

Corrections also need to be made to the tests, see here:

micalevisk · 2023-11-15T22:49:33Z

I'd suggest to add some comments on why we are using factory over value providers in those places to prevent on future refactoring on them

micalevisk · 2023-11-15T22:51:02Z

packages/core/injector/internal-core-module/internal-core-module-factory.ts

-        useValue: ExternalContextCreator.fromContainer(container),
+        useFactory: () => ExternalContextCreator.fromContainer(container),
      },
      {
        provide: ModulesContainer,
-        useValue: container.getModules(),
+        useFactory: () => container.getModules(),
      },
      {
        provide: HttpAdapterHost,
-        useValue: httpAdapterHost,
+        useFactory: () => httpAdapterHost,
      },
      {
        provide: LazyModuleLoader,
        useFactory: lazyModuleLoaderFactory,
      },
      {
        provide: SerializedGraph,
-        useValue: container.serializedGraph,
+        useFactory: () => container.serializedGraph,


I notice another good side-effect on changing ExternalContextCreator and SerializedGraph providers to factory:

SerializedGraph#toJSON was called 6x in a very simple nestjs app before these changes, which I think it was useless because it was only invoked due to the name toJSON being known as a special method for JSON.stringify (used by stringify from fast-safe-stringify)

🥳

coveralls · 2023-11-16T00:29:12Z

Pull Request Test Coverage Report for Build 349dfb92-7ead-48ea-8cfa-4a0a9202a4d3

4 of 9 (44.44%) changed or added relevant lines in 4 files are covered.
16 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.3%) to 91.979%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/common/module-utils/configurable-module.builder.ts	0	1	0.0%
packages/core/injector/internal-core-module/internal-core-module-factory.ts	0	4	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/common/module-utils/configurable-module.builder.ts	6	79.03%
packages/core/helpers/external-context-creator.ts	10	86.11%

Totals
Change from base Build 23ebebfc-1ad4-4b2b-8c07-2691151ef178:	-0.3%
Covered Lines:	6674
Relevant Lines:	7256

💛 - Coveralls

kamilmysliwiec · 2023-11-17T12:03:10Z

packages/platform-express/multer/multer.module.ts

-        { provide: MULTER_MODULE_OPTIONS, useValue: options },
+        // useFactory is for performance reasons
+        // see more: https://github.com/nestjs/nest/issues/12738#issuecomment-1810987001
+        { provide: MULTER_MODULE_OPTIONS, useFactory: () => options },


With this change, you won't be able to use multiple multer modules in your project (due to equal hash tokens).

Yeah, but if the user uses https://docs.nestjs.com/techniques/file-upload#async-configuration, they already have this effect.

Anyone that does not use useValue will have this same issue, so I think we can consider this a minor if we want to release a safer version.

Maybe we can study emit warnings when we detect two modules duplicated (with same hash), so the users can be aware of this behavior.

But this could be better in a follow-up PR, what do you think?

Actually, the https://github.com/nestjs/nest/pull/12753/files/8a2d19db5a2bf3c9026139cef9f884bb95b0100c#diff-cfbc60d072c116c16132ec78536eae4514513069d6e2020ab1444e41175ad5f1R25 already makes the module be unique.

packages/microservices/module/clients.module.ts

kamilmysliwiec · 2023-11-17T12:07:58Z

Long term we should circle back on something we discussed a while ago - so basically replacing the hashing logic with something very simple (and not ideal but that's the middle ground I guess), for example with a custom function that simply generates a unique ID based on the registered providers (their stringified tokens), imports, and exports - that should be sufficient and will leave us with a (fairly) unique hash and with no requirement for hashing useValue providers. Would you like to create a PR for this (if it all makes sense)?

Eventually, we'll leave that hash for the purpose of diff algorithms (devtools) and just use weak references, dropping the entire "modules redundancy optimization" technique in future major versions.

H4ad · 2023-11-17T12:35:17Z

@kamilmysliwiec I will create an issue to compile all the viable options to solve this issue based on that previous comment I already made and include your suggestion then we can validate which one is possible to build and which has the best DX.

But as soon I have more time, I can create a PR for this.

H4ad · 2023-11-17T13:13:08Z

Was not possible to defer RoutesModule due to the issue with the duplicated hash (from what I saw), so I removed that optimization.

kamilmysliwiec · 2023-12-18T07:49:13Z

lgtm

micalevisk · 2024-01-22T18:29:41Z

@H4ad do you think that this change is related with the issue #13076 ?

H4ad · 2024-01-22T18:54:48Z

Improbable but not impossible, if you have some reproduction, I can investigate.

perf: prefer use factory instead of use value when possible

53d2094

Tony133 approved these changes Nov 15, 2023

View reviewed changes

H4ad mentioned this pull request Nov 15, 2023

Avoid serializing buffers when creating module's opaque token #12719

Closed

15 tasks

micalevisk reviewed Nov 15, 2023

View reviewed changes

fixup! perf: prefer use factory instead of use value when possible

8a2d19d

kamilmysliwiec reviewed Nov 17, 2023

View reviewed changes

packages/microservices/module/clients.module.ts Outdated Show resolved Hide resolved

fixup! perf: prefer use factory instead of use value when possible

9a1ea46

fixup! perf: prefer use factory instead of use value when possible

31a0b18

fixup! perf: prefer use factory instead of use value when possible

ae03ccc

H4ad requested a review from kamilmysliwiec November 17, 2023 14:48

H4ad mentioned this pull request Dec 7, 2023

feat(core): added unique dynamic module factory #12898

Closed

12 tasks

kamilmysliwiec merged commit d165eed into nestjs:master Dec 18, 2023
5 checks passed

kamilmysliwiec added the type: enhancement 🐺 label Dec 18, 2023

rickdgeerling mentioned this pull request Jan 23, 2024

Feat: create styleguide rule for providers TrilonIO/eslint-plugin#13

Closed

thiagomini mentioned this pull request Feb 3, 2024

Feat: Enforce custom provider types TrilonIO/eslint-plugin#24

Closed

H4ad deleted the perf/use-factory-instead-of-use-value branch June 1, 2024 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: prefer use factory instead of use value when possible #12753

perf: prefer use factory instead of use value when possible #12753

H4ad commented Nov 15, 2023

jmcdo29 commented Nov 15, 2023

Tony133 commented Nov 15, 2023

Tony133 commented Nov 15, 2023

micalevisk commented Nov 15, 2023

micalevisk Nov 15, 2023 •

edited

Loading

coveralls commented Nov 16, 2023 •

edited

Loading

kamilmysliwiec Nov 17, 2023

H4ad Nov 17, 2023

H4ad Nov 17, 2023

kamilmysliwiec commented Nov 17, 2023

H4ad commented Nov 17, 2023

H4ad commented Nov 17, 2023

kamilmysliwiec commented Dec 18, 2023

micalevisk commented Jan 22, 2024 •

edited

Loading

H4ad commented Jan 22, 2024

perf: prefer use factory instead of use value when possible #12753

perf: prefer use factory instead of use value when possible #12753

Conversation

H4ad commented Nov 15, 2023

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

jmcdo29 commented Nov 15, 2023

Tony133 commented Nov 15, 2023

Tony133 commented Nov 15, 2023

micalevisk commented Nov 15, 2023

micalevisk Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 16, 2023 • edited Loading

Pull Request Test Coverage Report for Build 349dfb92-7ead-48ea-8cfa-4a0a9202a4d3

💛 - Coveralls

kamilmysliwiec Nov 17, 2023

Choose a reason for hiding this comment

H4ad Nov 17, 2023

Choose a reason for hiding this comment

H4ad Nov 17, 2023

Choose a reason for hiding this comment

kamilmysliwiec commented Nov 17, 2023

H4ad commented Nov 17, 2023

H4ad commented Nov 17, 2023

kamilmysliwiec commented Dec 18, 2023

micalevisk commented Jan 22, 2024 • edited Loading

H4ad commented Jan 22, 2024

micalevisk Nov 15, 2023 •

edited

Loading

coveralls commented Nov 16, 2023 •

edited

Loading

micalevisk commented Jan 22, 2024 •

edited

Loading