Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

limit parallel requests to registry during resolution #564

Merged
merged 3 commits into from
Nov 4, 2019
Merged

limit parallel requests to registry during resolution #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e563785
limit parallel requests to registry during resolution
Nov 3, 2019
4d2a667
No need to bump the builder
arcanis Nov 4, 2019
1851321
Update package.json
arcanis Nov 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/yarnpkg-builder/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
{
"name": "@yarnpkg/builder",
"version": "2.0.0-rc.9",
"nextVersion": {
"nonce": "2397602753556307"
},
"bin": "./sources/boot-dev.js",
"dependencies": {
"@babel/core": "^7.5.5",
Expand Down
2 changes: 1 addition & 1 deletion packages/yarnpkg-cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"version": "2.0.0-rc.11",
"nextVersion": {
"semver": "2.0.0-rc.12",
"nonce": "99525153414315"
"nonce": "6623953652684197"
},
"main": "./sources/index.ts",
"dependencies": {
Expand Down
3 changes: 2 additions & 1 deletion packages/yarnpkg-core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
"name": "@yarnpkg/core",
"version": "2.0.0-rc.11",
"nextVersion": {
"nonce": "2905733943814283"
"semver": "2.0.0-rc.12",
"nonce": "3714013031290855"
},
"main": "./sources/index.ts",
"sideEffects": false,
Expand Down
6 changes: 4 additions & 2 deletions packages/yarnpkg-core/sources/Project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,8 @@ export class Project {
mustBeResolved.add(workspaceDescriptor.descriptorHash);
}

const limit = pLimit(10);
Copy link
Member

@eps1lon eps1lon Nov 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a good candidate for a .yarnrc.yml configuration. As you said the npm registry can handle this many open connections and it should be the most used registry. Limiting the default use case for a stress case should be avoided IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can incorporate it into the .yarnrc.yml configuration and update the PR.

My one concern about not limiting the default is companies do use private npm registries or proxies in front of npm. This took down our internal nexus proxy and made it unusable. Lucky for me, we brought the server back up before it caused a major disruption. If any engineer decides to create a new package and use yarn v2 it will cause the same problem unless they remember to set this value. If a lockfile exists this logic is not hit, so the only performance hit is when packages are updated or the lockfile is deleted. In the case of updating packages the amount of requests to the registry is typically small enough to not see any slow downs.

Does it make sense to still have an upper boundary even for npm? In theory, the current algorithm could make over a 1000 connections simultaneous depending on the packages being resolved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did v1 behave?

Copy link
Contributor Author

@lroling8350 lroling8350 Nov 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yarn v1 limited concurrent requests to the npm registry at 8 but it is configurable via the .yarnrc file by setting the network-concurrency param.

The rest is a description with code links for how this behaviour was discovered.

From what i can tell the request-manager.js had a queue for both metadata and tarball requests that had a configurable limit which default to 8 via the constant NETWORK_CONCURRENCY. It is configurable via the .yarnrc file with the value network-concurrency code link

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well then it sounds like we should merge this. I would even argue to use the old default.

Copy link
Contributor Author

@lroling8350 lroling8350 Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change the default to 8, would you like me to expose this as a configuration option as well? If you do want the configuration option, would you like it in this PR or as a follow on?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A separate PR is fine for me. Wdyt @arcanis ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, let's keep this in a separate PR (and we can also make the fetch concurrency configurable) 👍

Let's also keep 10 - the Yarn codebase even used to have 16 but switched to 8 due to some weird bugs in the unpacking logic. Since we don't unpack anymore we don't need to be too cautious there.


while (mustBeResolved.size !== 0) {
// We remove from the "mustBeResolved" list all packages that have
// already been resolved previously.
Expand All @@ -451,7 +453,7 @@ export class Project {
// match the given ranges. That will give us a set of candidate references
// for each descriptor.

const passCandidates = new Map(await Promise.all(Array.from(mustBeResolved).map(async descriptorHash => {
const passCandidates = new Map(await Promise.all(Array.from(mustBeResolved).map(descriptorHash => limit(async () => {
const descriptor = allDescriptors.get(descriptorHash);
if (!descriptor)
throw new Error(`Assertion failed: The descriptor should have been registered`);
Expand All @@ -469,7 +471,7 @@ export class Project {
throw new Error(`No candidate found for ${structUtils.prettyDescriptor(this.configuration, descriptor)}`);

return [descriptor.descriptorHash, candidateLocators] as [DescriptorHash, Array<Locator>];
})));
}))));

// That's where we'll store our resolutions until everything has been
// resolved and can be injected into the various stores.
Expand Down