-
Notifications
You must be signed in to change notification settings - Fork 29.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fs: add fast api for InternalModuleStat
#51344
fs: add fast api for InternalModuleStat
#51344
Conversation
Review requested:
|
src/node_file.cc
Outdated
if (UNLIKELY(!env->permission()->is_granted( | ||
permission::PermissionScope::kFileSystemRead, path))) { | ||
options.fallback = true; | ||
return -1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking about this, if this is true, it will:
- ignore the value -1.
- re-run
InternalModuleStat
but in the slower version. - throw error since the permission is not granted.
I don't know if worthy but maybe one test just to ensure this behavior, maybe a loop and then pass a value that is not allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test (6 months later)
Failing because of the Node version: https://github.com/nodejs/node/actions/runs/7390448695/job/20105393172?pr=51344#step:6:5719 Who we should ping to solve this kind of issue? |
cc @nodejs/build @nodejs/npm Any suggestions on how to fix the error? |
There are two separate items in that log. First is a warning that will always show up when you're using a prerelease version of node. This is intentional to remind folks that they're in an unsupported prerelease version of node. npm still will install just fine, the part of npm that prevents itself from being installed globally in an unsupported node version throws a different exception and allows prereleases in its semver check against npm's engines field. The second error has to do with a missing file that is being required. As to why that file is missing, that's another mystery. It's definitely in the tarball of the published module, and in source control. |
The error must be related to the changes made in this PR. In other words, there's a bug in the implementation. |
This needs a rebase. |
11cea33
to
3f32a24
Compare
3f32a24
to
9b6a9f8
Compare
9b6a9f8
to
1a71169
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
is this improving startup time in any way?
I don't know, but better ask our friend "benchmark ci": https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1570/ |
|
Landed in 1aa8a78 |
PR-URL: #51344 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#51344 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#51344 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Adds V8 Fast API to
InternalModuleStat