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

Add multi-threaded support to TrajOpt #333

Conversation

Levi-Armstrong
Copy link
Contributor

@Levi-Armstrong Levi-Armstrong commented Jun 19, 2023

This replaces #302 and surprised that it was not having issues because the Models themselves needs to be threadsafe. This is why that PR required the omp critical around the addHinge, but does not solve the issue where other constraints are also adding variables and other items to the model which was not threadsafe.

@marip8 I came up with a way to separate running trajopt with and without multi-threading but not sure if there is a better way.

@Levi-Armstrong
Copy link
Contributor Author

@marip8 Another option is make these methods virtual method of BasicTrustRegionSQP and then create BasicTrustRegionSQPOMP which overloads these methods. I think this approach is cleaner what do you think?

@Levi-Armstrong
Copy link
Contributor Author

Alright I decided to leverage inheritance which I think is cleaner. Also added multiple unit tests and benchmark.

@Levi-Armstrong
Copy link
Contributor Author

I ran the benchmarks and heaptrack to confirm the changes to not impact the default behavior. The multi threaded approach has a marginal improvement for the planning unit test and memory is doubled based on heaptrack. This is most likely due to each of the operation are not leveraging the same thread pool so the thread local caching is not as effective. In tesseract_planning I will implement a version which leverage the executor so it may perform better.

@marip8
Copy link
Contributor

marip8 commented Jun 20, 2023

The multi threaded approach has a marginal improvement for the planning unit test and memory is doubled based on heaptrack.

Interesting. How hard are these unit test problems (number of waypoints, constraints, etc.)? I would have thought there would be a significant time improvement

I haven't had a chance to look through this thoroughly yet, but it seems like a reasonable approach

@Levi-Armstrong
Copy link
Contributor Author

The multi threaded approach has a marginal improvement for the planning unit test and memory is doubled based on heaptrack.

Interesting. How hard are these unit test problems (number of waypoints, constraints, etc.)? I would have thought there would be a significant time improvement

The planning unit test takes like 23 seconds but only has 6 constraints and 8 costs. I think this is a small problem, but the other issue is I believe each use of omp has its own thread pool so the caching is not as effective hindering performance.

@Levi-Armstrong
Copy link
Contributor Author

I will test it on the puzzle piece example.

@Levi-Armstrong
Copy link
Contributor Author

After CI finishes I will merge, but it would be good if you all have a test case to try and report back.

@Levi-Armstrong
Copy link
Contributor Author

Levi-Armstrong commented Jun 20, 2023

I tried the puzzle piece example but it also showed no difference. I see the threads ramp up but does not make a difference for these use cases.

@Levi-Armstrong Levi-Armstrong merged commit 6533b3d into tesseract-robotics:master Jun 20, 2023
6 checks passed
@Levi-Armstrong Levi-Armstrong deleted the feat/TrajOptMultiThreaded branch June 20, 2023 03:10
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.

2 participants