esm: remove proxy for builtin exports #29737

bmeck · 2019-09-27T17:15:32Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This removes the proxy from builtin modules used to sync named exports. Instead, a manual call to require('module').syncESMExports() will sync all the builtin exports except default properties. The semantics need documentation updated, but wanted to open this to make sure the semantics are the expected ones and make sure test changes match expectations.

CC: @nodejs/modules

guybedford

Thanks so much for getting to this one. As discussed in the last meeting this seems a great way to avoid any potential perf issues from the proxies.

We should probably include a documentation note on the syncESMExports API in module.md.

lib/internal/modules/esm/translators.js

lib/internal/modules/cjs/loader.js

jkrems

I think this is great! Thanks for working on it. It looks pretty close to something we can ship as-is, one minor bikeshed on "sync all" method name aside.

jkrems · 2019-09-27T17:25:40Z

lib/internal/bootstrap/loaders.js


-  this.exports = new Proxy(this.exports, handler);
+  // const handler = {


I assume this code will be removed before shipping?

yes, cleanup and docs are pending semantic reviews

Sounds good, I'll leave this thread as a reminder.

@jkrems should be good for another review

lib/internal/modules/esm/translators.js

lib/internal/modules/cjs/loader.js

guybedford

LGTM

ljharb · 2019-09-27T18:00:32Z

Does this mean that live bindings no longer work? Or is this just an alternate approach with the same runtime semantics but improved performance?

jkrems · 2019-09-27T18:02:18Z

@ljharb It means live bindings are no longer implicit. If an instrumentation library wants to mutate http.request and have it show up in import { request } from 'http', it now has to call .syncESMExports afterwards. It felt like a valid trade-off between advanced users wanting to mess with internals and performance.

ljharb · 2019-09-27T18:07:50Z

Gotcha, thanks for clarifying.

ljharb · 2019-09-27T18:25:02Z

Instead of this, or a Proxy, have setters been considered?

bmeck · 2019-09-27T18:27:45Z

@ljharb setters would suffer same perf problem which was the issue in #29426 also it wouldn't cover cases like deletes or defineProperty

devsnek

👀

jdalton · 2019-09-27T19:39:43Z

Is there a performance problem in the wild for reference?

bmeck · 2019-09-27T19:42:32Z

@jdalton this partly based upon #29426 , which saw slowdown on process.nextTick, if modules are enabled even if they are not used; it shows up in most benchmarks that spin the event loop.

jdalton · 2019-09-27T19:48:15Z

I'd be curious to know what the slowdown is (how dramatic in %) and if it's something we should direct to V8. They've been doing a lot of work on optimizing Proxies.

I think being explicit for core is fine. Maybe not so user-friendly but user-land is there in most cases to smooth things over as needed.

mcollina · 2019-09-27T22:33:57Z

There is multiple evidence that any slowdown in process.nextTick will slow down http servers for example. Node 12.10.0 had a 30% regression in http throughput because of changes in process.nextTick. Adding any intermediate layer there is not possible.

Note that this applies to process.nextTick and some other few hot paths in Node core. As a result, we cannot have the global process object be a Proxy.

mcollina

LGTM

jdalton · 2019-09-27T22:54:04Z

Is this design premature optimization then since the nextTick scenario is one of a small group of super hot areas?

mcollina · 2019-09-28T06:47:53Z

@jdalton my point of view is expressed in #29426 (comment).

nodejs-github-bot · 2019-09-30T01:52:15Z

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

guybedford

I notice that syncExports doesn't re-sync the default export of the module.exports value is overridden somehow. Not sure if this is possible, but it could be worth ensuring this for definite?

guybedford · 2019-10-04T17:41:44Z

Just had a brief chat with @bmeck about this and we're both happy with the state of the PR to merge. Removing the WIP flag.

nodejs-github-bot · 2019-10-04T18:12:52Z

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

nodejs-github-bot · 2019-10-05T19:15:40Z

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

Trott

tools/doc/README.md should not change. The REPLACEME example in that doc is correct.

tools/doc/README.md

Trott · 2019-10-05T20:36:29Z

The CI output indicates that the REPLACEME problem is in modules.md so it is almost certainly introduced in this PR.

doc/api/modules.md

nodejs-github-bot · 2019-10-05T22:22:58Z

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

guybedford · 2019-10-05T22:23:32Z

@Trott we should be ready to land here finally.

guybedford · 2019-10-05T22:56:43Z

Windows CI failure seems unrelated.

requested change made

nodejs-github-bot · 2019-10-05T23:49:30Z

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

Trott · 2019-10-05T23:51:12Z

Windows CI failure seems unrelated.

When you're looking at a node-test-pull-request result, the "Resume Build" option in the left nav will create a new job that retains all the green results and only re-runs the yellow and green results. Useful for this situation! (And not to be confused with "Rebuild" which will re-run everything.) I just used it (and the bot posted the link right above).

PR-URL: #29737 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

guybedford · 2019-10-06T00:12:05Z

@Trott thanks for the suggestion and quick update... will definitely use that one next time.

Landed in 0b495a8.

guybedford · 2019-10-06T00:49:04Z

Merging this at the same time as the source maps PR it seems we might have an interaction causing a failure. Looking into a fix PR now, but if I can't get something soon will post a revert.

guybedford · 2019-10-06T00:56:31Z

So far haven't been able to replicate locally. #29846 is passing against the same master, so it may just be a Travis thing... let's see how the other commits on master go too.

