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: globals should be wrapped to stay in sync with ESM #29426

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Sep 3, 2019

This fixes the behavior of the builtin module proxy for process so that test/parallel/test-global-setters.js works even when ESM is turned on via --experimental-modules by changing the global to use the Proxy form that ESM creates for named export syncing.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@bmeck
Copy link
Member Author

bmeck commented Sep 3, 2019

test/parallel/test-bootstrap-modules.js is still failing and we can add NativeModule process if thats ok @joyeecheung

test/parallel/test-bootstrap-modules.js adds a getter that errors to process and the sync operation of the ESM proxy errors out on that overly eagerly, should resolve to a path forward on how to handle that test @nodejs/modules

// proxify the global process to keep in sync with ESM
const mod = NativeModule.map.get('process');
mod.compileForPublicLoader(true);
globalThis.process = mod.exports;
Copy link
Member

Choose a reason for hiding this comment

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

If this is done when ESM is not even used doesn't this just slow down access to globalThis.process unconditionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, but that's how to keep ESM names synced

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.

I’m provisionally -1 as it’ll slow down access to process and it sits in a lot of hot paths (because of process.nextTick).

This needs a benchmark run on nextTick and stream at a minimum.

@bmeck
Copy link
Member Author

bmeck commented Sep 3, 2019

@mcollina the way that --experimental-modules added named exports wraps all builtin modules in Proxies, I agree there will be slowdown but the request of having named exports would still require proxies around things even outside this PR and already does so. For now the PR still is gated by the --experimental-modules flag. I think discussions on performance impact of unflagging --experimental-modules extends far outside this PR. This PR is just a bugfix essentially if --experimental-modules were to be unflagged.

@mcollina
Copy link
Member

mcollina commented Sep 4, 2019

An alternative option is not to expose static export for the process object and module, as the issue is very specific for process.

There is correlation between process.nextTick performance and Node.js baseline throughput in both http and file processing, because it's so heavily used within the whole of Node.js core. We cannot simply const { nextTick } = process because we have to support https://github.com/sinonjs/lolex and similar. As a result, making process being a proxy is not feasible. The process.nextTick case is unique in this sense, however we might have to look at EventEmitter and Streams as well.

My overall assumption about ESM is that it should not slow down normal execution, while it could slow down application startup. This PR is going to affect execution.

@MylesBorins
Copy link
Contributor

Could we internally us a non proxy instance (maybe via internal reference)?

@bmeck bmeck added esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team. labels Sep 13, 2019
GeoffreyBooth added a commit to nodejs/modules that referenced this pull request Sep 25, 2019
@guybedford
Copy link
Contributor

Shall we close this PR?

@bmeck
Copy link
Member Author

bmeck commented Sep 27, 2019

closed in favor of #29737

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants