perf(resolve-dependencies): avoid copying preferredVersions object #6735
Conversation
replacing object spread with a prototype chain, avoiding extra memory allocations in resolveDependencies this becomes noticeable on relatively large monorepo(~2k packages) reducing memory usage and making it slightly faster 1. allowing to complete `pnpm install` without `--max_old_space_size=8192` (it is noticeably slower, but at least works) 2. prevents OOM issue with `auto-install-peers=true` (this is crashing otherwise no matter `max_old_space_size`)
zkochan
force-pushed
the
perf-prefered-versions
branch
from
50b29bb
to
3ac24dd
Compare
| @@ -522,7 +522,7 @@ export async function resolveDependencies ( | |||
| postponedPeersResolutionQueue.push(postponedPeersResolution) | |||
| } | |||
| }) | |||
| const newPreferredVersions = { ...preferredVersions } | |||
| const newPreferredVersions = Object.create(preferredVersions) as PreferredVersions | |||
There was a problem hiding this comment.
But this means that when we replace a field in newPreferredVersions, the same field will also be replaced in preferredVersions, which is not what we want.
There was a problem hiding this comment.
hm, looks like in this case we don't override existing fields in newPreferredVersions, only adding new ones. And the new ones will not appear in preferredVersions. So I guess it should work fine.
There was a problem hiding this comment.
there is no change in this regard, when assigning the property to newPreferredVersions - it would affect only the new object created here (not a prototype of it)
the main difference is to be when reading the property - if it not found in newPreferredVersions, it would be read from the prototype (preferredVersions)
however, the opposite is true - changing properties of preferredVersions, if not overridden will be reflected on newPreferredVersions
There was a problem hiding this comment.
about the possibility of overrides in preferredVersions, just noticed in code below(for both - before and after the pr), not sure if that can cause issues:
if (!newPreferredVersions[resolvedPackage.name]) {
newPreferredVersions[resolvedPackage.name] = {}
}
if (!newPreferredVersions[resolvedPackage.name][resolvedPackage.version]) {
newPreferredVersions[resolvedPackage.name][resolvedPackage.version] = 'version'
// ^^ -- here the content of `preferredVersions[resolvedPackage.name][resolvedPackage.version]` is possibly modified
// that will happen if `resolvedPackage.name` was already in `preferredVersions` ()
}There was a problem hiding this comment.
yes, I think this should be fixed. It is probably causing some hard to reproduce issues.
There was a problem hiding this comment.
I have created an issue from your comment #6737
Let me know if you want to work on it.
|
Congrats on merging your first pull request! 🎉🎉🎉 |
…6735) replacing object spread with a prototype chain, avoiding extra memory allocations in resolveDependencies this becomes noticeable on relatively large monorepo(~2k packages) reducing memory usage and making it slightly faster 1. allowing to complete `pnpm install` without `--max_old_space_size=8192` (it is noticeably slower, but at least works) 2. prevents OOM issue with `auto-install-peers=true` (this is crashing otherwise no matter `max_old_space_size`)
Replacing object spread with a prototype chain, avoiding extra memory allocations in
resolveDependenciesThis becomes noticeable on relatively large monorepo(~2k packages) reducing memory usage and making it faster
Also in my case:
pnpm installwithout--max_old_space_size=8192(it is slower, but at least works without OOM crash)auto-install-peers=true(for me this is crashing otherwise no mattermax_old_space_size)flame graph using pprof-it, running
NODE_OPTIONS="--max_old_space_size=8192" pprof-it ../pnpm/pnpm/bin/pnpm.cjs install --offline --resolution-onlybefore:
after:
