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

Added parallelization to PreprocessShortestPath. #21770

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adityapande-1995
Copy link

@adityapande-1995 adityapande-1995 commented Aug 3, 2024

This PR tries to resolve #21705 by parallelizing the MathematicalProgram solving. I'm not sure if this solves the problem, but I'm open for suggestions :)


This change is Reviewable

@adityapande-1995 adityapande-1995 marked this pull request as ready for review August 3, 2024 20:12
@jwnimmer-tri
Copy link
Collaborator

\CC @cohnt FYI

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Aug 5, 2024

A few big picture points:

(1) Drake avoids unbounded parallelism. Every multicore par-for loop must always have a governor for max number of cores. Typically this a Parallelism parallelism argument with a default value. Then the loop implementation must respect that upper limit.

(2) For binning work across cores, we typically use our StaticParallelForIndexLoop wrapper (not direct calls to std::async) except in extreme cases where the work is extremely large, lumpy chunks. My (possibly wrong) first impression of the preprocessing is that is not very large nor lumpy.

@jwnimmer-tri jwnimmer-tri self-assigned this Aug 5, 2024
@jwnimmer-tri
Copy link
Collaborator

(3) For efficiency, there should be one mathematical program per core, not one program per edge.

Copy link
Contributor

@AlexandreAmice AlexandreAmice left a comment

Choose a reason for hiding this comment

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

Related issue is #19119. Perhaps resolving there would be more appropriate than having ad-hoc parallelization of solving MathematicalPrograms scattered throughout Drake.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @adityapande-1995)

@RussTedrake
Copy link
Contributor

RussTedrake commented Aug 20, 2024

FYI -- I just commented on #21705, but I also think that this specific codepath is likely to change soon (#21341). This particular code optimization might become irrelevant once we do that.

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @adityapande-1995)

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

Successfully merging this pull request may close these issues.

Parallelize the Preprocessing in GraphOfConvexSets
4 participants