Skip to content

perf(yaml): use direct assignment for mapping keys - #7126

Merged
bartlomieju merged 2 commits into
denoland:mainfrom
tomas-zijdemans:yaml-perf1
May 14, 2026
Merged

perf(yaml): use direct assignment for mapping keys#7126
bartlomieju merged 2 commits into
denoland:mainfrom
tomas-zijdemans:yaml-perf1

Conversation

@tomas-zijdemans

Copy link
Copy Markdown
Contributor
  • storeMappingPair and mergeMappings now use direct property assignment instead of Object.defineProperty for ordinary keys. The descriptors defineProperty was writing ({writable, enumerable, configurable} all true) are equivalent to plain assignment, and defineProperty is significantly slower in V8.
  • __proto__ still goes through Object.defineProperty to keep the prototype-chain protection in runtimes that enable the Object.prototype.__proto__ setter.

Based on a wide array of yaml files, I see a consistent 1.24x to 1.43x improvement in mapping-heavy YAML, with no regressions in other payloads

@github-actions github-actions Bot added the yaml label May 5, 2026
@codecov

codecov Bot commented May 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.61%. Comparing base (a496da2) to head (59d4424).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7126      +/-   ##
==========================================
- Coverage   94.61%   94.61%   -0.01%     
==========================================
  Files         634      634              
  Lines       51801    51809       +8     
  Branches     9329     9334       +5     
==========================================
+ Hits        49011    49018       +7     
  Misses       2216     2216              
- Partials      574      575       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fibibot fibibot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Descriptor produced by destination[key] = value matches the old {writable, enumerable, configurable}: true triple, and the key === "__proto__" branch preserves the prototype-pollution guard — without it, direct assignment would hit Object.prototype's __proto__ setter and mutate the chain instead of creating an own property. Object.hasOwn at _loader_state.ts:453 still works as the merge de-dup guard for __proto__ because the defineProperty branch leaves an own property behind. Tests cover both storeMappingPair (top-level __proto__) and mergeMappings (<<: merge path), and CI is green.

@fibibot

fibibot commented May 12, 2026

Copy link
Copy Markdown

@bartlomieju this is ready to merge

@lunadogbot lunadogbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Descriptors produced by dest[key] = value match what Object.defineProperty(dest, key, {writable, enumerable, configurable: true, value}) was producing for ordinary keys, so the data shape is unchanged. __proto__ is the only key where the difference matters (because of the accessor on Object.prototype), and it still goes through defineProperty in both mergeMappings and storeMappingPair. The new parse() does not pollute prototype with __proto__ key test exercises both call sites (plain mapping and << merge). CI green.

@lunadogbot

Copy link
Copy Markdown
Contributor

@bartlomieju this is ready to merge

@bartlomieju
bartlomieju merged commit 5ea9159 into denoland:main May 14, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants