Reduce the promise overhead in the ESM loader #50900
Labels
loaders
Issues and PRs related to ES module loaders
performance
Issues and PRs related to the performance of Node.js.
Comments
joyeecheung
added
performance
|
@nodejs/loaders @nodejs/performance |
|
Should we move this issue to performance repository? |
|
I'd say this is more than about just performance but also about making the ESM loader generate less async activity when it's loading a synchronous graph (one of the reasons why using ESM loader to load CJS modules can fail the tests). |
|
Using #50899 The amount of promises created for an logs |
|
Almost half of the promises are created because of @GeoffreyBooth the In theory, using But trying to benchmark it in a project: I don't have a good project to benchmark it, so if someone here has any repository to test these improvements, I will be very happy to test this little change. In general, I don't think the amount of promises created is problematic, removing some of them makes the debug of this code a little bit harder since we don't know where that promise was created when an error happens (since we didn't await in the original function). |
There are several benefits from making the ESM loader sync; see #50356 and https://github.com/orgs/nodejs/discussions/45711, for starters. Sure it might get a little harder to debug, but no harder than the all-sync CommonJS loader. |
I think we talked about this previously and I thought we had made this change already. At the moment, I can't think of a reason to not do this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As a follow up of #50356, one of the problem of using the current ESM loader as the central loader is that it creates a lot of noisy promises during loading - even when loading a completely synchronous graph. For example, when loading a completely empty .mjs file, it creates 66 promises (using #50899)
See log
And when loading one extra module in the entry point it creates 125 promises in total - from my testing, every module loaded results in 59 more promises. And this is when the graph is completely synchronous (i.e. no http modules, no top-level await).
After discussion with @GeoffreyBooth we think some work should be put into this to reduce the overhead/noise. Either more work can be moved to C++ to restrict unnecessary async-await or we can carve out a synchronous path in the ESM loader to handle synchronous graphs.
Theoretically only one immediately settled promise has to be created by v8 for each synchronous module - even that one may not really have to be a promise - in the V8 API contract, as far as a synchronous graph is concerned, that promise is more of a poor-man's Maybe type rather than an actual async construct (if evaluation succeeds V8 immediately resolves it with undefined, otherwise immediately rejects). So the quest is to ideally reduce the overhead to just that one per module (even then is should be possible for a different V8 API to skip that one completely) for a synchronous graph.
The text was updated successfully, but these errors were encountered: