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

'install -j': allow limiting the number of parallel linker invocations #1572

Closed
wants to merge 8 commits into from

Conversation

23Skidoo
Copy link
Member

@23Skidoo 23Skidoo commented Nov 7, 2013

Fixes #1529. Will probably refactor to use JobLimit once JobLimit is reimplemented on top of OS-level semaphore.

Next up: building components in parallel.

@dcoutts
Copy link
Contributor

dcoutts commented Nov 7, 2013

Nice, I'll have a look.

@tibbe
Copy link
Member

tibbe commented Nov 7, 2013

LGTM

@dcoutts
Copy link
Contributor

dcoutts commented Nov 22, 2013

Actually, I'm now even more confused. Why does cabal-install have a --max-linker-jobs-semaphore flag? It doesn't need to pass this flag to itself, just to the ./Setup processes it invokes. What am I missing?

@23Skidoo
Copy link
Member Author

@dcoutts

Why is this not just another JobLimit?

I'll make this code use a JobLimit once I implement OS semaphore-based JobLimits (as I said in the first comment).

Actually, I'm now even more confused. Why does cabal-install have a --max-linker-jobs-semaphore flag? It doesn't need to pass this flag to itself, just to the ./Setup processes it invokes. What am I missing?

Yes, It's a setup build-only flag. cabal build doesn't have it. See the fourth commit.

@23Skidoo
Copy link
Member Author

Rebased on top of current master.

@tibbe
Copy link
Member

tibbe commented Dec 19, 2013

Do you need anything from me here or are you waiting for @dcoutts?

@23Skidoo
Copy link
Member Author

@tibbe I just need to rebase this branch.

@23Skidoo
Copy link
Member Author

23Skidoo commented Jan 4, 2014

The branch was rebased. I'd like to have this merged soon.

@dcoutts

I remember that you want me to extend the JobLimit module to support semaphores and use a JobLimit instead of a semaphore in executeInstallPlan. I'll do that in a separate pull request.

@23Skidoo
Copy link
Member Author

Current status: @dcoutts objects to merging this in the current state, says that it's better to implement a semaphore-based JobLimit instead of passing a Semaphore around.

@tibbe
Copy link
Member

tibbe commented Jan 14, 2014

@23Skidoo is this something that you can realistically do if we want a 1.20 release by e.g. end on January? I don't perfect be the enemy of the good here. The semaphore-based (file-based?) version seems like a nice clean-ups of the passing around a Semaphore but it doesn't seem to actually change the actual end result in terms of amount of parallelism right?

@23Skidoo
Copy link
Member Author

@tibbe

I'm a bit overloaded right now, so I can't guarantee that this will be finished before the end of the month.

The plan is to extend this work later to support component-level parallelism and building profiling/dynamic versions in parallel. @dcoutts thinks that an OS-level semaphore is a too low-level abstraction and we should be using something more high-level (we haven't yet come up with a design).

Personally I'm in favour of merging this pull request in the current state and refactoring later (the command-line UI should stay the same), but @dcoutts thinks it'd be better to implement a cleaner solution right away.

@tibbe
Copy link
Member

tibbe commented Jan 15, 2014

@23Skidoo I'm also in favor of merging, especially since we don't even have a design for the alternative implementation.

@dcoutts
Copy link
Contributor

dcoutts commented Jan 15, 2014

It's not an outright objection, but I'd like to have an idea of where we're going. The plan to merge now involves making the code uglier and more complicated with the intention of simplifying and cleaning up later. That's ok in itself but I don't think we have a clear picture of what that simplifying and cleaning up looks like, and so that's what worries me. I'm prepared to spend time on it (with @23Skidoo) but I can't guarantee meeting an end of Jan deadline.

Is that important? The code already has some of the parallelism stuff in, since it passes -j through to ghc --make -j. Is there a big rush for the rest?

@tibbe
Copy link
Member

tibbe commented Jan 15, 2014

Is that important? The code already has some of the parallelism stuff in, since it passes -j through to ghc --make -j. Is there a big rush for the rest?

Incremental progress. It's generally more productive overall than big bang development, which typically relies on time no one actually has. :)

@tibbe
Copy link
Member

tibbe commented Apr 7, 2014

There are several open pull requests for various parallelism features. My understanding is that they're blocked on some work by @23Skidoo on this one. Could we get a status update so I can decide whether to try to get this in for 1.20 or wait for 1.22? Thanks!

@23Skidoo
Copy link
Member Author

23Skidoo commented Apr 8, 2014

@tibbe

I don't want to block 1.20 on this. If you want, you can close these pull requests for now, and I will resubmit once I get time to work on this (probably not before summer).

@tibbe
Copy link
Member

tibbe commented Apr 9, 2014

Closing as per @23Skidoo's request. This doesn't mean that we don't want these changes! It just means that there's some legwork we need to do first and that these changes will essentially have to written from scratch on top of the new foundation.

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.

Add option to limit number of concurrent calls to linker when building with -j
3 participants