[Bug] Extremely slow yarn run xxxx commands on large monorepo
#973
Comments
crubier
changed the title
yarn run xxxx commands on large monorepo
|
What does the |
|
The workspaces already looks like this: |
|
Hi @arcanis I am working with @crubier and @etiennedupont to migrate our repo from yarn v1 + pnp to yarn v2. Would you have any idea where we should start debugging from? Running any Our guess is that we should look into the source code of v2 using Thanks :) |
|
Hey @RaynalHugo! I took a quick look at generating a huge monorepo with 200 packages, but no luck - my run time was mostly unaffected (I used the following script inside a project configured with a for x in {1..200} ; do
(mkdir -p packages/pkg-$x && cd packages/pkg-$x && yarn init)
doneI think the best would be to use |
|
Hi @arcanis, after going through some profiling, it looks like the function that causes this delay is
So we profiled the inner call that is made through
Indeed, most of the time is spent in |
|
There seem to be two phases in the The first phase last 6 seconds (which is quite long already), with a profile like this:
The second phase is even longer, lasting 25 seconds, with very shallow stacks:
|
|
Here is the bottom up view of this profile:
The AsyncFunctionAwaitResolveClosure in resolveEverything takes all the time |
|
I suspect there's a dependency cycle somewhere that takes a long time to resolve - by any chance, could you provide the workspaces' Otherwise, I'd suggest to add |
|
We did the group thing, the stacks don't seem that deep, but the stream is neverending, there are millions of lines...
|
|
To give you some context, the way the I'm not entirely sure of the circumstances under which you'd end up with a huge tree - there's likely a mix of peer dependencies and regular dependencies that trigger a catastrophic expansion. My main guess is that something somewhere (possibly in multiple places) is listed as a regular dependency instead of being a peer dependency, causing Yarn to dig into it and generating those huge graphs. If you print |
|
We did that and ran: It gives these stats: the most imported package is |
|
No idea what is wrong in our package structure to be honest... We will try to make a fake repo to replicate... |
|
That would be perfect. Clearly Yarn should better detect and surface the problem, whatever it is, and a repro will certainly be very useful to do that! Thanks for your help 🙏 |
|
We just created this repo to reproduce our package structure. https://github.com/etiennedupont/yarn-v2-stress-test This reproduces the bug. Run The structure of the repo is similar, however our packages do not depend on that many of our other packages (our package typically depend on at most 10 of our other packages, while for example pkg-4-1 depends on 120 other packages here). But I believe this difference is not that relevant here. We will try after adjusting this but I think the conclusion would be similar. |
|
I did several tests on the repo above:
|
|
Ok, so it seems that reproduction is simple:
Then there is a combinatoric thing at work which means that When the peerDependencies are not here, the |
|
@arcanis here are our conclusions, Maybe you can:
Thank you so much! |
|
Thanks for the details! I think I know the place where we're missing an optimization (or rather, we optimize it too late in the process and the CPU is going haywire before reaching it). The solution won't be easy to implement but I think it should be doable on our side.
What's the actual size of the |
|
Here a few numbers from my latest tries: On our repo, after moving all our peerDeps to normal Deps (= basically putting react as dep instead of peerDep):
The problem is that we depend on third party packages who themselves have peerDependencies, so removing them on our side is not enough, the .pnp.js is still full of virtual resolutions. grep -o -i virtual: .pnp.js | wc -l
39058There are still 39058 instances of virtual resolutions in .pnp.js, which is not great, not terrible. What is the optimization supposed to look like? One thing is sure, .pnp.js does not grow as exponentially as the time to run the scripts. But I still think that it is too big. In particular, we paid attention to have consistent versions in our dependencies, so we have "only" 5069 packages in our transitive dependencies (5069 items in Given our numbers, I would expect an actually optimal 5069 packages * 15 deps/package * 1 line of code/package * 40 bytes/line of code = 3Mb So not too far off actually. The size of |
Conceptually we traverse and expand the whole tree, resolving all peer dependencies as we go. Then once we are done, we check which ones have been resolved to the same set of dependencies, and remove the duplicate. The problem with this approach is that even though the duplicate are trimmed they still contribute to the tree traversal, hence the catastrophic expansion problem. We should instead check for duplicate as we traverse the tree, and avoid recursing into nodes that have the same dependency sets as another virtual package we already traversed. Of course it's not as simple - the way peer dependencies work is that children mutate their direct parents, so I still need to wrap my head around that to make sure the traversal-time optimization will yield sound results in every case - I'm ~70% sure there's a weird non-obvious edge case hidden somewhere, so most of the work is to find it 😄 |
|
I see. Indeed this sounds like the way to go. Still, a question I have is what is done differently in yarn v2 as compared to yarn v1 ? On our use case, yarn v1 pnp takes around 8s to launch a script, while it takes 200s with yarn v2 and 90s with yarn v2 and no peerDeps in our repo. My question would be: Maybe it's fine to not optimize the behaviour you are talking about, but could there not be a way to "cache" the result more agressively once and for all after or during install so that these computations do not have to be run everytime we I would not mind my install time to be slightly longer, if my yarn run where instant afterwards. |
|
My message above may not be clear, so to rephrase it simply: There are two problems here:
My point is that Problem 1 is not a big deal, but Problem 2 is the actual showstopper here. Solving Problem 2 would make Problem 1 irrelevant. Ideally, nothing else than reading the So my question would be: Is there anything preventing solving problem 2? Is it strictly necessary that some resolution has to be done at runtime rather than install time? In other words is it impossible to serialize in a JS file the full dependency tree including resolved peerDependencies? |
|
Yep, I'd like to improve the The reason why we have the resolution pass is that otherwise we would need to store all the virtual packages not only inside the Maybe we could try again with a better format ... possibly by doing something like this: Still, it's quite a lot of data. An alternative would be to store it inside a separate file (
If I remember correctly, the v1 implementation was far less correct than the v2 - in particular, I think that postinstall scripts executed inside the context of a virtual package were losing access to their peer dependencies, because Yarn wasn't able to detect which variant was being built. |
|
I didn't know humans were supposed to review Indeed, I think yarn v2 is actually the first-ever fully-correct package manager and this is a big deal and much appreciated. The solution of having So conclusion: I give a big 👍 to the |
|
@arcanis regarding the
Best |
For internal lockfiles it's not so much required, but for open-source ones it's important to make sure the resolutions point to the expected location (otherwise I could make a PR adding lodash, except that the lockfile would actually point to
I think I need to work on the algorithm performances first, because it's less likely someone else will be able to efficiently navigate this part of the code. Given my other wips, I'd say at least a few weeks before I can implement it - so any help would be much appreciated! 😃
It would probably be better to implement this in the core - it's unlikely someone would want a different implementation, so a hook just for that would be awkwardly specific. Plus the experience won't be worse because of it, so better ship it by default.
I don't think it would be very complex - basically what I think would need to be done on the top of my head:
|
|
Ok I'm giving it a shot there #990 I just have one question: What should I persist in the virtual state file?The |
|
When I think about it, I still don't understand why |
|
First, a bit of context: our infrastructure is made to support JavaScript packages, but not exclusively. In the future, we want to implement support for installing packages from other languages. To do that, we've separated the core (the lockfile) from the linkers (PnP linker, n_m linker). This separation means that the core isn't allowed to directly speak to the
A bit of all 🙂 Basically, // The descriptor is a "package name"+"range" combination
declare const sourceDescriptor: Descriptor;
const resolvedPackage = project.storedPackages.get(
project.storedResolutions.get(
sourceDescriptor.descriptorHash,
),
);As you can see in |
|
Ok thank you that was really helpful: I think I managed to generate the correct I am now working on how to read this file during By something like: And the |
Yep that's a good first iteration. Later I think we'll want to optimize this format so that your example would end up being: "debug@virtual:81392a248a4eb1062284166bc5b126ea9dbc003b67ae1ded6b6a4bf4e58d3ad86c80934ef78b5b059394fa3051f64f2a5781fbc893f31c635c098e2c47b576c1#npm:^4.1.0":
virtualOf: "debug@npm:4.1.1"
"pkg-1-1@virtual:748f6670d9c6c066fb88aa3380f1d90d0e995ab209cda0c2465335227bd22f406f698d521c4c284ecb1f407affa50215728d2ff86737a8095b0baabe0fe59409#workspace:packages/pkg-1-1":
virtualOf: "pkg-1-1@workspace:packages/pkg-1-1"
peerResolutions:
react: "npm:^16.12.0"
react-dom: "virtual:748f6670d9c6c066fb88aa3380f1d90d0e995ab209cda0c2465335227bd22f406f698d521c4c284ecb1f407affa50215728d2ff86737a8095b0baabe0fe59409#npm:^16.12.0"
Yep!
Pretty much. The key difference between The most complex thing here is that you can't just copy-paste for (const [virtualEntryName, virtualEntry] of Object.values(virtualStateFileData)) {
const virtualLocator = structUtils.parseLocator(virtualEntryName);
const virtualDescriptor = structUtils.convertLocatorToDescriptor(virtualLocator);
const originalLocator = structUtils.parseLocator(virtualEntry.virtualOf);
const originalPackage = this.originalPackages.get(originalLocator.locatorHash);
const virtualPackage = structUtils.renamePackage(originalPackage, virtualLocator);
for (const [name, resolution] of Object.entries(virtualEntry.peerResolutions))
virtualPackage.dependencies.set(name, structUtils.parseDescriptor(resolution).descriptorHash);
this.storedPackages.set(virtualPackage.locatorHash, virtualPackage);
this.storedDescriptors.set(virtualDescriptor.descriptorHash, descriptor);
this.storedResolutions.set(virtualDescriptor.descriptorHash, virtualPackage.locatorHash);
} |
|
Ok, updated #990 , now generating the correct file (I think, not quite sure, to be checked, but it looks like your example, at least superficially.) Now trying the read part. |
|
I made progress, but I think I am blocked now. Running Here is an example run of I print the number of entries in @Command.Path(`run`)
async execute() {
const configuration = await Configuration.find(this.context.cwd, this.context.plugins);
const {project, workspace, locator} = await Project.find(configuration, this.context.cwd);
console.time("hydrateVirtualPackages");
await project.hydrateVirtualPackages();
console.timeEnd("hydrateVirtualPackages");
console.log(project.storedPackages.size);
console.time("resolveEverything");
await project.resolveEverything({
lockfileOnly: true,
report: new ThrowReport(),
});
console.timeEnd("resolveEverything");
console.log(project.storedPackages.size);
....I guess I should also run part of what is done in |
|
Getting there... slowly. I added a first phase in async hydrateVirtualPackages() {
const virtualStatePath = ppath.join(this.cwd, this.configuration.get(`virtualStateFilename`));
const content = await xfs.readFilePromise(virtualStatePath, `utf8`);
const virtualStateFileData: {[key:string]:{virtualOf:string,peerResolutions:{[key:string]:string}}} = parseSyml(content);
for (const [locatorHash, pkg] of this.originalPackages.entries()) {
const desc = structUtils.convertLocatorToDescriptor(pkg);
this.storedPackages.set(pkg.locatorHash, pkg);
this.storedDescriptors.set(desc.descriptorHash, desc);
this.storedResolutions.set(desc.descriptorHash, pkg.locatorHash);
}
for (const [virtualEntryName, virtualEntry] of Object.entries(virtualStateFileData)) {
if (virtualEntryName === `__metadata`)
continue;
const virtualLocator = structUtils.parseLocator(virtualEntryName);
const virtualDescriptor = structUtils.convertLocatorToDescriptor(virtualLocator);
const originalLocator = structUtils.parseLocator(virtualEntry.virtualOf);
const originalPackage = this.originalPackages.get(originalLocator.locatorHash);
if (!originalPackage)
throw new Error("Wowowowow could not find original package");
const virtualPackage = structUtils.renamePackage(originalPackage, virtualLocator);
for (const [name, resolution] of Object.entries(virtualEntry.peerResolutions))
virtualPackage.dependencies.set(name as IdentHash, structUtils.parseDescriptor(resolution));
this.storedPackages.set(virtualPackage.locatorHash, virtualPackage);
this.storedDescriptors.set(virtualDescriptor.descriptorHash, virtualDescriptor);
this.storedResolutions.set(virtualDescriptor.descriptorHash, virtualPackage.locatorHash);
}
}I now have roughly the right number of packages after hydrateVirtualPackages: ( 3087 ), but I still get an error later on when runnnig |
|
@arcanis it looks like the result I get with My question would be: Why not run |
|
I've opened #997 which aims to fix the virtual dependencies atrocious speed. Using your repro repository with parameters of 10-50-50-50-10, I got 1.18s of resolution time 🌟 Can you confirm you get the same results on your private repo? |
|
@arcanis Yes indeed I confirm that it is really effective! On the repro repository I get around 2s resolution time! Much better! On our own internal repo, I get around 5s resolution time instead of 200s, so this is night and day! But this 5s resolution time on our repo is still around 5x slower than yarn v1 which was less than 1s. Hence I think we still need #998 (if it has a chance to work) as explained there #990 (comment) Combining these two MRs will make yarn v2 actually super useful for people with large monorepos |
The bug was explained there by my colleague @etiennedupont , but maybe it should be reported here as it concerns v2 ?
https://github.com/yarnpkg/yarn/issues/7917
Bug description
When running scripts defined in package.json using yarn run xxx on a large monorepo, there is an extremely long delay before the command is actually run. This delay is always the same and seems to depend on the size of the monorepo.
Command
Here is a simple example:
In package.json
Then in shell run:
Using time to confirm the duration:
What is the current behavior?
Yarn does something using 100% of a CPU core for 265s (on my repo and machine) before actually running the command.
What is the expected behavior?
Yarn runs the command instantly
Steps to Reproduce
Node Version: 10.15.3
Yarn v1 Version: 2.0.0-rc.29.git.20200218.926781b7
OS and version: MacOS Catalina
The text was updated successfully, but these errors were encountered: