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

Remove usage of Boost::fiber #671

Closed
wants to merge 10 commits into from
Closed

Remove usage of Boost::fiber #671

wants to merge 10 commits into from

Conversation

markaren
Copy link
Contributor

@markaren markaren commented Nov 11, 2021

This PR aims to remove Boost::fiber, refactoring the code to become sequential.
Parallelism becomes an implementation detail of the algorithm used. For this PR, the fixed-step algorithm uses the parallel std::for_each algorithm. A bool flag could be added to make it not use paralellism.

The overall aims is to make the code less verbose, easier to understand and provide a perfomance boost.

There are some issues with file locking that I have not been able to resolve.

Let the discussion begin.

@markaren
Copy link
Contributor Author

markaren commented Nov 11, 2021

Might need to fallback on a thread pool for older gcc versions (if we still want to support old versions)..
And the docker image needs libtbb-dev

@kyllingstad
Copy link
Member

This looks very interesting, and I'll give it a proper review as soon as I have the time (not today, unfortunately).

I do think the claim of a performance boost needs to be a bit better substantiated. What kinds of performance testing has been done? How many different system configurations; how many slaves in each; were they performing lightweight or heavyweight computations, or a combination thereof; etc.? (And yes, I understand that performance is just a part of the motivation here.)

@markaren
Copy link
Contributor Author

markaren commented Nov 12, 2021

The claims about performance needs more vetting for sure. I've only done minor non-scientifc tests so far, which do not actually compare the numeric results. So there could be bugs. However, the Knuckleboom case for instance runs 4x faster with this PR. Setting simulation to 5 sec for the test case and timing simulate_until yields 43s for fibers and 10s for std:.for_each.

However, looking at the benchmarks from https://ntnuopen.ntnu.no/ntnu-xmlui/bitstream/handle/11250/2728069/1-s2.0-S1569190X20301726-main%2b%25281%2529.pdf?sequence=2&isAllowed=y, we know that libcosim is not as fast as it could be.

@markaren
Copy link
Contributor Author

https://github.com/open-simulation-platform/libcosim/tree/parallel-pool uses a threadpool, so it should work with older compilers.

@markaren
Copy link
Contributor Author

markaren commented Feb 16, 2022

Supporting std::for_each also means dropping gcc8. Is that still fine?
Also I don't know how to update the docker image to include libtbb-dev required by gcc9

@markaren
Copy link
Contributor Author

<execution> requires TBB (Intel Thread Building Blocks) 2018 on linux. Ubuntu 18.04 ships with 2017 version. Need to update to Ubuntu 20.04. I assume this is a showstopper for this PR, and we should use a thread pool ?

@ljamt
Copy link
Member

ljamt commented Feb 18, 2022

<execution> requires TBB (Intel Thread Building Blocks) 2018 on linux. Ubuntu 18.04 ships with 2017 version. Need to update to Ubuntu 20.04. I assume this is a showstopper for this PR, and we should use a thread pool ?

Should be possible to change our workflows to make it compile, but maybe we shouldn't require this for any user to build it locally. The threadpool solution is just as good in my mind :)

@markaren markaren closed this Mar 28, 2022
@markaren markaren deleted the parallel branch May 4, 2022 11:48
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.

3 participants