Trott · 2019-10-06T01:25:26Z

Here's a CI job I just kicked off against master: https://ci.nodejs.org/job/node-daily-master/1700/

Trott · 2019-10-06T01:41:27Z

Here's a CI job I just kicked off against master: https://ci.nodejs.org/job/node-daily-master/1700/

@guybedford That CI job is looking very good. I think you're in the clear.

guybedford · 2019-10-06T01:47:31Z

Yes it seems the Travis build doesnt do well with merges close together, but all looks good now.

…

On Sat, Oct 5, 2019 at 21:41 Rich Trott ***@***.***> wrote: Here's a CI job I just kicked off against master: https://ci.nodejs.org/job/node-daily-master/1700/ @guybedford <https://github.com/guybedford> That CI job is looking very good. I think you're in the clear. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29737?email_source=notifications&email_token=AAESFSXUDO2RQZKDKPRTSP3QNE65DA5CNFSM4I3I2BZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAN7QTI#issuecomment-538703949>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAESFSTLBWGCMYPD7UJNZ3LQNE65DANCNFSM4I3I2BZA> .

PR-URL: #29737 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

esm: remove proxy for builtin exports

41b5cc9

bmeck added wip Issues and PRs that are still a work in progress. 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 27, 2019

bmeck requested review from guybedford, jkrems and devsnek September 27, 2019 17:15

guybedford reviewed Sep 27, 2019

View reviewed changes

lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

jkrems reviewed Sep 27, 2019

View reviewed changes

reviews

a961763

guybedford approved these changes Sep 27, 2019

View reviewed changes

bmeck mentioned this pull request Sep 27, 2019

esm: globals should be wrapped to stay in sync with ESM #29426

Closed

2 tasks

docs

6c49aa3

devsnek approved these changes Sep 27, 2019

View reviewed changes

jkrems approved these changes Sep 27, 2019

View reviewed changes

mcollina approved these changes Sep 27, 2019

View reviewed changes

JungMinu approved these changes Sep 28, 2019

View reviewed changes

benjamingr approved these changes Sep 28, 2019

View reviewed changes

guybedford reviewed Oct 4, 2019

View reviewed changes

guybedford removed modules-agenda Issues and PRs to discuss during the meetings of the Modules team. wip Issues and PRs that are still a work in progress. labels Oct 4, 2019

guybedford mentioned this pull request Oct 4, 2019

v8 Synthetic Modules Implementation #29846

Closed

3 tasks

Trott previously requested changes Oct 5, 2019

View reviewed changes

tools/doc/README.md Outdated Show resolved Hide resolved

Trott reviewed Oct 5, 2019

View reviewed changes

doc/api/modules.md Outdated Show resolved Hide resolved

fixup replaceme

089de0b

guybedford force-pushed the manual-sync-named-exports branch from b03bad2 to 089de0b Compare October 5, 2019 21:26

guybedford requested a review from Trott October 5, 2019 21:32

guybedford closed this Oct 6, 2019

BridgeAR mentioned this pull request Oct 10, 2019

v12.12.0 proposal #29919

Merged

bmeck mentioned this pull request Jan 31, 2020

Why aren’t built‑in ESM exports synced automatically? nodejs/modules#481

Closed


		this.exports = new Proxy(this.exports, handler);
		// const handler = {

esm: remove proxy for builtin exports #29737

esm: remove proxy for builtin exports #29737

Conversation

bmeck commented Sep 27, 2019 • edited Loading

Checklist

guybedford left a comment

Choose a reason for hiding this comment

jkrems left a comment

Choose a reason for hiding this comment

jkrems Sep 27, 2019

Choose a reason for hiding this comment

bmeck Sep 27, 2019

Choose a reason for hiding this comment

jkrems Sep 27, 2019

Choose a reason for hiding this comment

bmeck Sep 27, 2019

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

ljharb commented Sep 27, 2019

jkrems commented Sep 27, 2019

ljharb commented Sep 27, 2019

ljharb commented Sep 27, 2019

bmeck commented Sep 27, 2019 • edited Loading

devsnek left a comment

Choose a reason for hiding this comment

jdalton commented Sep 27, 2019

bmeck commented Sep 27, 2019 • edited Loading

jdalton commented Sep 27, 2019 • edited Loading

mcollina commented Sep 27, 2019

mcollina left a comment

Choose a reason for hiding this comment

jdalton commented Sep 27, 2019

mcollina commented Sep 28, 2019

nodejs-github-bot commented Sep 30, 2019

guybedford left a comment

Choose a reason for hiding this comment

guybedford commented Oct 4, 2019

nodejs-github-bot commented Oct 4, 2019

nodejs-github-bot commented Oct 5, 2019

Trott left a comment

Choose a reason for hiding this comment

Trott commented Oct 5, 2019

nodejs-github-bot commented Oct 5, 2019

guybedford commented Oct 5, 2019

guybedford commented Oct 5, 2019

nodejs-github-bot commented Oct 5, 2019

Trott commented Oct 5, 2019

guybedford commented Oct 6, 2019

guybedford commented Oct 6, 2019

guybedford commented Oct 6, 2019

Trott commented Oct 6, 2019

Trott commented Oct 6, 2019

guybedford commented Oct 6, 2019 via email

bmeck commented Sep 27, 2019 •

edited

Loading

bmeck commented Sep 27, 2019 •

edited

Loading

bmeck commented Sep 27, 2019 •

edited

Loading

jdalton commented Sep 27, 2019 •

edited

Loading