Conversation
a4f901c to
2cb2d3c
Compare
There was a problem hiding this comment.
Just did an initial pass and thought I'd share my thoughts.
This is pretty impressive. It seems like we are patching jest, which is OK, but would be nice if they could make mock instantiation more perf on their side.
In the meantime, I'm wondering whether we can find a solution that does not rely on meta programming. If I have it right, the primary culprit leading to GC thrashing is calls to jest.fn() adding a lot of mock objects to the heap/creating work for jest. Is it possible to create a more targeted solution to handle this?
Something like this to be run at test setup time (using jest's setupFiles, did not test, not sure if possible)
const originalJestFn = jest.fn;
const lazyJestFn: typeof jest.fn = (impl?: (...args: any[]) => any): jest.Mock => {
/* logic to create jest mock instance lazily */
return fn as any;
};
jest.fn = lazyJestFn;This seems like it could get the same thing done and touch a lot fewer files. If it's not possible that's OK too, overall this seems like a really nice improvement.
src/platform/packages/shared/kbn-lazy-object/src/create_lazy_object_from_annotations.ts
Outdated
Show resolved
Hide resolved
| set(v) { | ||
| Object.defineProperty(target, key, { |
There was a problem hiding this comment.
If a lazy prop gets set before it got get will the getter logic be clobbered (effectively nullified)? It seems that's the intent based on your tests, just wanted to confirm.
There was a problem hiding this comment.
yes, it's treated as an override
|
|
||
| const DISABLED = !!process.env.DISABLE_LAZY_OBJECT; | ||
|
|
||
| export function createLazyObjectFromFactories<TFactories extends Record<string, () => any>>( |
There was a problem hiding this comment.
nit: not quite clear to me where this fn is being used.
There was a problem hiding this comment.
I'll add some clarifying comments - the idea is that you can use createLazyObjectFromFactories directly (without the implicit rewrite that lazyObject does).
I think this would help with jest's internals, but it still allocates many objects even when they are not being used, and we'd have to reimplement the Jest |
426ea4a to
d1355c9
Compare
d1355c9 to
1a2073f
Compare
cesco-f
left a comment
There was a problem hiding this comment.
Changes in x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts LGTM.
janmonschke
left a comment
There was a problem hiding this comment.
kibana-cases changes lgtm!
dmlemeshko
left a comment
There was a problem hiding this comment.
src/platform/packages/shared/kbn-test/jest-preset.js changes LGTM
There was a problem hiding this comment.
Nice work @dgieselaar , given the trade-offs this approach makes sense to me. Thanks for driving this to a conclusion.
Do you think it's worth raising the impact of 1000s of repeat calls to jest.fn() to the Jest team in an issue? Perhaps they can delay mock instantiation internally -- I'm happy to do so at some point.
| visitor: { | ||
| CallExpression(path) { | ||
| const { node } = path; | ||
| if (!isLazyObjectCallee(node.callee)) return; |
There was a problem hiding this comment.
Couldn't quite tell whether it is already handled, but we should prevent this transformation from replacing a definition of lazyObject that is not defined by @kbn/lazy-object.
There was a problem hiding this comment.
thanks for the nudge, implemented an import check and added a test for it
kc13greiner
left a comment
There was a problem hiding this comment.
Security changes LGTM!
Ikuni17
left a comment
There was a problem hiding this comment.
The new package @kbn/lazy-object needs to be added to Codeowners. The package itself doesn't have any tests either.
|
@Ikuni17 it does have tests actually. What do you mean that it needs to be added to CODEOWNERS? A codeowner is defined in kibana.jsonc, thats usually enough? |
⏳ Build in-progress, with failures
Failed CI StepsHistory
|
3881e3c to
b6f8c81
Compare
Closes elastic#235079 Lazily instantiate core mocks, to relieve memory pressure. This has the benefit of tests using less memory usage, and thus us being able to run them in parallel without upsizing our CI workers, and CPU time spent on garbage collection goes down. Here's how it affects one of the alerting plugin test files, significant improvements across the board: ```stdout Benchmark diff: cwd -> f69f92 cwd: [Jest] Lazily instantiate mocks f69f92: [Agent Builder] Setup doclinks structure (elastic#235804) alerting-plugin-test - cwd f69f92 Δ CI Duration 6.3s ±1.7% 7.0s ±1.8% 711ms (+11.3%) 95%, +7.9%–+14.8% CPU Usage 6.5s ±2.0% 7.4s ±2.1% 852ms (+13.1%) 95%, +9.1%–+17.2% Max RSS 857.90 MB ±1.3% 977.20 MB ±2.4% 119.30 MB (+13.9%) 95%, +10.3%–+17.6% GC time 190ms ±4.4% 252ms ±3.4% 62ms (+32.6%) 95%, +23.8%–+42.0% ``` ### How it works Core mocks are instantiated using `lazyObject`. This is a helper utility that will be rewritten using a Babel plugin to create annotated getters that will only instantiate the mock when it is accessed. ### Why it helps Core mocks are eagerly created, and this means that many objects will be allocated. Specifically, jest will keep mocks in a global registry to support `jest.clearAllMocks()`. This means only when a test file completes these functions will be garbage collected. Increased memory consumption has two consequences that are relevant in this context: - they increase memory pressure, which means garbage collection has to run more frequently to prevent the process from running out of memory - our CI workers (that run Jest tests) have 16gb of memory. running jest configs in parallel means that there currently is a risk that either the processes themselves run out of memory, or the agent will. if we reduce memory usage, we can run them in parallel. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes #235079 Lazily instantiate core mocks, to relieve memory pressure. This has the benefit of tests using less memory usage, and thus us being able to run them in parallel without upsizing our CI workers, and CPU time spent on garbage collection goes down. Here's how it affects one of the alerting plugin test files, significant improvements across the board: ```stdout Benchmark diff: cwd -> f69f92 cwd: [Jest] Lazily instantiate mocks f69f92: [Agent Builder] Setup doclinks structure (#235804) alerting-plugin-test - cwd f69f92 Δ CI Duration 6.3s ±1.7% 7.0s ±1.8% 711ms (+11.3%) 95%, +7.9%–+14.8% CPU Usage 6.5s ±2.0% 7.4s ±2.1% 852ms (+13.1%) 95%, +9.1%–+17.2% Max RSS 857.90 MB ±1.3% 977.20 MB ±2.4% 119.30 MB (+13.9%) 95%, +10.3%–+17.6% GC time 190ms ±4.4% 252ms ±3.4% 62ms (+32.6%) 95%, +23.8%–+42.0% ``` ### How it works Core mocks are instantiated using `lazyObject`. This is a helper utility that will be rewritten using a Babel plugin to create annotated getters that will only instantiate the mock when it is accessed. ### Why it helps Core mocks are eagerly created, and this means that many objects will be allocated. Specifically, jest will keep mocks in a global registry to support `jest.clearAllMocks()`. This means only when a test file completes these functions will be garbage collected. Increased memory consumption has two consequences that are relevant in this context: - they increase memory pressure, which means garbage collection has to run more frequently to prevent the process from running out of memory - our CI workers (that run Jest tests) have 16gb of memory. running jest configs in parallel means that there currently is a risk that either the processes themselves run out of memory, or the agent will. if we reduce memory usage, we can run them in parallel. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes elastic#235079 Lazily instantiate core mocks, to relieve memory pressure. This has the benefit of tests using less memory usage, and thus us being able to run them in parallel without upsizing our CI workers, and CPU time spent on garbage collection goes down. Here's how it affects one of the alerting plugin test files, significant improvements across the board: ```stdout Benchmark diff: cwd -> f69f92 cwd: [Jest] Lazily instantiate mocks f69f92: [Agent Builder] Setup doclinks structure (elastic#235804) alerting-plugin-test - cwd f69f92 Δ CI Duration 6.3s ±1.7% 7.0s ±1.8% 711ms (+11.3%) 95%, +7.9%–+14.8% CPU Usage 6.5s ±2.0% 7.4s ±2.1% 852ms (+13.1%) 95%, +9.1%–+17.2% Max RSS 857.90 MB ±1.3% 977.20 MB ±2.4% 119.30 MB (+13.9%) 95%, +10.3%–+17.6% GC time 190ms ±4.4% 252ms ±3.4% 62ms (+32.6%) 95%, +23.8%–+42.0% ``` ### How it works Core mocks are instantiated using `lazyObject`. This is a helper utility that will be rewritten using a Babel plugin to create annotated getters that will only instantiate the mock when it is accessed. ### Why it helps Core mocks are eagerly created, and this means that many objects will be allocated. Specifically, jest will keep mocks in a global registry to support `jest.clearAllMocks()`. This means only when a test file completes these functions will be garbage collected. Increased memory consumption has two consequences that are relevant in this context: - they increase memory pressure, which means garbage collection has to run more frequently to prevent the process from running out of memory - our CI workers (that run Jest tests) have 16gb of memory. running jest configs in parallel means that there currently is a risk that either the processes themselves run out of memory, or the agent will. if we reduce memory usage, we can run them in parallel. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes #235079
Lazily instantiate core mocks, to relieve memory pressure. This has the benefit of tests using less memory usage, and thus us being able to run them in parallel without upsizing our CI workers, and CPU time spent on garbage collection goes down. Here's how it affects one of the alerting plugin test files, significant improvements across the board:
How it works
Core mocks are instantiated using
lazyObject. This is a helper utility that will be rewritten using a Babel plugin to create annotated getters that will only instantiate the mock when it is accessed.Why it helps
Core mocks are eagerly created, and this means that many objects will be allocated. Specifically, jest will keep mocks in a global registry to support
jest.clearAllMocks(). This means only when a test file completes these functions will be garbage collected. Increased memory consumption has two consequences that are relevant in this context: