Skip to content

Filtered kernel: Epeck reentrance#648

Closed
lrineau wants to merge 24 commits intoCGAL:masterfrom
lrineau:Filtered_kernel-Epeck_reentrance-GF
Closed

Filtered kernel: Epeck reentrance#648
lrineau wants to merge 24 commits intoCGAL:masterfrom
lrineau:Filtered_kernel-Epeck_reentrance-GF

Conversation

@lrineau
Copy link
Copy Markdown
Member

@lrineau lrineau commented Jan 18, 2016

Add re-entrance to the CGAL::Epeck kernel.

  • Use atomic types in <CGAL/Handle_for.h>,
  • Use a mutex in <CGAL/Lazy.h>.

Depends on #642.

Fixes #4340.

@lrineau lrineau added this to the 4.8-beta milestone Jan 18, 2016
@lrineau lrineau changed the title Filtered kernel epeck reentrance gf Filtered kernel: Epeck reentrance Jan 18, 2016
@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Jan 18, 2016

Merged in CGAL-4.8-Ic-102

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Jan 19, 2016

Merged in CGAL-4.8-Ic-104

@lrineau lrineau force-pushed the Filtered_kernel-Epeck_reentrance-GF branch 2 times, most recently from 0214c85 to ae7678a Compare January 19, 2016 17:24
@lrineau lrineau added the Not yet approved The feature or pull-request has not yet been approved. label Jan 20, 2016
@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Jan 20, 2016

I have marked the PR with "not yet approved" because I want the PR to be tested in integration, but not merged in master yet. We have to do benchmarks before.

@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Jan 20, 2016

Merged in CGAL-4.8-Ic-105.

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Jan 21, 2016

I think this branch is responsible for many warnings about c++11.
For example:
warning: variadic templates only available with -std=c++11 or -std=gnu++11

@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Jan 21, 2016

Actually I do not think so. The issue seems to be because I was using g++-4.5.3 with an Boost library version compiled with g++-4.8.

@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Jan 21, 2016

Commits
lrineau@6206c0c and
lrineau@8d83f01 fix the headers with Boost<1.53.

@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Jan 21, 2016

Merged in CGAL-4.8-Ic-106.

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Jan 22, 2016

Merged in CGAL-4.8-Ic-107

@sloriot
Copy link
Copy Markdown
Member

sloriot commented Jan 25, 2016

Fails on platforms 21, 22 and 24.

@lrineau lrineau added this to the 4.13-beta milestone Jan 24, 2018
@luckyrantanplan
Copy link
Copy Markdown

Hi Laurent,
I really hope you will be able to merge this PR soon !
Cheers,

@lrineau lrineau removed this from the 4.13-beta milestone Jun 26, 2018
@lrineau lrineau added this to the Trash / Attic milestone Jan 11, 2019
@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Sep 16, 2019

I have refreshed the branch: merged it with master to solve the conflicts. I plan to work on it this week, during the CGAL developers meeting.

So far, the update of Lazy objects is done using a lock on a mutex. The bad thing is that all readers and writers have to lock the mutex. I have two other ideas, that might be more efficient:

  • have a shared mutex, to have multiple readers and one exclusive writer,
  • or using a pair of atomic<double> in Interval_nt (and 128 bits atomic operations in case SSE is used).

Both three variants will have to be benched, plus master.

@mglisse
Copy link
Copy Markdown
Member

mglisse commented Sep 16, 2019

or using a pair of atomic<double> in Interval_nt (and 128 bits atomic operations in case SSE is used).

Modifying Interval_nt so that all operations are atomic seems like overkill that will badly affect the performance of approximate functors. We only want load/store of approximate objects in Lazy to be atomic-ish (it is not obvious if we will hit bugs if we keep it non-atomic).

Both three

2 = 3 ? 😕

@lrineau

This comment has been minimized.

@mglisse

This comment has been minimized.

The template argument for `Thread_safety_policy` can be:

  - anything, like `void`,
  - `Atomic_reference_counting_tag`, to enable the atomic reference
    counting,
  - `Thread_safe_tag`, to enable the full thread-safety.
@mglisse
Copy link
Copy Markdown
Member

mglisse commented Nov 14, 2020

@lrineau The PoC branch https://github.com/mglisse/cgal/tree/mt-glisse seems to have less overhead than this one currently does. (it is messy, several ideas are not prototyped yet, etc, it is to get an idea of what a reasonable overhead looks like)

@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Nov 16, 2020

@lrineau The PoC branch https://github.com/mglisse/cgal/tree/mt-glisse seems to have less overhead than this one currently does. (it is messy, several ideas are not prototyped yet, etc, it is to get an idea of what a reasonable overhead looks like)

That is indeed very interesting. What are the next steps, now?

@mglisse
Copy link
Copy Markdown
Member

mglisse commented Nov 16, 2020

What are the next steps, now?

I don't know, some possible next steps

  • experiment with some of the ideas I mention in various comments
  • run the complete testsuite
  • benchmark on programs that use construction and currently spend a significant amount of time in update_exact (suggestions?)
  • also check memory use during benchmark
  • write a test (for validity) and a benchmark with parallelism and a significant amount of contention
  • use that benchmark to determine what variant is faster
  • 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)
  • eventually, clean things up and make the whole thing optional (behind macros or tags or something)

@mglisse mglisse mentioned this pull request Jan 27, 2021
@lrineau
Copy link
Copy Markdown
Member Author

lrineau commented Jun 15, 2021

Replaced by #5402.

@lrineau lrineau closed this Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Not yet approved The feature or pull-request has not yet been approved. Pkg::Kernel_d Replaced

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrently readable lazy-evaluated number type in CGAL

6 participants