Skip to content

Limit threads #127

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 12 commits into from
Jul 19, 2018
Merged

Limit threads #127

merged 12 commits into from
Jul 19, 2018

Conversation

eviefp
Copy link

@eviefp eviefp commented Jul 17, 2018

This is more of a "is this what you had in mind", I'm totally open to feedback.

I added a --threads N option for all commands that (eventually) hit a mapConcurrently or a forConcurrently_. I did it by adding a Maybe Int parameter down the call stack (not sure if it's worth it refactoring all of the functions to some ReaderT env IO; probably not with this PR but I could take that on if it would get accepted).

A value of Nothing means use old behaviour, otherwise use the method suggested by @kritzcreek's link in #126.

I would also like to keep the changes to the import because I like them arranged nicely, but I'm totally fine reverting that chunk if you want me to.

This has been tested on Appveyor on our project and fixed our issues. We have only tested with --threads 1 so far and that works (but is obviously a bit slow).

justinwoo
justinwoo reviewed Jul 17, 2018
View reviewed changes
app/Main.hs Outdated
@@ -586,6 +605,10 @@ main = do
Opts.metavar "PACKAGE"
<> Opts.help "The name of the package to install"

limitThreads = Opts.option Opts.auto $
Opts.long "threads"
<> Opts.help "Limit the number of threads"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs better explanation

Copy link
Author

Choose a reason for hiding this comment

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

How about renaming it to jobs and the text to Limit the number of jobs that can run concurrently?

justinwoo
justinwoo reviewed Jul 17, 2018
View reviewed changes
app/Main.hs Outdated
import System.Environment (getArgs)
import qualified System.IO as IO
import qualified System.Process as Process
import Data.Either.Combinators (rightToMaybe)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really would rather not have all of these diff changes because it makes maintenance harder

@justinwoo
Copy link
Collaborator

This sounds fine overall? Just I guess we should do a pre-release in any case if we merge this to see if anyone has problems with this.

@eviefp
Copy link
Author

eviefp commented Jul 17, 2018

No rush for me, we've included the binary from this branch in our repo until it's up on chocolatey.

kritzcreek
kritzcreek requested changes Jul 17, 2018
View reviewed changes
Copy link
Member

@kritzcreek kritzcreek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should be using the word thread, it's jobs not threads that we are limiting here (so the word thread might mislead someone).

psc-package.cabal Outdated
@@ -27,7 +27,8 @@ executable psc-package
system-filepath -any,
text -any,
errors -any,
turtle <1.6
turtle <1.6,
SafeSemaphore -any
Copy link
Member

Choose a reason for hiding this comment

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

No need to depend on this package, the bugs in base's semaphore have been fixed.

@eviefp
Copy link
Author

eviefp commented Jul 17, 2018

Thanks for the reviews, I think I addressed the issues. Let me know if the current message is okay @justinwoo

I haven't tested it yet, will do that tomorrow at work on our codebase and will update then.

@eviefp
Copy link
Author

eviefp commented Jul 18, 2018

Alright, after a bit of confusion I think everything is working correctly. Tested it on appveyor and it works just fine with up to 6 jobs. Didn't encounter any issues with this version.

kritzcreek
kritzcreek requested changes Jul 19, 2018
View reviewed changes
app/Main.hs Outdated
Nothing -> mapConcurrently dirFor dependencies
Just max' -> do
sem <- newQSem max'
mapConcurrently (\x -> waitQSem sem *> dirFor x <* signalQSem sem) dependencies
Copy link
Member

@kritzcreek kritzcreek Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You'll want to use bracket_ (waitQSem sem) (signalQSem sem) (dirFor x) to make sure you don't lose a capacity on exception in dirFor.

EDIT:

I got that one from http://hackage.haskell.org/package/base-4.11.1.0/docs/Control-Concurrent-QSem.html#t:QSem

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, thank you!

@eviefp
Copy link
Author

eviefp commented Jul 19, 2018

Added bracket_, made sure there's no more formatting noise and tested it on our appveyor. Seems to work even with jobs = 10, but still fails with unlimited.

If you mind the Makefile I could remove that too.

@justinwoo justinwoo merged commit 8c25bb5 into purescript:master Jul 19, 2018
@justinwoo
Copy link
Collaborator

Thanks! I made a pre-release here: https://github.com/purescript/psc-package/releases/tag/v0.4.1-pre

@justinwoo justinwoo mentioned this pull request Jul 21, 2018
Closed
Dretch added a commit to Dretch/psc-package that referenced this pull request Jul 22, 2018
@Dretch
index on master: 8c25bb5 Limit threads (purescript#127)
dd9c278
@justinwoo justinwoo mentioned this pull request Jul 25, 2018
Merged
9 tasks
Profpatsch pushed a commit to justinwoo/nixpkgs that referenced this pull request Jul 28, 2018
@justinwoo @Profpatsch
psc-package: 0.3.2 -> 0.4.1
eff76a6 
Updates that remove features and improve user experience.

Adds warnings for trying to install packages without (purescript/psc-package#126 by @Dretch)
Filters "installing" messages for build (purescript/psc-package#130 by @Dretch)
Adds options for limiting jobs for install (purescript/psc-package#127 by @Vladciobanu)
Per purescript/psc-package#121, removes the confusing misfeature "add-from-bower", which led to many users thinking this command was for adding "extra-deps" like Stack. See the thread for details on how you could readily replace this command if you used it before.
Profpatsch pushed a commit to NixOS/nixpkgs that referenced this pull request Jul 28, 2018
@justinwoo @Profpatsch
psc-package: 0.3.2 -> 0.4.1
7dadfbd 
Updates that remove features and improve user experience.

Adds warnings for trying to install packages without (purescript/psc-package#126 by @Dretch)
Filters "installing" messages for build (purescript/psc-package#130 by @Dretch)
Adds options for limiting jobs for install (purescript/psc-package#127 by @Vladciobanu)
Per purescript/psc-package#121, removes the confusing misfeature "add-from-bower", which led to many users thinking this command was for adding "extra-deps" like Stack. See the thread for details on how you could readily replace this command if you used it before.
@justinwoo justinwoo mentioned this pull request Feb 2, 2019
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@justinwoo justinwoo justinwoo left review comments

@kritzcreek kritzcreek kritzcreek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eviefp @justinwoo @kritzcreek