module: compute module.paths lazily - #51611
Conversation
Sorry, something went wrong.
Make module.paths a getter which computes the property when it's requested. The paths property is entirely deterministically computed from the path property, so it's not necessary to keep the relatively large paths array in memory. On an internal application, this reduces the used heap by around 5.5%. Furthermore (although it was not a goal of this PR) it speeds up the execution time of `vite --version` by ~2% according to hyperfine. A similar speed improvement is observed on the internal application. Notably, this introduces two minor changes which may be considered breaking. The obvious one is that `paths` is now a getter/setter and not a property. The other one is more subtle: the lazy computation means that if a user changes `mod.path = X` and then later invokes `mod.paths`, the results will reflect the updated path as opposed to the current behavior where it would reflect the original path. One way to fix the latter would be to force capturing `paths` before `path` is changed, I'm open to doing that (or some cleaner approach?) if the reviewers deem it necessary.
|
Review requested:
|
Sorry, something went wrong.
joyeecheung
left a comment
There was a problem hiding this comment.
Code-wise LGTM though we should run this through CITGM to see if we are breaking any popular packages.
Sorry, something went wrong.
| const lazyPathsGetter = { | ||
| __proto__: null, | ||
| enumerable: true, | ||
| get() { return this[kPaths] ?? Module._nodeModulePaths(this.path); }, |
There was a problem hiding this comment.
I think this could also just replace this.paths with a value property once computed so subsequent accesses are not invoking a getter again & the property type change is less observable from the user land.
Sorry, something went wrong.
|
Added the dont-land labels for LTS because of the potential for breakage mentioned in the OP. |
Sorry, something went wrong.
|
I would still like to see this happening...@kvakil are you still working on this? |
Sorry, something went wrong.
|
This needs a rebase. |
Sorry, something went wrong.
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
Sorry, something went wrong.
|
Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
Sorry, something went wrong.
Make module.paths a getter which computes the property when it's
requested. The paths property is entirely deterministically computed
from the path property, and so it's not necessary to keep the relatively
large paths array in memory.
On an internal application, this reduces the used heap by around
5.5%. Furthermore (although it was not a goal of this PR) it speeds
up the execution time of vite --version by ~2% according to
hyperfine. A similar speed improvement is observed on the internal
application.
Notably, this introduces two minor changes which may be considered
breaking. The obvious one is that paths is now a getter/setter
and not a property. The other one is more subtle: the lazy computation
means that if a user changes mod.path = X and then later invokes
mod.paths, the results will reflect the updated path as opposed
to the current behavior where it would reflect the original path.
One way to fix the latter would be to force capturing paths before
path is changed, I'm open to doing that (or some cleaner approach?)
if the reviewers deem it necessary.