esm: improve `getFormatOfExtensionlessFile` perf. #49965

anonrig · 2023-09-29T18:25:45Z

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 · 2023-09-29T18:25:50Z

Review requested:

@nodejs/loaders

GeoffreyBooth · 2023-09-29T18:42:15Z

lib/internal/modules/esm/formats.js

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


We generally call this path.

Suggested change

const pathModule = require('path');

const path = require('path');

GeoffreyBooth · 2023-09-29T18:42:59Z

lib/internal/modules/esm/formats.js

+  const path = pathModule.toNamespacedPath(getValidatedPath(url));
+
+  switch (fsBindings.getFormatOfExtensionlessFile(path)) {


Suggested change

const path = pathModule.toNamespacedPath(getValidatedPath(url));

switch (fsBindings.getFormatOfExtensionlessFile(path)) {

const filePath = pathModule.toNamespacedPath(getValidatedPath(url));

switch (fsBindings.getFormatOfExtensionlessFile(filePath)) {

arcanis · 2023-09-29T18:50:21Z

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 · 2023-09-29T18:52:28Z

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 · 2023-09-29T19:00:38Z

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

arcanis · 2023-09-29T19:00:59Z

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 · 2023-09-29T19:03:09Z

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 · 2023-09-29T19:11:02Z

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 · 2023-09-29T19:20:02Z

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 · 2023-09-29T19:51:43Z

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 · 2023-09-29T20:19:08Z

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 · 2023-09-29T20:24:45Z

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 · 2023-09-29T20:42:15Z

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.

nodejs-github-bot · 2023-09-30T01:49:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54389/

mcollina

Lgtm

benjamingr

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.

LiviaMedeiros · 2023-09-30T17:23:44Z

src/node_constants.h

+#define EXTENSIONLESS_FORMAT_JAVASCRIPT (0)
+#define EXTENSIONLESS_FORMAT_WASM (1)


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

nodejs-github-bot · 2023-10-01T18:32:09Z

Landed in 05be31d

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>

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>

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>

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 force-pushed the optimize-extensionless-paths branch from 20d92eb to 19caf5d Compare September 29, 2023 18:26

anonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 29, 2023

anonrig force-pushed the optimize-extensionless-paths branch from 19caf5d to 071f97f Compare September 29, 2023 18:28

esm: improve getFormatOfExtensionlessFile speed

15bc0f1

anonrig force-pushed the optimize-extensionless-paths branch from 071f97f to 15bc0f1 Compare September 29, 2023 18:31

GeoffreyBooth approved these changes Sep 29, 2023

View reviewed changes

Qard approved these changes Sep 29, 2023

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023

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

mcollina approved these changes Sep 30, 2023

View reviewed changes

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

benjamingr approved these changes Sep 30, 2023

View reviewed changes

LiviaMedeiros reviewed Sep 30, 2023

View reviewed changes

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 merged commit 05be31d into nodejs:main Oct 1, 2023
58 checks passed

RafaelGSS mentioned this pull request Oct 9, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: improve `getFormatOfExtensionlessFile` perf. #49965

esm: improve `getFormatOfExtensionlessFile` perf. #49965

anonrig commented Sep 29, 2023 •

edited

nodejs-github-bot commented Sep 29, 2023

GeoffreyBooth Sep 29, 2023

GeoffreyBooth Sep 29, 2023

arcanis commented Sep 29, 2023

anonrig commented Sep 29, 2023 •

edited

Qard commented Sep 29, 2023

arcanis commented Sep 29, 2023 •

edited

arcanis commented Sep 29, 2023

CanadaHonk commented Sep 29, 2023

arcanis commented Sep 29, 2023

GeoffreyBooth commented Sep 29, 2023

arcanis commented Sep 29, 2023

GeoffreyBooth commented Sep 29, 2023

Qard commented Sep 29, 2023

nodejs-github-bot commented Sep 30, 2023

mcollina left a comment •

edited

benjamingr left a comment

LiviaMedeiros Sep 30, 2023

nodejs-github-bot commented Oct 1, 2023

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

		const path = pathModule.toNamespacedPath(getValidatedPath(url));

		switch (fsBindings.getFormatOfExtensionlessFile(path)) {

		#define EXTENSIONLESS_FORMAT_JAVASCRIPT (0)
		#define EXTENSIONLESS_FORMAT_WASM (1)

esm: improve getFormatOfExtensionlessFile perf. #49965

esm: improve getFormatOfExtensionlessFile perf. #49965

Conversation

anonrig commented Sep 29, 2023 • edited

nodejs-github-bot commented Sep 29, 2023

GeoffreyBooth Sep 29, 2023

Choose a reason for hiding this comment

GeoffreyBooth Sep 29, 2023

Choose a reason for hiding this comment

arcanis commented Sep 29, 2023

anonrig commented Sep 29, 2023 • edited

Qard commented Sep 29, 2023

arcanis commented Sep 29, 2023 • edited

arcanis commented Sep 29, 2023

CanadaHonk commented Sep 29, 2023

arcanis commented Sep 29, 2023

GeoffreyBooth commented Sep 29, 2023

arcanis commented Sep 29, 2023

GeoffreyBooth commented Sep 29, 2023

Qard commented Sep 29, 2023

nodejs-github-bot commented Sep 30, 2023

mcollina left a comment • edited

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

LiviaMedeiros Sep 30, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 1, 2023

esm: improve `getFormatOfExtensionlessFile` perf. #49965

esm: improve `getFormatOfExtensionlessFile` perf. #49965

anonrig commented Sep 29, 2023 •

edited

anonrig commented Sep 29, 2023 •

edited

arcanis commented Sep 29, 2023 •

edited

mcollina left a comment •

edited