Oom/combined zod patch lazy loading#264342
Closed
sdesalas wants to merge 6 commits intoelastic:mainfrom
Closed
Conversation
Contributor
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Comment on lines
+44
to
+57
| return new Proxy({} as T, { | ||
| get(_target, prop) { | ||
| const real = materialize() as unknown as Record<PropertyKey, unknown>; | ||
| const value = real[prop]; | ||
| if (typeof value === 'function') { | ||
| return (value as (...args: unknown[]) => unknown).bind(real); | ||
| } | ||
| return value; | ||
| }, | ||
| has(_target, prop) { | ||
| return prop in (materialize() as unknown as object); | ||
| }, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🟢 Low src/lazy_gcable_object.ts:44
The Proxy lacks a set trap, so assignments like lazyThing.prop = value write to the empty {} target rather than the materialized object. The value is stored on the proxy target but never propagated to the real object; subsequent reads return the unmodified value from the materialized object, causing the assignment to be silently lost.
- return new Proxy({} as T, {
- get(_target, prop) {
- const real = materialize() as unknown as Record<PropertyKey, unknown>;
- const value = real[prop];
- if (typeof value === 'function') {
- return (value as (...args: unknown[]) => unknown).bind(real);
- }
- return value;
- },
- has(_target, prop) {
- return prop in (materialize() as unknown as object);
- },
- });🤖 Copy this AI Prompt to have your agent fix this:
In file src/platform/packages/shared/kbn-lazy-object/src/lazy_gcable_object.ts around lines 44-57:
The `Proxy` lacks a `set` trap, so assignments like `lazyThing.prop = value` write to the empty `{}` target rather than the materialized object. The value is stored on the proxy target but never propagated to the real object; subsequent reads return the unmodified value from the materialized object, causing the assignment to be silently lost.
Evidence trail:
src/platform/packages/shared/kbn-lazy-object/src/lazy_gcable_object.ts lines 44-53 show the Proxy with only `get` and `has` traps, no `set` trap. JavaScript Proxy spec: without a `set` trap, assignments go to the target (empty `{}`) while the `get` trap reads from `materialize()`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Combination of 2 PRs for testing local memory impact:
Local testing results
Method
kibana-memory-profile-*.csvin.vscode/mem-stats(columnheap_used, bytes).timestamp_msare included (first 30s of each run discarded).heap_used(max after cutoff), averageheap_used(mean after cutoff).Test conditions (local)
--devmode.NODE_OPTIONS="--max_old_space_size=1400"— the1400in folder names refers to this V8 heap limit (MiB), not measuredheap_used.Datasets (folder names)
main-1400-round1,main-1400-round2,main-1400-round3combined-zod-patch-lazy-loading-1400-round1,combined-zod-patch-lazy-loading-1400-round2,combined-zod-patch-lazy-loading-1400-round3Results (heapUsed, after first 30s)
main
main-1400-round1main-1400-round2main-1400-round3main-1400-round2)combined zod-patch-lazy-loading
combined-zod-patch-lazy-loading-1400-round1combined-zod-patch-lazy-loading-1400-round2combined-zod-patch-lazy-loading-1400-round3combined-zod-patch-lazy-loading-1400-round2)Which set is more memory efficient?
The combined zod-patch-lazy-loading branch is more memory efficient on these runs: lower peak and lower average
heap_usedafter the 30s warm-up in every run, and lower means across the three runs (~58 MiB lower mean peak, ~115 MiB lower mean average vs main).Logs
Bundled
kibana.output.*.txtfiles in the same folders show noFATAL, OOM, or “JavaScript heap out of memory” lines in this sample.