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 boost_sync #4918

Closed
wants to merge 2 commits into from

Conversation

mossheim
Copy link
Contributor

@mossheim mossheim commented May 9, 2020

Purpose and Motivation

Fixes #4914 (confirmed locally using the boost from the source release)

I know this looks like a big change, but see #4914 for my reasoning. This is actually quite safe in terms of the amount of code that had to be ported.

There was only one place where i had to make a substantial change, in PyrSched.cpp, and i confirmed locally with a test program that this works as expected.

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@mossheim mossheim added comp: build CMake build system organizational labels May 9, 2020
@mossheim mossheim requested a review from dvzrv May 9, 2020 12:52
@mossheim
Copy link
Contributor Author

mossheim commented May 9, 2020

whoops, please wait to review, this has legitimate linting issues :) sorry

mostly requires just changing the include and type.

in one case (PyrSched.cpp) requires using a new API. confirmed locally
that this works as expected.

remove references to boost_sync in cmake and extract_boost

add interprocess semaphore files

update extract_boost

migrate to boost.ip.semaphore
@timblechmann
Copy link
Contributor

@brianlheim don't! semaphores in boost.interprocess and semaphores in boost.sync are two different beasts: interprocess semaphores are, well, interprocess and much slower that what is in boost.sync. if boost.sync doesn't build with 1.73, that has to be fixed (please file a bug report for boost.sync)

especially for supernova the semaphore performance is critical

@mossheim
Copy link
Contributor Author

oh, i see! would the std::binary_semaphore from c++20 be a suitable replacement in your opinion @timblechmann ? thanks for the warning, and sorry for my negligence here. apologies are also due to @dyfer then for my arrogance in assuming this was so cut-and-dry when we talked about it earlier.

i do think boost.sync -- at least, the latest version -- will build with 1.73; but, i'm not sure it will build with 1.66 that we package in 3.11, and i didn't bother to find out since it seemed easier to do this port. i'll do some more research.

@mossheim mossheim closed this May 12, 2020
@dyfer
Copy link
Member

dyfer commented May 12, 2020

thanks @brianlheim and no worries! I'm glad someone with more expertise than me chimed in on the issue.

@timblechmann
Copy link
Contributor

i hope that the c++20 semaphores (binary or counting) will have a good implementation, so they will probably be the way to go once they are widely available (macos for example has 3 different semaphore implementations, i hope the libc++ implementation will use dispatch semaphores rather than system v). in general i'd rather recommend updating boost and boost.sync to solve those build issues.

@mossheim
Copy link
Contributor Author

thanks! yes definitely, now that i know the boost.sync semaphores aren't available anywhere else i'll keep them around. i was happy to find that boost.sync is still being actively maintained

@mossheim mossheim deleted the topic/remove-boost-sync branch May 13, 2020 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants