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.

Sign up for GitHub

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

Jump to bottom

Reduce memory requirements for our main shared state #4759

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Reduce memory requirements for our main shared state #4759

merged 1 commit into from
Jun 17, 2020

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 16, 2020

  • flyby: add missing headers to CMakeLists.txt

@hkaiser
Reduce memory requirements for our main shared state
7465579 
- flyby: add missing headers to CMakeLists.txt
@hkaiser hkaiser added this to the 1.5.0 milestone Jun 16, 2020
msimberg
msimberg approved these changes Jun 17, 2020
View reviewed changes
Copy link
Contributor

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Thanks @hkaiser! The failure on CircleCI is unrelated (fixed in #4761), and the GH actions is due to the dependencies not downloading (I hope we didn't hit a bandwidth limit again?). I think we're safe to merge this.

@hkaiser
Copy link
Member Author

hkaiser commented Jun 17, 2020

The failure on CircleCI is unrelated (fixed in #4761), and the GH actions is due to the dependencies not downloading (I hope we didn't hit a bandwidth limit again?). I think we're safe to merge this.

I just retriggered those builders. Should go through this time.

@hkaiser hkaiser merged commit d87ee23 into master Jun 17, 2020
@hkaiser hkaiser deleted the shared_state branch June 17, 2020 14:25
@msimberg
Copy link
Contributor

By the way, I asked the linear algebra people at CSCS to check if this had any effect in their library (as they definitely have more than one continuation on most of their futures, but through dataflow which might make the comparison mostly useless). In any case, it seems like it had no negative effect and potentially small positive effect (but mostly within noise).

aurianer
aurianer reviewed Jul 23, 2020
View reviewed changes
Copy link
Contributor

@aurianer aurianer left a comment

Choose a reason for hiding this comment

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

Again sorry for the late review :/

libs/futures/CMakeLists.txt
@@ -54,6 +56,8 @@ add_hpx_module(
SOURCES ${futures_sources}
HEADERS ${futures_headers}
COMPAT_HEADERS ${futures_compat_headers}
EXCLUDE_FROM_GLOBAL_HEADER "hpx/futures/detail/future_data.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary anymore since we exclude all detail headers from the global one in HPX_AddModule

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you open a PR to remove that, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msimberg msimberg msimberg approved these changes

@aurianer aurianer aurianer left review comments

@biddisco biddisco Awaiting requested review from biddisco

Assignees
No one assigned
Labels
category: core type: enhancement
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@hkaiser @msimberg @aurianer