Skip to content

src: reuse cached strings in node_locks.cc - #65081

Open
agape1225 wants to merge 1 commit into
nodejs:mainfrom
agape1225:src-node-locks-reuse-cached-strings
Open

src: reuse cached strings in node_locks.cc#65081
agape1225 wants to merge 1 commit into
nodejs:mainfrom
agape1225:src-node-locks-reuse-cached-strings

Conversation

@agape1225

@agape1225 agape1225 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

CreatePerIsolateProperties() in src/node_locks.cc created new "shared" and "exclusive" V8 strings via FIXED_ONE_BYTE_STRING() on every isolate, even though these strings are already cached on IsolateData as shared_string() / exclusive_string() (defined via PER_ISOLATE_STRING_PROPERTIES in src/env_properties.h) and are already reused this way elsewhere in the same file (CreateLockInfoObject()).

This replaces the two ad-hoc FIXED_ONE_BYTE_STRING(isolate, "shared"/"exclusive") calls with the existing cached accessors for consistency with the rest of the file and to avoid creating duplicate string instances.

CreatePerIsolateProperties() created new "shared" and "exclusive"
strings via FIXED_ONE_BYTE_STRING() on every isolate, even though
these are already cached on IsolateData as shared_string() and
exclusive_string() and reused that way elsewhere in this file
(CreateLockInfoObject()).
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 6, 2026
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (ee64033) to head (2bc2f72).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65081      +/-   ##
==========================================
+ Coverage   90.29%   90.32%   +0.02%     
==========================================
  Files         759      759              
  Lines      247754   248258     +504     
  Branches    46721    46848     +127     
==========================================
+ Hits       223708   224230     +522     
+ Misses      15521    15459      -62     
- Partials     8525     8569      +44     
Files with missing lines Coverage Δ
src/node_locks.cc 72.67% <100.00%> (ø)

... and 36 files with indirect coverage changes

🚀 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.

@daeyeon

daeyeon commented Aug 6, 2026

Copy link
Copy Markdown
Member

Please add the Signed-off-by trailer to your commit message to pass the linter check.

@daeyeon

daeyeon commented Aug 6, 2026

Copy link
Copy Markdown
Member

Thanks for the contribution. I missed this during my initial review. Duplicate of #65039.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants