Epeck thread safety by mglisse · Pull Request #5402 · CGAL/cgal

mglisse · 2021-01-27T17:54:32Z

Summary of Changes

A preview of a prototype of some changes to make Epeck safer in multi-threaded applications, in case some people want to play with it, look at the code, or discuss the topic. Alternative to #648, it makes more sense as a separate PR than a comment at the end of that PR.
It is surprising that the hardest part is cleaning up. If we were ok with not pruning the Lazy trees after update_exact (or if we had garbage collection?), other strategies would become possible. Anyway, call_once seems rather efficient and once_flag is small, at least on linux (it is bigger on windows), and it avoids reinventing the (spinning) wheel.
I think the worst overhead is for operations on Lazy_exact_nt, but people shouldn't do a lot of computations in that type.
One thing I hope is that the overhead will prove small enough that we don't need a thread-unsafe version for use in multi-threaded applications that do not share objects.
This change means that copying a Handle(_for) is now expensive, so work to reduce copies (for instance for mpq_rational) would also benefit reference-counted types (Gmpq, Lazy).

Some possible future steps:

benchmarks (single-thread), also check memory use. Careful that glibc-2.32+ may have very different perf than the rest.
write a test (for validity) and a benchmark with parallelism and a significant amount of contention
benchmark also on windows (and mac?), things like call_once could have wildly different behavior
test on non-x86 hardware (memory ordering on x86 is special, and thread sanitizer only goes so far)
discuss configuration: how should this be enabled? (CMake has the imported target Threads::Threads defined in FindThreads, this is already used in CGAL_TBB and CGAL_Eigen3 support files, see also THREADS_PREFER_PTHREAD_FLAG) Should there be a CMake option defined before find_package(CGAL) to enable it? Or a CGAL::Threads component that users need to request?
fix the FIXMEs, do the TODOs, etc

Release Management

Affected package(s): Filtered_kernel, STL_Extension, Installation, Number_types
Issue(s) solved (if any): fix Concurrently readable lazy-evaluated number type in CGAL #4340 fix Read-safe multithreading with Lazy_exact_nt #2685
Feature/Small Feature (if any):
Link to compiled documentation (obligatory for small feature) wrong link name to be changed
License and copyright ownership:

That's cheaper than the current t=T() which includes several atomic ops For the cases I changed, it was clear that T was Lazy_*. In Lazy.h, there are other cases where one would need to check what the valid types are, and maybe create a reset utility which does t=T() by default and is overloaded for types that derive from Handle.

The exact choice of memory_order_* is hard, and even for versions that seem to work on x86_64 TSAN complains like crazy. TODO: a version that, instead of blocking if another thread is updating, also does the computation, with suitable protection to ensure that pruning the tree cannot happen while anyone is computing on it. TODO: try having AT in Lazy instead of Lazy_rep (i.e. not shared) to avoid the indirection. We can possibly choose one or the other depending on the type.

This doesn't seem to provide anything useful with the current implementations. It probably helped avoid an atomic operation in some earlier version.

lrineau · 2021-07-27T13:25:21Z

Benchs of this PR seems good. I will merge it.

The issue is fixed by CGAL/cgal#5402. Unfortunately it's not officially released yet, so this is an internal build.

Supranaturaler · 2021-10-29T08:35:11Z

@mglisse Thank you for the update. Is it possible to apply this change to CGAL5.2.3？

sloriot · 2021-10-29T09:06:16Z

Considering how invasive is the patch, I wonder what could prevent you from switching to the master branch (and 5.4 when released).

lrineau · 2021-10-29T09:14:37Z

By the way, I think the change of this PR should be announced in CHANGES.md.

Supranaturaler · 2021-10-29T09:52:11Z

Considering how invasive is the patch, I wonder what could prevent you from switching to the master branch (and 5.4 when released).

I have tried 5.3 and find that boolean set operations become slower. I don't tend to upgrade right now. Is there any solution
to use exact_predicates_exact_constructions_kernel in multithread?

sloriot · 2021-10-29T09:57:52Z

Could you share a case that shows the regression? We have been working recently to improve the runtime of boolean set operations so it might show a bug. Thanks.

mglisse · 2021-10-29T09:59:11Z

Could you share a case that shows the regression?

Indeed, please open a new issue about this.

By the way, I think the change of this PR should be announced in CHANGES.md.

Uh, I thought we had written something, but apparently not...

Supranaturaler · 2021-10-29T10:11:39Z

Could you share a case that shows the regression? We have been working recently to improve the runtime of boolean set operations so it might show a bug. Thanks.

OK. I will test a simple case and make sure this is a problem. Later I will open a new issue about this.

mglisse · 2021-10-29T10:15:16Z

CHANGES.md: in the Kernel_23 section:

Most operations on Exact_predicates_exact_constructions_kernel objects are now thread-safe if CGAL::Exact_rational is mpq_class (from GMPXX), boost::multiprecision::mpq_rational or CGAL::Quotient<CGAL::MP_Float>. The objects are not atomic though, so the usual restrictions on avoiding race conditions apply. For users who do not use threads, this can be disabled with CGAL_HAS_NO_THREADS.

and in the section "dD Geometry Kernel":

Most operations on Epeck_d objects are now thread-safe, see 2D and 3D Linear Geometry Kernel for details.

?

("most" because for instance Aff_transformation_3 still has an unsafe reference count, and I am not sure if there may be other hidden problems)

lrineau · 2021-10-29T12:10:06Z

CHANGES.md: in the Kernel_23 section:

Most operations on Exact_predicates_exact_constructions_kernel objects are now thread-safe if CGAL::Exact_rational is mpq_class (from GMPXX), boost::multiprecision::mpq_rational or CGAL::Quotient<CGAL::MP_Float>. The objects are not atomic though, so the usual restrictions on avoiding race conditions apply. For users who do not use threads, this can be disabled with CGAL_HAS_NO_THREADS.

and in the section "dD Geometry Kernel":

Most operations on Epeck_d objects are now thread-safe, see 2D and 3D Linear Geometry Kernel for details.

?

("most" because for instance Aff_transformation_3 still has an unsafe reference count, and I am not sure if there may be other hidden problems)

Where do you see that? It seems it was never committed, or never pushed.

mglisse · 2021-10-29T12:21:57Z

Where do you see that?

Ah no, sorry, that wasn't clear enough, I was suggesting that we could add that, not saying that it was already there.

Supranaturaler · 2021-10-29T14:45:18Z

Could you share a case that shows the regression? We have been working recently to improve the runtime of boolean set operations so it might show a bug. Thanks.
@sloriot @mglisse

Please refer to #6092 . As I can't upgrade to higher version, I wonder if I could merge some of the code in #5402 to CGAL 5.2.3 to solve the multithread problem? If so, which code should I merge?

sloriot · 2021-10-29T14:48:32Z

I already did that merge somewhere but I'd need some time to get a non-official patched release.

Supranaturaler · 2021-10-29T14:56:15Z

I already did that merge somewhere but I'd need some time to get a non-official patched release.

Thank you for your hard work. The patched release of CGAL 5.2.3 is sorely needed for me.

sloriot · 2021-10-29T16:36:28Z

@Supranaturaler You can try https://cgal.geometryfactory.com/~sloriot/CGAL-5.2.2-mt.tgz. I hope I did not mess up while setting it up. Note that it is not official and patched master should be the best choice.

Supranaturaler · 2021-10-29T16:48:23Z

@Supranaturaler You can try https://cgal.geometryfactory.com/~sloriot/CGAL-5.2.2-mt.tgz. I hope I did not mess up while setting it up. Note that it is not official and patched master should be the best choice.

Sorry, the url is unreacheable.

Supranaturaler · 2021-11-02T15:56:17Z

@sloriot plz offer the right url to me, thanks!

sloriot · 2021-11-02T16:21:43Z

Should be fine now.

Supranaturaler · 2021-11-03T09:20:41Z

Should be fine now.

Compile error. You should remove line 217 in Lazy_kernel.h

Anyway, it worked in multithread test. Thank you for all this.

mglisse added 30 commits November 13, 2020 19:39

Use a constructor for RefCounted

511103e

Constructor for Rep

695fe37

atomic count in Handle_for

3edf7a8

Refactor ++ and -- in Handle

29c9fd3

atomic count in Handle

0bd0e7e

atomic update_exact

c862e7b

Indirection for the approximate value in Lazy

7151181

set_exact minor refactor

ba054d5

conditional code for TSAN

6338f89

alternate (not working) version

6c2bf2c

Use default-constructed Lazy more.

b88dd3b

mention std::call_once

9d554b9

call_once version

2112f00

improve approximate type

d7555a3

Special-case intervals

ad34659

Destructor

9e160d6

other version

d4ad295

unsafe version for intervals

bbc7891

More uniform interface

d41c421

General unsafe version

acec599

Mark some classes final

5ccab34

Missing Lazy_exact_Ex_Cst

b752dd3

comment on Lazy_exact_nt

e9ee3ae

comment on static filters in Epeck_d

398a228

comment about __libc_single_threaded

a513a48

selector for the Lazy_rep implementation

60891a9

Remove is_point wrapper

f90cec8

This doesn't seem to provide anything useful with the current implementations. It probably helped avoid an atomic operation in some earlier version.

Unused #include

40ff696

lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Jul 27, 2021

lrineau merged commit b1ccf3f into CGAL:master Jul 27, 2021

lrineau removed Ready to be tested Under Testing labels Jul 27, 2021

lrineau deleted the mt-glisse branch July 27, 2021 14:11

lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Jul 27, 2021

alecjacobson mentioned this pull request Sep 20, 2021

Bump CGAL → Boolean + remesh_*intersections performance boost libigl/libigl#1895

Merged

aabtop added a commit to aabtop/reify that referenced this pull request Sep 20, 2021

Fix CGAL race condition by using a later release.

7534593

The issue is fixed by CGAL/cgal#5402. Unfortunately it's not officially released yet, so this is an internal build.

lrineau added the Merged_in_5.4-beta1 label Jan 3, 2022

lrineau added the Merged_in_5.4 label Jan 31, 2022

lrineau mentioned this pull request Jan 27, 2023

Failure when compiling with static libs #7192

Closed

Conversation

mglisse commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Release Management

Uh oh!

lrineau commented Jul 27, 2021

Uh oh!

Supranaturaler commented Oct 29, 2021

Uh oh!

sloriot commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrineau commented Oct 29, 2021

Uh oh!

Supranaturaler commented Oct 29, 2021

Uh oh!

sloriot commented Oct 29, 2021

Uh oh!

mglisse commented Oct 29, 2021

Uh oh!

Supranaturaler commented Oct 29, 2021

Uh oh!

mglisse commented Oct 29, 2021

Uh oh!

lrineau commented Oct 29, 2021

Uh oh!

mglisse commented Oct 29, 2021

Uh oh!

Supranaturaler commented Oct 29, 2021 • edited by MaelRL Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloriot commented Oct 29, 2021

Uh oh!

Supranaturaler commented Oct 29, 2021

Uh oh!

sloriot commented Oct 29, 2021

Uh oh!

Supranaturaler commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Supranaturaler commented Nov 2, 2021

Uh oh!

sloriot commented Nov 2, 2021

Uh oh!

Supranaturaler commented Nov 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mglisse commented Jan 27, 2021 •

edited

Loading

sloriot commented Oct 29, 2021 •

edited

Loading

Supranaturaler commented Oct 29, 2021 •

edited by MaelRL

Loading

Supranaturaler commented Oct 29, 2021 •

edited

Loading