-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
b553c63
to
5ce6b34
Compare
429af6a
to
42e9d5b
Compare
@@ -275,15 +275,6 @@ jobs: | |||
CXXCOMPILER: g++-12 | |||
CXXFLAGS: '-Wno-array-bounds -Wno-uninitialized' | |||
|
|||
- name: gcc-11-release |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Good riddance.
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? |
@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? 🤔 |
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... |
We get rid of old unsupported dependency this way + even gain some performance according to some of benchmarks.
Benchmark Results
plain u32: 1165.4
aliased double: 1221.16
plain double: 1194.4
plain u32: 1169.35
aliased double: 1205.46
plain double: 1205.09
Stringstream: 10.6403ms
Vector: 6.72893ms
Stringstream: 9.33254ms
Vector: 6.87933ms
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
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
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
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
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
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
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
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
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
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
209.484ms -> 0.0209484 ms/query
10 results:
244.768ms -> 0.0244768 ms/query
208.05ms -> 0.020805 ms/query
10 results:
242.531ms -> 0.0242531 ms/query