Skip to content
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

esm: improve getFormatOfExtensionlessFile perf. #49965

Merged

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Sep 29, 2023

Move openSync, readSync and closeSync logic to C++ and return int for performance.

cc @nodejs/performance we can use a similar approach to this pull request on other places in node.js core.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 29, 2023
@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 29, 2023

const { closeSync, openSync, readSync } = require('fs');
const { getValidatedPath } = require('internal/fs/utils');
const pathModule = require('path');
Copy link
Member

Choose a reason for hiding this comment

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

We generally call this path.

Suggested change
const pathModule = require('path');
const path = require('path');

Comment on lines +49 to +51
const path = pathModule.toNamespacedPath(getValidatedPath(url));

switch (fsBindings.getFormatOfExtensionlessFile(path)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const path = pathModule.toNamespacedPath(getValidatedPath(url));
switch (fsBindings.getFormatOfExtensionlessFile(path)) {
const filePath = pathModule.toNamespacedPath(getValidatedPath(url));
switch (fsBindings.getFormatOfExtensionlessFile(filePath)) {

@arcanis
Copy link
Contributor

arcanis commented Sep 29, 2023

cc @nodejs/performance we can use a similar approach to this pull request on other places in node.js core.

And break places that rely on patching the fs module to intercept I/O (for example virtual filesystems). That's relatively common.

@anonrig
Copy link
Member Author

anonrig commented Sep 29, 2023

cc @nodejs/performance we can use a similar approach to this pull request on other places in node.js core.

And break places that rely on patching the fs module to intercept I/O (for example virtual filesystems). That's relatively common.

Would you mind adding those usages/packages to CITGM? I have no information regarding such usage.

@Qard
Copy link
Member

Qard commented Sep 29, 2023

If virtual filesystems are a need then we should be explicit about such support. Patching is an unreliable approach.

@arcanis
Copy link
Contributor

arcanis commented Sep 29, 2023

Would you mind adding those packages to CITGM? I have no information regarding such usage.

A significant amount of time and efforts has been put into it, but your CI is a little unstable at times, and having to ask a manual run after every workaround has added a lot of friction. Would be worth resurrecting, though.

@arcanis
Copy link
Contributor

arcanis commented Sep 29, 2023

If virtual filesystems are a need then we should be explicit about such support. Patching is an unreliable approach.

I'm all for that (it's something that's been discussed from time to time with loaders / customizations, or the SEA initiative), but until there is, I don't think it should be broken. Same thing as breaking the CJS patching, really.

@CanadaHonk
Copy link
Member

In many PRs (open and merged) sync FS ops are moving from using internal js funcs to being in pure C++ for more performance. Shouldn't this also be a concern with them if so or do VFS patches only care about async?

@arcanis
Copy link
Contributor

arcanis commented Sep 29, 2023

In many PRs (open and merged) sync FS ops are moving from using internal js funcs to being in pure C++ for more performance. Shouldn't this also be a concern with them if so or do VFS patches only care about async?

Changing the internals of the public fs methods is fine, since patches replace those methods (or delegate to the original implementation after checking their parameters). What's more problematic is having internal Node.js utilities (like in this PR) completely stop using the fs APIs, with no way to intercept their calls.

For example, if we make a require('fs').getFormatOfExtensionlessFile, it's fine (although it admittedly makes public something that's intended to be an implementation detail). If you make a internalBinding('fs').getFormatOfExtensionlessFile, that's a problem (at least it is if you think patching is bad - because then patchers will have to attempt to patch the internal bindings, using even darker magic...).

@GeoffreyBooth
Copy link
Member

Monkey-patching is not something that Node supports, and should be a minimal consideration (if any) as part of implementing Node features. If tools want customization hooks for filesystem operations, then put up PRs to create those hooks, along the lines of the module customization hooks API. We could even abstract the “run the hooks in a separate thread” stuff from the module customization hooks so that hooks for other systems also run in a separate thread, so that hooks can do async stuff within sync flows.

I also disagree that it’s on the Node core team to create the hooks first and wait for the ecosystem to migrate, and support monkey-patching in the interim. We’ve never supported monkey-patching; it’s not part of our public API. We would be happy to support something official, a “filesystem customization hooks” similar to the module ones, if people want to put in the effort to create it. This should not impede performance improvements that will benefit everyone in the short term.

@arcanis
Copy link
Contributor

arcanis commented Sep 29, 2023

I'm not really looking to discuss the fine frontier between "officially supported" vs "it just so happened to work for the past 10+ major releases". My point is simply that this change, as is, is breaking existing scenarios, period. It doesn't have to (as I mentioned the new fs primitive could be exposed somewhere), so I hope you can take that into consideration.

@GeoffreyBooth
Copy link
Member

this change, as is, is breaking existing scenarios

Getting the format of a loaded module is already covered by the module customization hooks. If overriding the behavior of this new API is desired, it can already be done within the load hook.

@Qard
Copy link
Member

Qard commented Sep 29, 2023

I agree we should try to avoid breaking existing cases, but the task should not be "do nothing" it should be to identify cases like that and reach out to come up with better solutions or direct them to using existing better solutions. Stability is important, but we also need to ensure we're not blocking ourselves from being able to make improvements in the future.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 30, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Lgtm

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Agree with the sentiment, happy for Node to provide hooks into APIs if interception is desirable.

As a long time project contributor/member it could be great if @arcanis can drive an effort to identify those requirements though I understand it if they don't have time.

Comment on lines +30 to +31
#define EXTENSIONLESS_FORMAT_JAVASCRIPT (0)
#define EXTENSIONLESS_FORMAT_WASM (1)
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT these are going to be exposed as enumerable properties of fs.constants in userland?

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 1, 2023
@nodejs-github-bot nodejs-github-bot merged commit 05be31d into nodejs:main Oct 1, 2023
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 05be31d

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49965
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49965
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49965
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants