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

Multicore improvements #69

Merged
merged 24 commits into from Sep 2, 2021
Merged

Multicore improvements #69

merged 24 commits into from Sep 2, 2021

Conversation

str4d
Copy link
Member

@str4d str4d commented Jun 1, 2021

Cherry-picked from https://github.com/filecoin-project/bellperson

Part of #7.
Closes #21.

@str4d
Copy link
Member Author

str4d commented Jun 1, 2021

Ignore the clippy errors and warnings; they are being addressed separately.

CHANGELOG.md Outdated Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
type Item = T;
type Error = E;
impl<T> Waiter<T> {
/// Wait for the result.
Copy link
Contributor

@daira daira Jun 1, 2021

Choose a reason for hiding this comment

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

As far as I can see, this is inaccurate: std::Option::take will never wait.

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't, but this is emulating the threaded API. Both instances are effectively blocking, one just blocks for longer.

src/multicore.rs Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK with suggestions.

src/multicore.rs Outdated Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
dignifiedquire and others added 14 commits June 4, 2021 22:23
Uses the existing env variable to restrict the number of threads used for parallel proving.

Cleaned up version of #81

(cherry picked from commit 0c1868d)

pick:
- Only the changes to bellman::multicore.
- Added lazy_static and rayon dependencies.
(cherry picked from commit dca4c5a)

pick: Only the changes to bellman::multicore.
(cherry picked from commit c55d5e4)

Changes during pick:
- Excluded GPU-related changes (not upstreamed yet).
- Added fix for change to Field::double API.
- Fixed no-multicore Worker impl.

Co-authored-by: Jack Grigg <thestr4d@gmail.com>
If a waiter is used from within a thread pool it might deadlock. Don't
do that.

(cherry picked from commit dc13606)
The Worker/Waiter model creates a Worker that requests that threads
be spawned to complete some work.  Since there is no bound to the
number of requests for work, it's possible to exhaust memory with
pending closures waiting to be processed.

This was not an issue when 'everything was bounded by a single thread
pool', however that setup has race conditions built into it.

We have recently spent time splitting that model out, and this should
be the final step required.

This code places a limit on the number of requests that can be queued
in an attempt to lessen the chance of memory exhaustion due to pending
requests.  It does this by placing a limit that is a simple multiple of
the number of cores available (and in testing is far below observed
crash criteria).  When the number of requests for spawning new threads
reaches that limit, instead of adding a new request, we block on execution
to help clear the backlog.

(cherry picked from commit 74947c0)

pick: Also includes log change from b481a34
(cherry picked from commit 007487f)
…dpool


This occurring is a programming error.

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
@str4d
Copy link
Member Author

str4d commented Jun 4, 2021

Rebased on 0.10.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK

Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
… is expected to play more nicely

with other uses of rayon in the same process (avoiding excess parallelism).

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK with suggested changes in #71.

CHANGELOG.md Outdated Show resolved Hide resolved
src/multicore.rs Outdated Show resolved Hide resolved
daira and others added 4 commits August 25, 2021 16:56
Co-authored-by: str4d <thestr4d@gmail.com>
…icore) implementation.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Use global rayon threadpool for multicore
Copy link
Member Author

@str4d str4d left a comment

Choose a reason for hiding this comment

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

utACK mod comment

src/multicore.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK (modulo @str4d's suggested change to a comment).

src/multiexp.rs Outdated Show resolved Hide resolved
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.

Switch to using rayon exclusively
6 participants