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

improve one_size_heap_list: use rwlock to speedup the allocation/free #6370

Merged
merged 6 commits into from
Dec 3, 2023

Conversation

JiakunYan
Copy link
Contributor

@JiakunYan JiakunYan commented Oct 16, 2023

The original implementation uses an HPX mutex to ensure thread safety. This PR replaces it with a pthread rwlock, which greatly improves its performance. Multiple allocation/free calls can proceed simultaneously under the read lock, and the pthread read lock only costs an atomic fetch-and-add in most cases.

Need to discuss: directly using pthread rwlock in HPX codebase may not be a good idea, but implementing the whole pthread rwlock algorithm in HPX can also be cumbersome.

@StellarBot
Copy link

Can one of the admins verify this patch?

@codacy-production
Copy link

codacy-production bot commented Oct 16, 2023

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.05% 91.27%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (5ffdfbe) 193075 164320 85.11%
Head commit (49e0553) 189172 (-3903) 160902 (-3418) 85.06% (-0.05%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6370) 355 324 91.27%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@hkaiser
Copy link
Member

hkaiser commented Oct 17, 2023

@JiakunYan I think we can improve the implementation of our shared_mutex by placing its state data into a single atomic and start locking things internally only if really needed; i.e. replace

struct state_data
{
unsigned shared_count;
bool exclusive;
bool upgrade;
bool exclusive_waiting_blocked;
};

@JiakunYan JiakunYan marked this pull request as ready for review November 8, 2023 04:24
@JiakunYan JiakunYan requested a review from hkaiser as a code owner November 8, 2023 04:24
@JiakunYan
Copy link
Contributor Author

@hkaiser I did some brief benchmarking using Octotiger on Rostam. The performance of the new shared lock is pretty good! (around 5% improvement for a specific octotiger configuration)

@hkaiser
Copy link
Member

hkaiser commented Dec 3, 2023

@JiakunYan this looks good now and the test errors are unrelated. Can this be merged now?

@JiakunYan
Copy link
Contributor Author

@JiakunYan this looks good now and the test errors are unrelated. Can this be merged now?

Sure!

@hkaiser
Copy link
Member

hkaiser commented Dec 3, 2023

@JiakunYan this looks good now and the test errors are unrelated. Can this be merged now?

Sure!

Could you fix the conflicts, please?

@JiakunYan
Copy link
Contributor Author

JiakunYan commented Dec 3, 2023

@JiakunYan this looks good now and the test errors are unrelated. Can this be merged now?

Sure!

Could you fix the conflicts, please?

I tried rebasing this PR to the current master branch. However, git is behaving somehow weirdly. It kept missing to apply some changes in the Trying to streamline shared_mutex. Could you try rebasing it on your end and see what will happen?

@hkaiser
Copy link
Member

hkaiser commented Dec 3, 2023

@JiakunYan this looks good now and the test errors are unrelated. Can this be merged now?

Sure!

Could you fix the conflicts, please?

I tried rebasing this PR to the current master branch. However, git is behaving somehow weirdly. It kept missing to apply some changes in the Trying to streamline shared_mutex. Could you try rebasing it on your end and see what will happen?

If I rebase my branch, then you will not be able to merge it to your branch because of conflicts. I'll resolve the conflicts manually and merge your work manually as well. Thanks!

@hkaiser hkaiser merged commit 2c3216f into STEllAR-GROUP:master Dec 3, 2023
4 checks passed
@hkaiser hkaiser added this to the 1.10.0 milestone Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants