Improvement backports from CDT_3 branch (Follow-up to PR #8170) #8273
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
lrineau
commented
Jun 13, 2024
Triangulation_2/include/CGAL/Triangulation_2/internal/Polyline_constraint_hierarchy_2.h
Outdated
Show resolved
Hide resolved
lrineau
added a commit
to lrineau/cgal
that referenced
this pull request
Jul 1, 2024
lrineau
changed the title
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
...and `refactor Polyline_constraint_hierarchy_2` to use it. `CGAL::unordered_flat_map` will be Boost `unordered_flat_map` if availlable, or the standard `std::unordered_map` otherwise.
To debug non-determinism on Linux platforms.
lrineau
commented
Jan 10, 2025
Triangulation_2/include/CGAL/Triangulation_2/internal/Polyline_constraint_hierarchy_2.h
Outdated
Show resolved
Hide resolved
That uses Boost.STLInterfaces from Boost >= 1.74.
That code was never used nor tested, and cannot compile anyway.
- remove all mentions of `Edge` and `Constraint` - `Subconstraint_iterator` is renamed `Subconstraint_and_contexts_iterator` (because of its value type) - a new `Subconstraint_iterator`, with value type `Subconstraint` - a few unused/untested and uncompilable functions are removed from the code - a lot of internal renamings == Breaking changes == For `Constrained_triangulation_plus_2`, there are a few breaking changes... - The value type of `subconstraints_begin()`, `subconstraints_end()`, of the range `subconstraints()` has changed to `Subconstraint` (a simple `std::pair` of vertex handles). That is actually a kind of bug-fix, because it was documented as such in the user manual. - The new member functions `subconstraints_and_contexts_begin()`, `subconstraints_and_contexts_end()`, `subconstraints_and_contexts()` are created get the old value type (`std::pair<const Subconstraint, std::list<Context>*>`). - A few range types have changed from `CGAL::Iterator_range<It>` to `unspecified_type`, for efficiency reasons. - Doc fixes. == Determinism == Even if it was not documented, the range `subconstraints()` is deterministic (used by Mesh_2), and `subconstraints_and_contexts()` is not.
It is invalid to try to detect if an iterator is singular or value-initialized.
The only operations allowed with value-initialized iterators are:
- copy the iterator, or
- destroy or assign the iterator.
Comparisons like `vertex_it == Vertex_it{}` are not allowed.
The code was doing bad operations with value-initialized iterators. Fixed in d837dbd.
With C++20, the behavior of Boost.STLinterface was different. I have fixed, and tested with and without C++20. And I found a bug in the macro
A known issue with MSVC 2017 (see the repro). Fixed in 571c2cc as well (it should have been a distinct commit). |
Using the tool `compare.py` from [Google Benchmark Tools][1]... [1]: https://github.com/google/benchmark/blob/main/docs/tools.md ...given that: - `benchmark_simplify-AFTER` is the file `benchmark_simplify.cpp` compiled from this branch, and flags `-msse3 -DNDEBUG -O3` with gcc version 14.2.1, - `benchmark_simplify-BEFORE` is the same file compiled from the branch `master`, and same flags and compiler. the command line was: ```shell compare.py benchmarks build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-BEFORE build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-AFTER Data/data/wkt/norway-MP.wkt --benchmark_repetitions=20 --benchmark_counters_tabular=true --benchmark_time_unit=s ``` The following results show that there is a speedup of about 15% (mean) or 18% (median) ``` RUNNING: build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-BEFORE Data/data/wkt/norway-MP.wkt --benchmark_repetitions=20 --benchmark_counters_tabular=true --benchmark_time_unit=s --benchmark_out=/tmp/tmpvxyq_i24 2025-02-06T17:23:08+01:00 Running Data/data/wkt/norway-MP.wkt Run on (16 X 800 MHz CPU s) CPU Caches: L1 Data 48 KiB (x8) L1 Instruction 32 KiB (x8) L2 Unified 1280 KiB (x8) L3 Unified 24576 KiB (x1) Load Average: 1.47, 2.07, 1.65 ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Benchmark Time CPU Iterations #points #polygons #polylines nb of constraints nb of sub-constraints nb of vertices ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- simplify file Data/data/wkt/norway-MP.wkt 0.067 s 0.067 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.066 s 0.066 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.065 s 0.065 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.073 s 0.072 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.066 s 0.066 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.064 s 0.064 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.067 s 0.066 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.071 s 0.071 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.064 s 0.064 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.064 s 0.064 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.074 s 0.074 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.066 s 0.066 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.075 s 0.074 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.074 s 0.074 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.076 s 0.076 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.078 s 0.078 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.074 s 0.074 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.067 s 0.067 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.064 s 0.064 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.069 s 0.069 s 12 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt_mean 0.069 s 0.069 s 20 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt_median 0.067 s 0.067 s 20 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt_stddev 0.005 s 0.005 s 20 0 0 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt_cv 6.73 % 6.68 % 20 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% RUNNING: build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-AFTER Data/data/wkt/norway-MP.wkt --benchmark_repetitions=20 --benchmark_counters_tabular=true --benchmark_time_unit=s --benchmark_out=/tmp/tmp59ui0dsw 2025-02-06T17:29:26+01:00 Running Data/data/wkt/norway-MP.wkt Run on (16 X 800.961 MHz CPU s) CPU Caches: L1 Data 48 KiB (x8) L1 Instruction 32 KiB (x8) L2 Unified 1280 KiB (x8) L3 Unified 24576 KiB (x1) Load Average: 1.61, 1.94, 1.73 ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Benchmark Time CPU Iterations #points #polygons #polylines nb of constraints nb of sub-constraints nb of vertices ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- simplify file Data/data/wkt/norway-MP.wkt 0.054 s 0.054 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.057 s 0.057 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.059 s 0.059 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.065 s 0.065 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.058 s 0.058 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.056 s 0.056 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.053 s 0.053 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.061 s 0.061 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.058 s 0.058 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.052 s 0.052 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.054 s 0.054 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.055 s 0.054 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.054 s 0.054 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.055 s 0.054 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.054 s 0.054 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.059 s 0.059 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.057 s 0.057 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.058 s 0.057 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.055 s 0.055 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt 0.059 s 0.059 s 13 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt_mean 0.057 s 0.057 s 20 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt_median 0.057 s 0.056 s 20 0 848 0 849 40.568k 40.565k simplify file Data/data/wkt/norway-MP.wkt_stddev 0.003 s 0.003 s 20 0 0 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt_cv 5.37 % 5.32 % 20 0.00% 0.00% 0.00% 0.00% 0.00% 0.00% Comparing build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-BEFORE to build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-AFTER Benchmark Time CPU Time Old Time New CPU Old CPU New ----------------------------------------------------------------------------------------------------------------------------------------------- simplify file Data/data/wkt/norway-MP.wkt -0.1941 -0.1942 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1269 -0.1272 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.0900 -0.0918 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1049 -0.1065 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1204 -0.1206 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1279 -0.1282 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1962 -0.1961 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1370 -0.1377 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.0951 -0.0962 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1851 -0.1852 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.2720 -0.2716 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1702 -0.1717 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.2700 -0.2703 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.2644 -0.2634 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.2861 -0.2856 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.2492 -0.2488 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.2310 -0.2301 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1382 -0.1389 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1401 -0.1406 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt -0.1464 -0.1467 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt_pvalue 0.0000 0.0000 U Test, Repetitions: 20 vs 20 simplify file Data/data/wkt/norway-MP.wkt_mean -0.1804 -0.1806 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt_median -0.1547 -0.1549 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt_stddev -0.3458 -0.3479 0 0 0 0 simplify file Data/data/wkt/norway-MP.wkt_cv -0.2018 -0.2041 0 0 0 0 OVERALL_GEOMEAN -0.1797 -0.1800 0 0 0 0 ```
BenchmarksThis PR got even bigger now. I had to:
ResultsUsing The results can be seen at https://cgal.geometryfactory.com/~lrineau/simplify-bench.html
Details in text (click for more...)
Using the tool ...given that:
the command line was: compare.py benchmarks build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-BEFORE build/Release/benchmark/Polyline_simplification_2/benchmark_simplify-AFTER Data/data/wkt/norway-MP.wkt --benchmark_repetitions=20 --benchmark_counters_tabular=true --benchmark_time_unit=sThe following results show that there is a speedup |
|
@lrineau could you please fix conflicts? |
lrineau
commented
Feb 10, 2025
There was a problem hiding this comment.
Sorry for the mess in https://cgal.geometryfactory.com/CGAL/testsuite/results-6.1-Ic-81.shtml. It was because of this PR. Now fixed, and tested locally.
|
Successfully tested in CGAL-6.1-Ic-84 (and 85) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of Changes
In the PR #8170, merged for CGAL-6.0, there was several commits that I had to revert, because they broke the testsuite results:
save_binary_file. That was no correct as regards to the C3t3 concept. It is now fixed by commit c10dcf7 from this PR.Polyline_constraint_hierarchy_2using an unordered map, instead of astd::map. The issue was that is broke the determinism of Mesh_2. See Improvement backports from CDT_3 branch (Follow-up to PR #8170) #8273 (comment).CGAL::Segment_3, for some CGAL kernels.This pull-request reintroduces those features, for CGAL-6.1.
Release Management