perf(core): fix cold start performance for big projects #10886
Conversation
|
can you run |
| if (typeof obj === 'function') { | ||
| return obj.name; |
There was a problem hiding this comment.
IIRC, and I could be misremembering here, this is how Nest v7 used to work, and it was changed in v8 so that we could have multiple classes with the same name without having a collision based on the name alone, which is why we use the full class reference.
There was a problem hiding this comment.
@jmcdo29 I see, thanks for the context. And it's interesting that it's working with a depth of 3 for me without any collisions, but I didn't test in other scenarios, which I apologize.
|
I've applied this change in a private nestjs v9 app and it didn't work. It wasn't able to find the provider created by |
| return value.name; | ||
| } | ||
| return hash(funcAsString, { ignoreUnknown: true }); | ||
| public shallow(obj: Record<string, any>, depth = 3) { |
There was a problem hiding this comment.
I like your intention of creating this function but I don't think that this function could be a good fit here.
We don't know exactly the depth that could be ideal to take as a parameter here.
If we log what is passed to this function, we could see values like this:
This is InternalCoreModule, which is passed in every initialization.
There was a problem hiding this comment.
@H4ad thanks for going through it, indeed I was fitting the value to what works with the project at hand, which uses nestjs 8.x.
IMHO the regex there is an unnecessary overhead for something like this.
In theory the recursion is infinite at current state, even with Circular traps. It could be make better with tail call recursion so it won't rely on the stack though.
If this lands in object-hash, this function could definitely go away
There was a problem hiding this comment.
object-hash already handles Circular references, so we can just pass the entire object to it without worrying.
|
I did some testing, if we remove Code: const opaqueToken = {
id: moduleId,
module: this.getModuleName(metatype),
dynamic: dynamicModuleMetadata,
};Before: After:
But I had a problem, if we just do this optimization we see this error: This issue is caused by nest/packages/core/injector/modules-container.ts Lines 4 to 10 in 0e0e24b I think that by extending So I thought of two options: First, ignore
The second option is to pass the I think only Kamil could answer if worth to continue this optimization based on what I discovered, the option with lower effort is to ignore I tried importing |
This sounds like a nice workaround |
|
@pocesar Hey man, could you push the changes based on my comment? Basically, you will need to remove Also, after pushing the changes, we will need to test better the modules to check if we could find modules that could not be hashed to document to users. |
|
@H4ad if you want to go ahead and make a PR feel free! we needed this faster so this is my monkeypatch for it if anyone needs it: working fine and down to 48ms locally /* eslint-disable no-param-reassign */
import type { Type } from '@nestjs/common/interfaces/type.interface.js';
import { randomStringGenerator } from '@nestjs/common/utils/random-string-generator.util.js';
import type { INestApplication, DynamicModule } from '@nestjs/common';
import type { NestContainer } from '@nestjs/core';
import hash from 'object-hash';
import { NestFactoryStatic } from '@nestjs/core/nest-factory.js';
import { ModuleCompiler } from '@nestjs/core/injector/compiler.js';
export class FastModuleTokenFactory {
private readonly moduleIdsCache = new WeakMap<Type<unknown>, string>();
public create(
metatype: Type<unknown>,
dynamicModuleMetadata?: Partial<DynamicModule> | undefined,
): string {
const moduleId = this.getModuleId(metatype);
const opaqueToken = {
id: moduleId,
module: this.getModuleName(metatype),
dynamic: dynamicModuleMetadata,
};
return hash(this.replace(opaqueToken));
}
public getModuleId(metatype: Type<unknown>): string {
let moduleId = this.moduleIdsCache.get(metatype);
if (moduleId) {
return moduleId;
}
moduleId = randomStringGenerator() as string;
this.moduleIdsCache.set(metatype, moduleId);
return moduleId;
}
public getModuleName(metatype: Type<any>): string {
return metatype.name;
}
replace(obj: Record<string, any>, maxDepth = 4) {
if (typeof obj === 'object' && obj !== null) {
if (maxDepth <= 0) {
return `[object Object(${maxDepth})]`;
}
// for-loop + Object.create is much faster than reduce
const result = Object.create(null);
for (const key of Object.keys(obj)) {
result[key] = this.replace(obj[key], maxDepth - 1);
}
return result;
}
if (typeof obj === 'function' || typeof obj === 'symbol') {
return `${(obj as any).name ?? (obj as any).toString()}${maxDepth}`;
}
return obj;
}
}
export default async <T extends INestApplication>(
rootModule: any,
adapter: any,
): Promise<T> => {
const moduleTokenFactory = new FastModuleTokenFactory();
const instance = new Proxy(new NestFactoryStatic(), {
get(target, p) {
if (p === 'initialize') {
return function trapInitialize(
module: T,
container: NestContainer,
...args
) {
(container as any).moduleTokenFactory = moduleTokenFactory;
(container as any).getModuleTokenFactory = () => moduleTokenFactory;
(container as any).moduleCompiler = new ModuleCompiler(
moduleTokenFactory as any,
);
return target[p].call(target, module, container, ...args);
};
}
return target[p];
},
});
return instance.create<T>(rootModule, adapter);
}; |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Very bad performance on cold starts, around 22 seconds for a medium sized project on Cloud Run
Issue Number: #10844
What is the new behavior?
Get the start time to around 140ms
Does this PR introduce a breaking change?
Other information
I wasn't able to test locally