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

Fast path for CommonJS on Module._compile #139

Open
H4ad opened this issue Nov 28, 2023 · 6 comments
Open

Fast path for CommonJS on Module._compile #139

H4ad opened this issue Nov 28, 2023 · 6 comments

Comments

@H4ad
Copy link
Member

H4ad commented Nov 28, 2023

In theory, we can move the load of content of a module to C++ when:

  • the experimental policy is not enabled
  • and when the module is not ESM
  • and when the module is not patched

Since the content goes directly to the C++ without modification:


Also, I don't have any idea if worth or is viable but maybe we could use StreamedSource instead of Source, in this way, we probably can save some memory and load the script much faster.

Let me know what you think, this is something that was in my mind for a while and I just wanna to share it with more people to understand if is possible or not.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 28, 2023

Pinging @nodejs/loaders

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Nov 28, 2023

In general we've been treating the CommonJS loader as legacy that is frozen and not to be updated, because we don't have the resources to debug any issues that may result from changes there. No one on the current loaders/modules team has worked with the CommonJS loader, as it hasn't had updates (other than parts shared with the ESM loader) in years.

The ESM loader is much more ripe for optimizations, if I can encourage you to focus there.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 14, 2023

I don't think we've ever agreed that CommonJS loader is a legacy. It is frozen, but only because of the impact caused by compatibility. Many CLI tools (e.g. eslint, npm and tsc) still choose to bundle their sources as a CommonJS bundle (with parhaps one or more CommonJS entry points) to speed up start up time, so optimizations of the CommonJS loader would benefit the ecosystem through these popular apps that still use CommonJS.

No one on the current loaders/modules team has worked with the CommonJS loader, as it hasn't had updates (other than parts shared with the ESM loader) in years.

I don't think this is true, looking at the git log of lib/internal/modules/cjs/ I see >10 updates for SEA, policy and the permission model in just the past year (and in the year before, updates for --watch), all from people outside of the loaders/modules team. I think the state is just that the modules/loaders team do not possess any more expertise on the CommonJS loader than other contributors, since those teams were created specifically for ESM and the CommonJS loader is not in their scope. The CommonJS loader is a long-time resident that people are just more familiar with than the ESM loader, it's also much easier to hack on for contributors not familiar with any loader due to its smaller size and lower complexity IMO (its inner working is also widely documented on the Internet via various deep-dive blog posts).

@arcanis
Copy link

arcanis commented Dec 14, 2023

In theory, we can move the load of content of a module to C++ when:

  • the fs module isn't patched (virtual filesystems)

@H4ad
Copy link
Member Author

H4ad commented Dec 19, 2023

@arcanis You mean if we move the loading of the content to the C++, we will not be able to patch fs to support virtual filesystems?

If this is true, I think we already broke the support for virtual filesystems by moving the package reader to C++ (during module resolution if I remember correctly).

@arcanis
Copy link

arcanis commented Dec 19, 2023

If this is true, I think we already broke the support for virtual filesystems by moving the package reader to C++ (during module resolution if I remember correctly).

Yes, and it had to be fixed.

It's fine to use faster primitives, but there should be a way for virtual filesystem to still affect them. Ideally that would be through a blessed API, perhaps similar to the loader hooks, and failing that through discouraged access points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants