Skip to content

Limit threads#127

Merged
justinwoo merged 12 commits into
purescript:masterfrom
eviefp:limit-threads
Jul 19, 2018
Merged

Limit threads#127
justinwoo merged 12 commits into
purescript:masterfrom
eviefp:limit-threads

Conversation

@eviefp
Copy link
Copy Markdown

@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
Comment thread app/Main.hs Outdated

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

@justinwoo justinwoo Jul 17, 2018

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
Copy Markdown
Author

@eviefp eviefp Jul 17, 2018

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
Comment thread 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
Copy Markdown
Collaborator

@justinwoo justinwoo Jul 17, 2018

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
Copy Markdown
Collaborator

justinwoo commented Jul 17, 2018

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
Copy Markdown
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
Copy Markdown
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).

Comment thread psc-package.cabal Outdated
errors -any,
turtle <1.6
turtle <1.6,
SafeSemaphore -any
Copy link
Copy Markdown
Member

@kritzcreek kritzcreek Jul 17, 2018

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
Copy Markdown
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
Copy Markdown
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
Comment thread 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
Copy Markdown
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
Copy Markdown
Author

@eviefp eviefp Jul 19, 2018

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
Copy Markdown
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
Copy Markdown
Collaborator

justinwoo commented Jul 19, 2018

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

This was referenced Jul 21, 2018
Closed
Merged
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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