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

Use std::variant instead of mapbox::util::variant #6903

Merged
merged 45 commits into from
May 28, 2024

Conversation

SiarheiFedartsou
Copy link
Member

@SiarheiFedartsou SiarheiFedartsou commented May 25, 2024

We get rid of old unsupported dependency this way + even gain some performance according to some of benchmarks.

Benchmark Results

Benchmark Base PR
alias aliased u32: 1156.49
plain u32: 1165.4
aliased double: 1221.16
plain double: 1194.4
aliased u32: 1173.03
plain u32: 1169.35
aliased double: 1205.46
plain double: 1205.09
json-render String: 6.82666ms
Stringstream: 10.6403ms
Vector: 6.72893ms
String: 6.54718ms
Stringstream: 9.33254ms
Vector: 6.87933ms
match_ch Default radius:
4.52153ms/req at 82 coordinate
0.0551406ms/coordinate
Radius 5m:
4.51387ms/req at 82 coordinate
0.0550472ms/coordinate
Radius 10m:
15.311ms/req at 82 coordinate
0.186719ms/coordinate
Radius 15m:
37.246ms/req at 82 coordinate
0.454219ms/coordinate
Radius 30m:
318.143ms/req at 82 coordinate
3.8798ms/coordinate
Default radius:
4.60197ms/req at 82 coordinate
0.0561215ms/coordinate
Radius 5m:
4.81285ms/req at 82 coordinate
0.0586933ms/coordinate
Radius 10m:
15.2201ms/req at 82 coordinate
0.185611ms/coordinate
Radius 15m:
37.3425ms/req at 82 coordinate
0.455396ms/coordinate
Radius 30m:
317.949ms/req at 82 coordinate
3.87743ms/coordinate
match_mld Default radius:
2.8728ms/req at 82 coordinate
0.0350341ms/coordinate
Radius 5m:
2.88596ms/req at 82 coordinate
0.0351947ms/coordinate
Radius 10m:
10.6612ms/req at 82 coordinate
0.130015ms/coordinate
Radius 15m:
26.8552ms/req at 82 coordinate
0.327502ms/coordinate
Radius 30m:
314.604ms/req at 82 coordinate
3.83663ms/coordinate
Default radius:
2.82204ms/req at 82 coordinate
0.0344151ms/coordinate
Radius 5m:
2.81336ms/req at 82 coordinate
0.0343093ms/coordinate
Radius 10m:
10.4019ms/req at 82 coordinate
0.126853ms/coordinate
Radius 15m:
26.7546ms/req at 82 coordinate
0.326275ms/coordinate
Radius 30m:
312.855ms/req at 82 coordinate
3.8153ms/coordinate
packedvector random write:
std::vector 11404.9 ms
util::packed_vector 74442.1 ms
slowdown: 6.5272
random read:
std::vector 11162.7 ms
util::packed_vector 31109.9 ms
slowdown: 2.78697
random write:
std::vector 11509.3 ms
util::packed_vector 78345 ms
slowdown: 6.8071
random read:
std::vector 11427.6 ms
util::packed_vector 34020 ms
slowdown: 2.97701
route_ch 1000 routes, 3 coordinates, no alternatives, overview=full, steps=true
595.822ms
0.595822ms/req
1000 routes, 2 coordinates, no alternatives, overview=full, steps=true
381.316ms
0.381316ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=full, steps=true
755.935ms
0.755935ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false
153.334ms
0.153334ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false
98.6903ms
0.0986903ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false
134.79ms
0.13479ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false, radius=750
153.34ms
0.15334ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false, radius=750
98.5865ms
0.0985865ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false, radius=750
134.563ms
0.134563ms/req
1000 routes, 3 coordinates, no alternatives, overview=full, steps=true
515.086ms
0.515086ms/req
1000 routes, 2 coordinates, no alternatives, overview=full, steps=true
355.196ms
0.355196ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=full, steps=true
635.942ms
0.635942ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false
152.472ms
0.152472ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false
97.3469ms
0.0973469ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false
132.701ms
0.132701ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false, radius=750
152.579ms
0.152579ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false, radius=750
97.9879ms
0.0979879ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false, radius=750
132.23ms
0.13223ms/req
route_mld 1000 routes, 3 coordinates, no alternatives, overview=full, steps=true
728.132ms
0.728132ms/req
1000 routes, 2 coordinates, no alternatives, overview=full, steps=true
467.816ms
0.467816ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=full, steps=true
947.211ms
0.947211ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false
284.93ms
0.28493ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false
165.884ms
0.165884ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false
302.766ms
0.302766ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false, radius=750
281.587ms
0.281587ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false, radius=750
166.466ms
0.166466ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false, radius=750
303.502ms
0.303502ms/req
1000 routes, 3 coordinates, no alternatives, overview=full, steps=true
647.257ms
0.647257ms/req
1000 routes, 2 coordinates, no alternatives, overview=full, steps=true
441.606ms
0.441606ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=full, steps=true
816.873ms
0.816873ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false
276.712ms
0.276712ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false
163.881ms
0.163881ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false
298.151ms
0.298151ms/req
1000 routes, 3 coordinates, no alternatives, overview=false, steps=false, radius=750
275.659ms
0.275659ms/req
1000 routes, 2 coordinates, no alternatives, overview=false, steps=false, radius=750
163.1ms
0.1631ms/req
1000 routes, 2 coordinates, 3 alternatives, overview=false, steps=false, radius=750
298.177ms
0.298177ms/req
rtree 1 result:
209.484ms -> 0.0209484 ms/query
10 results:
244.768ms -> 0.0244768 ms/query
1 result:
208.05ms -> 0.020805 ms/query
10 results:
242.531ms -> 0.0242531 ms/query

@@ -275,15 +275,6 @@ jobs:
CXXCOMPILER: g++-12
CXXFLAGS: '-Wno-array-bounds -Wno-uninitialized'

- name: gcc-11-release
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou May 27, 2024

Choose a reason for hiding this comment

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

GCC is much more capricious when it comes to std::variant. While with newer GCC versions I was able to make it working, but with GCC 11 it seems it doesn't like constructions like this:

struct Array;

using Value = std::variant<Array, ...>;

struct Array {
   std::vector<Value> values;
}

Workaround would be to use smth like boost::recursive_wrapper(event though there is no obvious need in it: compiler here has everything to calculate size of Value), but it would make code more complicated, so I chose the easier way: just drop GCC 11 job in favor of GCC 14 I recently added in another PR :) It seems GCC is getting better here: newer versions still had some issues I had no on Clang, but it was at least easy to fix.

Let me please know if there are any objections here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be ok to drop GCC11. I think in the past™️ we had aimed to track the latest Debian release for minimum compilers. No idea which one that is tho. 😉

If you want to go with a recursive wrapper, here's a write up of mine from also some time ago on how to implement such a wrapper with like five lines of code in standard C++.

@SiarheiFedartsou SiarheiFedartsou marked this pull request as ready for review May 27, 2024 14:56
Copy link
Collaborator

@DennisOSRM DennisOSRM left a comment

Choose a reason for hiding this comment

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

Nice. Good riddance.

@SiarheiFedartsou SiarheiFedartsou merged commit c1ed731 into master May 28, 2024
21 checks passed
@SiarheiFedartsou SiarheiFedartsou deleted the sf-std-variant branch May 28, 2024 16:52
@jcoupey
Copy link

jcoupey commented Nov 7, 2024

I guess I'm a bit late to the party here, but this merge results in a breaking change to the C++ API, see adjustments to the example file.

I definitely see the benefit of dropping a custom implementation to use something supported in the standard, it's just that historically the OSRM project has been very cautious in avoiding breaking changes unless bumping the major release number. Does this mean the next release would have to be v6 something, or simply that the non-breaking policy is somewhat loosened?

@SiarheiFedartsou
Copy link
Member Author

@jcoupey I think worth trying to target v6 - I believe it is not the only breaking change we made lately. There were too many problems with mapbox::variant(e.g. C++20 compatibility, performance)... But it in general are there that many libosrm users out there who would rely on us to be super backward compatible? 🤔
Do you use libosrm directly in VROOM btw?

@jcoupey
Copy link

jcoupey commented Nov 7, 2024

are there that many libosrm users out there

Not sure, but since you never really hear complaints until you break semver, you never know. ;-)

I do use libosrm in VROOM which is why I commented. Downstream we would have to make the change to support the next release, but that would mean dropping support for previous OSRM versions which is not ideal for users. Or we would have to implement some ugly workaround to adjust the calls based on libosrm version, not ideal either.

Either way since OSRM adheres to semver (and especially if there are other breaking changes), it would make sense to bump the major number...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants