-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Futures attempt to execute threads directly if those have not started executing #6099
Conversation
hkaiser
commented
Dec 7, 2022
- adding new API function hpx::threads::get_outer_self_id
26b44d8
to
146d101
Compare
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
f470ea5
to
207af34
Compare
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
bdd2fb9
to
eda6a19
Compare
f9d3a9c
to
9e3b499
Compare
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
9e3b499
to
bb43390
Compare
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
f41533a
to
731e192
Compare
f6334d3
to
d3f50ae
Compare
Although disabling run_as_child decreases allocations by a lot, here is still a leak-like issue arising with hpx:ini=hpx.stacks.use_guard_pages=0. I'm investigating it rn. Here's some plots running |
That would explain why the suspend_runtime et.al. now fail even for builders that don't enable the run_as_child hint by default. |
Here, does mmap-ing memory of (size + EXEC_PAGESIZE) make sense even if we are not using guard pages?
Deallocation seems to not take EXEC_PAGESIZE into account if we don't use guard pages: hpx/libs/core/coroutines/include/hpx/coroutines/detail/posix_utility.hpp Lines 156 to 172 in dd17010
I'm not familiar with mmap and such, but this made me raise an eyebrow |
Excellent catch! Could you patch that, please? |
Thank you, I'll do that :) |
I'm also preparing a change right now that starts deleting stack segments of recycled threads once more then a certain number of threads have been recycled. That should keep the amount of memory allocated down a bit as well. |
74c7216
to
bdfd6d4
Compare
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
Performance test reportHPX PerformanceComparison
Info
Comparison
Info
Comparison
Info
Explanation of Symbols
|
@Pansysk75 your fix did the trick! I will roll back my change that limits the numer of recycled stacks as it made the performance tests fail. Let's see if after that change things will be still green. |
Yes, things are much better. The memory mappings now barely go above a few hundred, as the OS is able to merge the mapped areas more efficiently. Even with guard_pages=on, they are in the hundreds now (more than an order of magnitude less than before). The only memory "concern" is the slightly higher memory usage when using direct execution (because of the immediate thread spawining), but it's not that significant. So the thread stack allocation / recycling change is not as crucial. |
b73ca59
to
498c7c8
Compare
498c7c8
to
fe7a35d
Compare
@Pansysk75 I think this is finally good to be merged. What do you think? |
@hkaiser There is a single timed-out test on daint (I had a look yesterday but didn't get anywhere), but other than that, nothing further to add. |
I believe this is unrelated. Could be another case of the synchronization object going out of scope too early. |
bors merge |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|