Wr/fix heap oom large custom labels @W-21251439@#1795
Conversation
…r-child Previously initMergeMap called child.parseXml() for each child, which re-parsed the entire parent XML file (e.g. 31K lines) for every one of 3,945 children. Now parses each parent once and extracts children directly from the parsed result.
Verifies correctness of multi-package merge with deduplication: - Labels unique to each package are preserved - Shared labels (same fullName) are deduplicated - First-encountered definition wins on conflicting content
WillieRuemmele
changed the title
jfeingold35
left a comment
jfeingold35
left a comment
There was a problem hiding this comment.
Left some comments, also there are merge conflicts to resolve.
| // eslint-disable-next-line no-await-in-loop | ||
| const parentXml = await component.parseXml(); |
There was a problem hiding this comment.
If you want to get rid of this eslint-disable statement, you could do something like
parsedComponents = await Promise.all(allComponentsOfType.map(c => c.parseXml()));
for (const parsedComponent of parsedComponents) {
...
}
There was a problem hiding this comment.
Done — switched to Promise.all upfront then iterate the results. No more eslint-disable needed.
| for (const child of childComponents.filter(ensureMetadataComponentWithParent)) { | ||
| childXmls.push({ | ||
| cmp: child, | ||
| // eslint-disable-next-line no-await-in-loop |
There was a problem hiding this comment.
Was there a way to fix the bug that didn't involve removing Promise.all?
There was a problem hiding this comment.
Yes — the real fix is deduplicateChildren (prevents the unbounded accumulation that caused OOM). The sequential loop here was unnecessary since getXmlFromCache already bounds memory via caching. Restored Promise.all.
| commentPropName: XML_COMMENT_PROP_NAME, | ||
| }); | ||
| const builtXml = String(builder.build(this.xmlObject)); | ||
| this.xmlObject = undefined; |
There was a problem hiding this comment.
Why does this explicitly need to be set to undefined?
There was a problem hiding this comment.
Two reasons: (1) guards against Node calling _read() more than once — the early-return at the top checks for undefined, and (2) releases the reference so GC can reclaim large JSON trees after they're serialized. Added a comment explaining this.
- nonDecompositionFinalizer: parallelize parseXml with Promise.all then iterate results, removing eslint-disable (Jamie suggestion) - recompositionFinalizer: restore Promise.all since getXmlFromCache handles memory; the real fix is deduplicateChildren - streams: add comment explaining why xmlObject is set to undefined
…tomLabels # Conflicts: # METADATA_SUPPORT.md
Promise.all would hold all parsed child XML in memory simultaneously. For large CustomLabels (thousands of children), this causes OOM. Sequential iteration with the cache lets GC reclaim between iterations.
…OM QA" This reverts commit 60f1b00.
3 participants
What does this PR do?
What issues does this PR fix or reference?
#, @W-21251439@
Functionality Before
<insert gif and/or summary>
Functionality After
<insert gif and/or summary>
Metadata Registry: Successful Deploy and Retrieve (required if applicable)
Deploy output:
<paste CLI output and/or screenshot>
Retrieve output:
<paste CLI output and/or screenshot>
Summary of Changes
Problem
When multiple package directories contain the same
CustomLabelsfile (common in multi-package projects), the recomposition finalizer (source→metadata conversion for deploy) was accumulating duplicate children. With large label files across 2-3 packages, this caused the merged output to double or triple in size, leading to heap OOM crashes.Fix:
recompositionFinalizer.tsAdded
deduplicateChildren()— when the same label (keyed bytype.name#fullName) appears from multiple packages, only the first occurrence is kept. This is safe because:CustomLabels.labelsis an ephemeral artifact for deploy (zipped and sent to the org, never written to disk).nonDecompositionFinalizer, which writes labels back to their respective package directories — users never see git diffs.Fix:
nonDecompositionFinalizer.tsRefactored
initMergeMapto parse each parent XML file once and extract all children directly, rather than re-parsing the parent for every individual child component. This eliminates redundant XML parsing that scaled quadratically with label count.Snapshot diff (~16k lines)
The
customLabels-multipletest snapshot went from 32,918 lines to 16,471 — almost exactly halved. The test data has labels across 2 packages (second-appandthird-app) with largely identical content, so the old behavior was duplicating nearly every entry. The ~92 "additions" in the diff are ordering artifacts from the dedup changing sort positions, not new labels.New test:
customLabels-mergedA focused snapshot test with small synthetic data (3 packages, 8 unique labels, 2 shared across packages) to verify the dedup behavior explicitly without relying on the large real-world test data.