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.

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

fix: should limit sub package install parallel #191

Merged
merged 2 commits into from
Feb 3, 2017

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Feb 3, 2017

This change is Reviewable

@mention-bot
Copy link

@fengmk2, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dead-horse, @welladamm and @popomore to be potential reviewers.

@fengmk2 fengmk2 force-pushed the fix-limit-sub-pkg-install-parallel branch from 907442a to 8dd2c76 Compare February 3, 2017 04:29
@codecov-io
Copy link

codecov-io commented Feb 3, 2017

Codecov Report

Merging #191 into master will increase coverage by 0.01%.

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   85.67%   85.69%   +0.01%     
==========================================
  Files          19       19              
  Lines        1201     1202       +1     
  Branches      233      233              
==========================================
+ Hits         1029     1030       +1     
  Misses        172      172
Impacted Files Coverage Δ
lib/install.js 96.94% <100%> (+0.02%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d54e9ef...1941eaa. Read the comment docs.

@fengmk2 fengmk2 force-pushed the fix-limit-sub-pkg-install-parallel branch from bb82d07 to 1941eaa Compare February 3, 2017 04:38
@fengmk2 fengmk2 force-pushed the fix-limit-sub-pkg-install-parallel branch from 1941eaa to 412bd99 Compare February 3, 2017 04:39
@@ -205,7 +206,7 @@ function* _install(parentDir, pkg, ancestors, options) {
}
tasks.push(install(realPkgDir, childPkg, ancestors.concat(`${realPkg.name}@${realPkg.version}`), options));
}
yield tasks;
yield parallel(tasks, 10);
Copy link
Member

Choose a reason for hiding this comment

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

感觉以后需要一个队列形式的模块来统一控制所有的并发。

options.taskHandler.push(tasks);

Copy link
Member Author

Choose a reason for hiding this comment

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

还真没法在外网全局控制。。。这里有2层并发控制。

Copy link
Member

Choose a reason for hiding this comment

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

所以需要一个全局的队列控制挂在 options 上。

Copy link
Member Author

Choose a reason for hiding this comment

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

这样不太行,有可能导致 sub_install 和 install 之间相互依赖等待没法进行下去

@fengmk2
Copy link
Member Author

fengmk2 commented Feb 3, 2017

local_install 已经控制了 10个 并发 install,而 install 里面又控制了 10个 sub_install 的并发,所以目前最大并发数是 100。

没改之前话,sub_install 的并发有可能超过几千。

@fengmk2 fengmk2 merged commit cb8eea7 into master Feb 3, 2017
@fengmk2 fengmk2 deleted the fix-limit-sub-pkg-install-parallel branch February 3, 2017 06:03
@fengmk2
Copy link
Member Author

fengmk2 commented Feb 3, 2017

2.19.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants