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

Adding feature, Range Based Lock #6379

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Johan511
Copy link
Contributor

@Johan511 Johan511 commented Nov 1, 2023

Adding feature, Range Based lock to HPX

API:
range_lock rl;
lockId = rl.lock(rangeBegin, rangeEnd);
rl.unlock(lockId);

Pending Task:
Adding tests

Copy link

codacy-production bot commented Nov 1, 2023

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-84.56%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (89f342b) 0 0 84.59%
Head commit (14e182e) 153777 (+153777) 51 (+51) 0.03% (-84.56%)

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 (#6379) 0 0 ∅ (not applicable)

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

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)??(=)

Info

PropertyBeforeAfter
HPX Commitdcb54159da61de
HPX Datetime2023-05-10T12:07:53+00:002023-11-02T04:22:07+00:00
Datetime2023-05-10T14:50:18.616050-05:002023-11-01T23:30:22.792853-05:00
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Envfile

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch(=)

Info

PropertyBeforeAfter
HPX Commitdcb54159da61de
HPX Datetime2023-05-10T12:07:53+00:002023-11-02T04:22:07+00:00
Datetime2023-05-10T14:52:35.047119-05:002023-11-01T23:32:35.779661-05:00
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Envfile

Comparison

BENCHMARKFORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATORPARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATORSCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add(=)(=)(=)
Stream Benchmark - Scale(=)(=)(=)
Stream Benchmark - Triad(=)(=)(=)
Stream Benchmark - Copy(=)-(=)

Info

PropertyBeforeAfter
HPX Commitdcb54159da61de
HPX Datetime2023-05-10T12:07:53+00:002023-11-02T04:22:07+00:00
Datetime2023-05-10T14:52:52.237641-05:002023-11-01T23:32:52.616083-05:00
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Envfile

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (≤5%)
++/--Large performance improvement/degradation (≤10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

Could you please add more tests, especially some tests that actually exercise the multi-threaded use of the new constructs?

class RangeLock
{
template <typename Key, typename Value>
using MapTy = boost::container::flat_map<Key, Value>;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use names conforming to our naming scheme requirements? In this case, please use map_type or similar.

Copy link
Member

Choose a reason for hiding this comment

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

std::size_t lock(std::size_t begin, std::size_t end);
std::size_t try_lock(std::size_t begin, std::size_t end);
void unlock(std::size_t lockId);
};
Copy link
Member

Choose a reason for hiding this comment

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

Usually, mutex types are movable, but not copyable. Could you please make sure to conform to the conventions? This will most likely also allow to replace the std::shared_ptr<std::atomic_bool> with a simpler solution based on unique_ptr.

Copy link
Contributor Author

@Johan511 Johan511 Nov 2, 2023

Choose a reason for hiding this comment

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

Usually, mutex types are movable, but not copyable.

Sure, just need to delete the copy constructor and define a move constructor for it I believe. Do I need to worry about thread safety while moving? like say another thread trying to lock?

This will most likely also allow to replace the std::shared_ptrstd::atomic_bool with a simpler solution based on unique_ptr.

I am unsure how adding move constructor would help change the flag from shared_ptr to unique_ptr.
To summarise, if a thread locks range [0, 20), it also creates an atomic_bool flag.
So if another thread (2) wants to lock an intersecting range, say [10, 30). It spins on the atomic_bool flag before trying to acquire the lock again, trying to acquire the range_lock is an expensive operation which we want to avoid unless we are confident the range_lock can be acquired.

When performing unlock, we delete the atomic_bool flag while the other threads might still be spinning on it, using unique_ptr would delete the atomic_bool causing a segmentation fault, shared_ptr will not cause this issue.

@Johan511
Copy link
Contributor Author

Johan511 commented Nov 2, 2023

More comprehensive tests on the way @hkaiser. Will add them soon.
Was planning something similar to what we have for shared_mutex.

@Johan511
Copy link
Contributor Author

Johan511 commented Dec 8, 2023

@hkaiser do we have a concurrent hash map like the one by boost or the one by folly?

@hkaiser
Copy link
Member

hkaiser commented Dec 8, 2023

@hkaiser do we have a concurrent hash map like the one by boost or the one by folly?

No we don't have something like that.

@hkaiser
Copy link
Member

hkaiser commented Jan 27, 2024

@Johan511 I know that currently the reporting of the build logs for the HPX CIs is broken (we're working on fixing this). For this reason, here is the error message that makes the builders fail:

/.../hpx/libs/core/synchronization/include/hpx/synchronization/range_mutex.hpp:71:25: error: no member named 'get' in 'hpx::synchronization::detail::range_mutex<hpx::detail::spinlock<true>, std::lock_guard>'

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)??-

Info

PropertyBeforeAfter
HPX Commitdcb54156834baa
HPX Datetime2023-05-10T12:07:53+00:002024-02-07T09:11:33+00:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Clusternamerostamrostam
Datetime2023-05-10T14:50:18.616050-05:002024-02-07T03:20:02.039903-06:00

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch=

Info

PropertyBeforeAfter
HPX Commitdcb54156834baa
HPX Datetime2023-05-10T12:07:53+00:002024-02-07T09:11:33+00:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Clusternamerostamrostam
Datetime2023-05-10T14:52:35.047119-05:002024-02-07T03:22:15.846511-06:00

Comparison

BENCHMARKFORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATORPARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATORSCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add(=)(=)(=)
Stream Benchmark - Scale+(=)=
Stream Benchmark - Triad(=)=(=)
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
HPX Commitdcb54156834baa
HPX Datetime2023-05-10T12:07:53+00:002024-02-07T09:11:33+00:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Clusternamerostamrostam
Datetime2023-05-10T14:52:52.237641-05:002024-02-07T03:22:32.733787-06:00

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (≤5%)
++/--Large performance improvement/degradation (≤10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

Copy link

codacy-production bot commented Feb 7, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.02% 98.02%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (17bdd0d) 206733 176202 85.23%
Head commit (600b4a2) 207519 (+786) 176912 (+710) 85.25% (+0.02%)

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 (#6379) 101 99 98.02%

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

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

range lock impl, spins for wait flag
added header file to cmake

added missing headers and return 0 in test

added test

added test

changed file names were not reflected

renaming was not committed

fixing static errors

adding signature to files

debugging CI

debugging CI

forgot header

removed debugging logs

codacy check change
…_id!=0 throws syystem error

fixed codacy warning

cleaning up unnecessery template params

fixing incorrect std::system_error syntax

fixing std::system_error syntax (again)

fixing std::system_error syntax (again)

changing errc to error_code

sys erro trying to fix

fixing reorder error

Hygiene changes
@biddisco
Copy link
Contributor

biddisco commented Feb 8, 2024

I have not looked at the implementation (I will examine it later), but I'm curious as to the use case for the range-lock. Is this work inspired by the GSoC project I proposed a few years ago (with an astrophysics particle code in mind), or do you have another use case that requires it? (or perhaps it was "just for fun" as an interesting project to try?)

@Johan511
Copy link
Contributor Author

Johan511 commented Feb 8, 2024

Hi @biddisco, this is a fun piece of work inspired by the GSoC project you had proposed. I didn't have an use-case in mind, I had tested out it's performance based on the amount of time it took to execute the test. It'd be great if I could test the impact of the code on a more real life scenario.

@Johan511
Copy link
Contributor Author

Johan511 commented Feb 8, 2024

@hkaiser can you have a look at the CI? I am not able to view the logs on the jenkins CI.
Also is there any reference I could use to write documentation? Maybe some sort of specialized mutex somewhere in HPX.

@hkaiser
Copy link
Member

hkaiser commented Feb 10, 2024

@hkaiser can you have a look at the CI? I am not able to view the logs on the jenkins CI. Also is there any reference I could use to write documentation? Maybe some sort of specialized mutex somewhere in HPX.

Sorry for the CI issues. We're currently working on a solution for this problem.

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.

4 participants