Skip to content

Conversation

@mertcanaltin
Copy link
Member

I tried to prevent the call from being repeated

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jan 14, 2026
@codecov
Copy link

codecov bot commented Jan 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.53%. Comparing base (8365edc) to head (cc8df6b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #61387   +/-   ##
=======================================
  Coverage   88.53%   88.53%           
=======================================
  Files         704      704           
  Lines      208849   208849           
  Branches    40324    40333    +9     
=======================================
+ Hits       184903   184909    +6     
+ Misses      15925    15919    -6     
  Partials     8021     8021           
Files with missing lines Coverage Δ
src/env-inl.h 94.45% <100.00%> (ø)
src/env.h 97.95% <ø> (ø)
src/node_realm-inl.h 88.67% <100.00%> (+0.44%) ⬆️
src/node_realm.cc 74.85% <ø> (-0.30%) ⬇️
src/node_realm.h 100.00% <ø> (ø)

... and 28 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.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

You can also move Realm::context() into node_realm-inl.h so that the compiler can already make this optimization itself.

@mertcanaltin
Copy link
Member Author

You can also move Realm::context() into node_realm-inl.h so that the compiler can already make this optimization itself.

thanks, I used

@mertcanaltin mertcanaltin requested a review from addaleax January 15, 2026 19:18
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Be sure to update the commit message/PR title here as well

return isolate_;
}

inline v8::Local<v8::Context> Realm::context() const {
Copy link
Member

Choose a reason for hiding this comment

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

Adding inline here isn't that important, but it should be applied in node_realm.h, next to virtual.

And since this is a virtual method, I think you'll want to mark the PrincipalRealm and ShadowRealm classes as final as well, otherwise the compiler won't be able to know that there's no subclass which overrides the method (in other words, otherwise the compiler won't know that the method can actually be inlined)

Copy link
Member

Choose a reason for hiding this comment

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

(and make it so Environment::principal_realm() returns a PrincipalRealm* pointer rather than a Realm* pointer – seems compilers can't always see through the cast that currently happens in that method)

Copy link
Member Author

Choose a reason for hiding this comment

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

done!, thanks I changed commit slug fs to src

@mertcanaltin mertcanaltin force-pushed the mert/fs-cache-context-in-vectored-io branch 2 times, most recently from 1ed8448 to 3d040d1 Compare January 17, 2026 15:43
@mertcanaltin mertcanaltin force-pushed the mert/fs-cache-context-in-vectored-io branch from b673036 to cc8df6b Compare January 17, 2026 15:48
@mertcanaltin mertcanaltin changed the title fs: cache context lookup in vectored io loops src: cache context lookup in vectored io loops Jan 17, 2026
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++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants