Adding feature, Range Based Lock #6379

Johan511 · 2023-11-01T20:33:40Z

Adding feature, Range Based lock to HPX

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

Pending Task:
Adding tests

codacy-production · 2023-11-01T23:00:50Z

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 · 2023-11-02T04:33:10Z

Performance test report

HPX Performance

Comparison

BENCHMARK	FORK_JOIN_EXECUTOR	PARALLEL_EXECUTOR	SCHEDULER_EXECUTOR
For Each	(=)	??	(=)

Info

Property	Before	After
HPX Commit	`dcb5415`	`9da61de`
HPX Datetime	2023-05-10T12:07:53+00:00	2023-11-02T04:22:07+00:00
Datetime	2023-05-10T14:50:18.616050-05:00	2023-11-01T23:30:22.792853-05:00
Hostname	medusa08.rostam.cct.lsu.edu	medusa08.rostam.cct.lsu.edu
Clustername	rostam	rostam
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

BENCHMARK	NO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch	(=)

Info

Property	Before	After
HPX Commit	`dcb5415`	`9da61de`
HPX Datetime	2023-05-10T12:07:53+00:00	2023-11-02T04:22:07+00:00
Datetime	2023-05-10T14:52:35.047119-05:00	2023-11-01T23:32:35.779661-05:00
Hostname	medusa08.rostam.cct.lsu.edu	medusa08.rostam.cct.lsu.edu
Clustername	rostam	rostam
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

BENCHMARK	FORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATOR	PARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATOR	SCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add	(=)	(=)	(=)
Stream Benchmark - Scale	(=)	(=)	(=)
Stream Benchmark - Triad	(=)	(=)	(=)
Stream Benchmark - Copy	(=)	-	(=)

Info

Property	Before	After
HPX Commit	`dcb5415`	`9da61de`
HPX Datetime	2023-05-10T12:07:53+00:00	2023-11-02T04:22:07+00:00
Datetime	2023-05-10T14:52:52.237641-05:00	2023-11-01T23:32:52.616083-05:00
Hostname	medusa08.rostam.cct.lsu.edu	medusa08.rostam.cct.lsu.edu
Clustername	rostam	rostam
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

Symbol	MEANING
=	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…

hkaiser

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

hkaiser · 2023-11-02T15:48:15Z

libs/core/synchronization/include/hpx/synchronization/detail/range_lock_impl.hpp

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


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

Also, we do have a flat_map in HPX, could you use that instead (see: https://github.com/STEllAR-GROUP/hpx/blob/master/libs/core/datastructures/include/hpx/datastructures/detail/flat_map.hpp)

hkaiser · 2023-11-02T15:51:39Z

libs/core/synchronization/include/hpx/synchronization/detail/range_lock_impl.hpp

+        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);
+    };


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.

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.

libs/core/synchronization/include/hpx/synchronization/detail/range_lock_impl.hpp

libs/core/synchronization/include/hpx/synchronization/range_lock.hpp

libs/core/synchronization/include/hpx/synchronization/detail/range_lock_impl.hpp

libs/core/synchronization/include/hpx/synchronization/range_lock.hpp

Johan511 · 2023-11-02T17:25:18Z

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

libs/core/synchronization/include/hpx/synchronization/range_mutex.hpp

Johan511 · 2023-12-08T00:25:43Z

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

hkaiser · 2023-12-08T00:26:28Z

@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 · 2024-01-27T20:33:22Z

@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 · 2024-02-07T09:23:23Z

Performance test report

HPX Performance

Comparison

BENCHMARK	FORK_JOIN_EXECUTOR	PARALLEL_EXECUTOR	SCHEDULER_EXECUTOR
For Each	(=)	??	-

Info

Property	Before	After
HPX Commit	`dcb5415`	`6834baa`
HPX Datetime	2023-05-10T12:07:53+00:00	2024-02-07T09:11:33+00:00
Envfile
Hostname	medusa08.rostam.cct.lsu.edu	medusa08.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
Clustername	rostam	rostam
Datetime	2023-05-10T14:50:18.616050-05:00	2024-02-07T03:20:02.039903-06:00

Comparison

BENCHMARK	NO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch	=

Info

Property	Before	After
HPX Commit	`dcb5415`	`6834baa`
HPX Datetime	2023-05-10T12:07:53+00:00	2024-02-07T09:11:33+00:00
Envfile
Hostname	medusa08.rostam.cct.lsu.edu	medusa08.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
Clustername	rostam	rostam
Datetime	2023-05-10T14:52:35.047119-05:00	2024-02-07T03:22:15.846511-06:00

Comparison

BENCHMARK	FORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATOR	PARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATOR	SCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add	(=)	(=)	(=)
Stream Benchmark - Scale	+	(=)	=
Stream Benchmark - Triad	(=)	=	(=)
Stream Benchmark - Copy	(=)	(=)	(=)

Info

Property	Before	After
HPX Commit	`dcb5415`	`6834baa`
HPX Datetime	2023-05-10T12:07:53+00:00	2024-02-07T09:11:33+00:00
Envfile
Hostname	medusa08.rostam.cct.lsu.edu	medusa08.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
Clustername	rostam	rostam
Datetime	2023-05-10T14:52:52.237641-05:00	2024-02-07T03:22:32.733787-06:00

Explanation of Symbols

Symbol	MEANING
=	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…

codacy-production · 2024-02-07T12:05:59Z

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 · 2024-02-08T13:53:47Z

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 · 2024-02-08T19:56:00Z

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 · 2024-02-08T19:59:05Z

@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 · 2024-02-10T22:52:24Z

@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.

Johan511 requested a review from hkaiser as a code owner November 1, 2023 20:33

Johan511 force-pushed the range-lock branch from a97f92f to b172d7c Compare November 2, 2023 04:06

hkaiser added type: enhancement category: LCOs labels Nov 2, 2023

hkaiser requested changes Nov 2, 2023

View reviewed changes

hkaiser reviewed Dec 7, 2023

View reviewed changes

Johan511 force-pushed the range-lock branch from 2c87d1b to 8e19bfe Compare February 8, 2024 12:04

Johan511 added 9 commits February 8, 2024 06:40

range_lock impl, spin wait

2eb644b

range lock impl, spins for wait flag

added basic test

9e7181c

changed name to RangeLock

8134400

added std::lock_guard and std::unique_lock_guard for range_lock

96c909e

changed flat_map datastructure to hpx::flat_map

d0d1822

guards are movable but not copyable

56dba82

fixed deadlock error in test

600b4a2

Johan511 force-pushed the range-lock branch from 8e19bfe to 600b4a2 Compare February 8, 2024 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding feature, Range Based Lock #6379

Adding feature, Range Based Lock #6379

Johan511 commented Nov 1, 2023 •

edited

codacy-production bot commented Nov 1, 2023 •

edited

StellarBot commented Nov 2, 2023

HPX Performance

Comparison

Info

Comparison

Info

Comparison

Info

Explanation of Symbols

hkaiser left a comment

hkaiser Nov 2, 2023

hkaiser Nov 2, 2023

hkaiser Nov 2, 2023

Johan511 Nov 2, 2023 •

edited

Johan511 commented Nov 2, 2023 •

edited

Johan511 commented Dec 8, 2023

hkaiser commented Dec 8, 2023

hkaiser commented Jan 27, 2024

StellarBot commented Feb 7, 2024

HPX Performance

Comparison

Info

Comparison

Info

Comparison

Info

Explanation of Symbols

codacy-production bot commented Feb 7, 2024 •

edited

biddisco commented Feb 8, 2024

Johan511 commented Feb 8, 2024

Johan511 commented Feb 8, 2024

hkaiser commented Feb 10, 2024 •

edited

Adding feature, Range Based Lock #6379

Are you sure you want to change the base?

Adding feature, Range Based Lock #6379

Conversation

Johan511 commented Nov 1, 2023 • edited

codacy-production bot commented Nov 1, 2023 • edited

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

StellarBot commented Nov 2, 2023

HPX Performance

Comparison

Info

Comparison

Info

Comparison

Info

Explanation of Symbols

hkaiser left a comment

Choose a reason for hiding this comment

hkaiser Nov 2, 2023

Choose a reason for hiding this comment

hkaiser Nov 2, 2023

Choose a reason for hiding this comment

hkaiser Nov 2, 2023

Choose a reason for hiding this comment

Johan511 Nov 2, 2023 • edited

Choose a reason for hiding this comment

Johan511 commented Nov 2, 2023 • edited

Johan511 commented Dec 8, 2023

hkaiser commented Dec 8, 2023

hkaiser commented Jan 27, 2024

StellarBot commented Feb 7, 2024

HPX Performance

Comparison

Info

Comparison

Info

Comparison

Info

Explanation of Symbols

codacy-production bot commented Feb 7, 2024 • edited

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

biddisco commented Feb 8, 2024

Johan511 commented Feb 8, 2024

Johan511 commented Feb 8, 2024

hkaiser commented Feb 10, 2024 • edited

Johan511 commented Nov 1, 2023 •

edited

codacy-production bot commented Nov 1, 2023 •

edited

Johan511 Nov 2, 2023 •

edited

Johan511 commented Nov 2, 2023 •

edited

codacy-production bot commented Feb 7, 2024 •

edited

hkaiser commented Feb 10, 2024 •

edited