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.

Sign up for GitHub

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

Jump to bottom

replace robin-hood-hashing with unordered_dense #4551

Open
nilsnolde opened this issue Feb 4, 2024 · 8 comments · May be fixed by #4552
Open

replace robin-hood-hashing with unordered_dense #4551

nilsnolde opened this issue Feb 4, 2024 · 8 comments · May be fixed by #4552
Labels
enhancement

Comments

@nilsnolde
Copy link
Member

Seems to be a drop-in replacement by the same dev: https://github.com/martinus/unordered_dense. There's a cpp file but that seems to be providing some C++20 specific modularization stuff, so not needed yet. I'll quickly give it a spin, would be nice to switch.
@nilsnolde nilsnolde linked a pull request Feb 4, 2024 that will close this issue
Open
@michaelkirk
Copy link
Contributor

michaelkirk commented Apr 19, 2024 •
edited

I wanted to note (and for searchability), the tests currently fail to build for me due to some deprecated code in robin-hood-hashing:

[188/502] Building CXX object test/CMakeFiles/idtable.dir/idtable.cc.o
FAILED: test/CMakeFiles/idtable.dir/idtable.cc.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DAUTO_DOWNLOAD=0 -DBOOST_ALLOW_DEPRECATED_HEADERS -DBOOST_BIND_GLOBAL_PLACEHOLDERS -DBOOST_NO_CXX11_SCOPED_ENUMS -DDATA_TOOLS -DENABLE_GDAL -DENABLE_HTTP -DHAS_REMOTE_API=0 -DPROTOBUF_USE_DLLS -DVALHALLA_BUILD_DIR=\"/home/blah/src/valhalla/cmake-build-debug/\" -DVALHALLA_SOURCE_DIR=\"/home/blah/src/valhalla/\" -I/home/blah/src/valhalla/test -I/home/blah/src/valhalla/test/gurka -I/home/blah/src/valhalla/third_party/protozero/include -I/home/blah/src/valhalla/third_party/just_gtfs/include -I/home/blah/src/valhalla/third_party/libosmium/include -I/home/blah/src/valhalla/third_party/robin-hood-hashing/src/include -I/home/blah/src/valhalla/third_party/microtar/src -I/home/blah/src/valhalla -I/home/blah/src/valhalla/src -I/home/blah/src/valhalla/valhalla -I/home/blah/src/valhalla/cmake-build-debug/src -I/home/blah/src/valhalla/cmake-build-debug/src/valhalla -I/home/blah/src/valhalla/third_party/rapidjson/include -I/home/blah/src/valhalla/third_party/date/include -I/home/blah/src/valhalla/third_party/cpp-statsd-client/include -isystem /opt/homebrew/include -isystem /opt/homebrew/Cellar/lz4/1.9.4/include -isystem /opt/homebrew/Cellar/libspatialite/5.1.0/include -isystem /opt/homebrew/opt/sqlite/include -isystem /opt/homebrew/Cellar/luajit/2.1.1710088188/include/luajit-2.1 -isystem /home/blah/src/valhalla/third_party/googletest/googletest/include -isystem /home/blah/src/valhalla/third_party/googletest/googletest -isystem /home/blah/src/valhalla/third_party/googletest/googlemock/include -isystem /home/blah/src/valhalla/third_party/googletest/googlemock -isystem /opt/homebrew/Cellar/geos/3.12.1/include -fcolor-diagnostics  -g -std=gnu++17 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk -fcolor-diagnostics -F/Library/Frameworks  -Wall -Werror -MD -MT test/CMakeFiles/idtable.dir/idtable.cc.o -MF test/CMakeFiles/idtable.dir/idtable.cc.o.d -o test/CMakeFiles/idtable.dir/idtable.cc.o -c /home/blah/src/valhalla/test/idtable.cc
In file included from /home/blah/src/valhalla/test/idtable.cc:1:
In file included from /home/blah/src/valhalla/test/../src/mjolnir/idtable.h:10:
/home/blah/src/valhalla/third_party/robin-hood-hashing/src/include/robin_hood.h:1445:33: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins]
        Cloner<Table, IsFlat && ROBIN_HOOD_IS_TRIVIALLY_COPYABLE(Node)>()(o, *this);
                                ^
/home/blah/src/valhalla/third_party/robin-hood-hashing/src/include/robin_hood.h:210:51: note: expanded from macro 'ROBIN_HOOD_IS_TRIVIALLY_COPYABLE'
#    define ROBIN_HOOD_IS_TRIVIALLY_COPYABLE(...) __has_trivial_copy(__VA_ARGS__)
                                                  ^
1 error generated.

Platform: mac

$ uname
Darwin Kernel Version 23.4.0: Fri Mar 15 00:10:42 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T6000 arm64

$ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ --version
Apple clang version 15.0.0 (clang-1500.3.9.4)
Target: arm64-apple-darwin23.4.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@kevinkreiser
Copy link
Member

in this case its a warning being turned into an error which im just about extremely sick of dealing with. you can avoid it on your platform by turning off -Werror which you can do when you run cmake by passing different options we have set up. actually though im pretty sure we have borked the way we add these thirdparty things to our include path. i think a simply cmakelists.txt fix can make this work for you but since i dont ahve your platform its hard for me to verify.. let me try to make a pr for you to try out

@kevinkreiser
Copy link
Member

hmm no, it seems i had vicky do that already as a suggestion in her pr long ago.. see here:

valhalla/src/CMakeLists.txt

Line 66 in c835e2c

target_include_directories(${library} SYSTEM ${MODULE_SYSTEM_INCLUDE_DIRECTORIES})

@michaelkirk can you build with VERBOSE=1 make then we can see if its properly using the thirdparty dir with -isystem

@nilsnolde
Copy link
Member Author

Ah the tests might actually do another include (which is pointless as long as we’re using the valhalla library target for including directories).

@nilsnolde
Copy link
Member Author

can you try #4698 @michaelkirk ?

@michaelkirk
Copy link
Contributor

you can avoid it on your platform by turning off -Werror which you can do when you run cmake by passing different options we have set up.

I'd assumed this behavior could be tuned with one or both of these options:

option(ENABLE_COMPILER_WARNINGS "Build with compiler warnings" OFF)
option(ENABLE_WERROR "Convert compiler warnings to errors. Requires ENABLE_COMPILER_WARNINGS=ON to take effect" OFF)

But they are both disabled by default, and I haven't enabled either of them that I know of. So I haven't yet figured out how to work around this. I'm unfamiliar with C/C++ development, and it's likely I'm doing something dumb. Here's the build command I'm running to try to convince myself that nothing is being cached:

cd valhalla
(rm -fr build \
    && mkdir build \
    && cd build \
    # I'm also having trouble building prime_server, so I've disabled things that rely on it for now. 
    && cmake .. -DENABLE_HTTP=OFF -DENABLE_SERVICES=OFF \
    && make -j$(nproc) tests)

can you build with VERBOSE=1 make then we can see if its properly using the thirdparty dir with -isystem

Here's the output of the erroring task with VERBOSE=1 make tests

/Applications/Xcode.app/Contents/Developer/usr/bin/make  -f test/CMakeFiles/idtable.dir/build.make test/CMakeFiles/idtable.dir/build
[ 91%] Building CXX object test/CMakeFiles/idtable.dir/idtable.cc.o
cd /Users/mkirk/src/headwaymaps/valhalla/build/test && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DAUTO_DOWNLOAD=0 -DBOOST_ALLOW_DEPRECATED_HEADERS -DBOOST_BIND_GLOBAL_PLACEHOLDERS -DBOOST_NO_CXX11_SCOPED_ENUMS -DDATA_TOOLS -DENABLE_GDAL -DENABLE_HTTP -DHAS_REMOTE_API=0 -DPROTOBUF_USE_DLLS -DVALHALLA_BUILD_DIR=\"/Users/mkirk/src/headwaymaps/valhalla/build/\" -DVALHALLA_SOURCE_DIR=\"/Users/mkirk/src/headwaymaps/valhalla/\" -I/Users/mkirk/src/headwaymaps/valhalla/test -I/Users/mkirk/src/headwaymaps/valhalla/test/gurka -I/Users/mkirk/src/headwaymaps/valhalla/third_party/protozero/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/just_gtfs/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/libosmium/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/robin-hood-hashing/src/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/microtar/src -I/Users/mkirk/src/headwaymaps/valhalla -I/Users/mkirk/src/headwaymaps/valhalla/src -I/Users/mkirk/src/headwaymaps/valhalla/valhalla -I/Users/mkirk/src/headwaymaps/valhalla/build/src -I/Users/mkirk/src/headwaymaps/valhalla/build/src/valhalla -I/Users/mkirk/src/headwaymaps/valhalla/third_party/rapidjson/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/date/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/cpp-statsd-client/include -isystem /opt/homebrew/include -isystem /opt/homebrew/Cellar/lz4/1.9.4/include -isystem /opt/homebrew/Cellar/libspatialite/5.1.0/include -isystem /opt/homebrew/opt/sqlite/include -isystem /opt/homebrew/Cellar/luajit/2.1.1710088188/include/luajit-2.1 -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googletest/include -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googletest -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googlemock/include -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googlemock -isystem /opt/homebrew/Cellar/geos/3.12.1/include -fcolor-diagnostics  -O3 -DNDEBUG -std=gnu++17 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk -F/Library/Frameworks  -Wall -Werror -MD -MT test/CMakeFiles/idtable.dir/idtable.cc.o -MF CMakeFiles/idtable.dir/idtable.cc.o.d -o CMakeFiles/idtable.dir/idtable.cc.o -c /Users/mkirk/src/headwaymaps/valhalla/test/idtable.cc
In file included from /Users/mkirk/src/headwaymaps/valhalla/test/idtable.cc:1:
In file included from /Users/mkirk/src/headwaymaps/valhalla/test/../src/mjolnir/idtable.h:10:
/Users/mkirk/src/headwaymaps/valhalla/third_party/robin-hood-hashing/src/include/robin_hood.h:1445:33: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins]
        Cloner<Table, IsFlat && ROBIN_HOOD_IS_TRIVIALLY_COPYABLE(Node)>()(o, *this);
                                ^
/Users/mkirk/src/headwaymaps/valhalla/third_party/robin-hood-hashing/src/include/robin_hood.h:210:51: note: expanded from macro 'ROBIN_HOOD_IS_TRIVIALLY_COPYABLE'
#    define ROBIN_HOOD_IS_TRIVIALLY_COPYABLE(...) __has_trivial_copy(__VA_ARGS__)
                                                  ^
1 error generated.
make[3]: *** [test/CMakeFiles/idtable.dir/idtable.cc.o] Error 1
make[2]: *** [test/CMakeFiles/idtable.dir/all] Error 2
make[1]: *** [test/CMakeFiles/tests.dir/rule] Error 2
make: *** [tests] Error 2

can you try #4698 @michaelkirk ?

Same failure with #4698 - heres the output of VERBOSE=1 make tests

Consolidate compiler generated dependencies of target idtable
/Applications/Xcode.app/Contents/Developer/usr/bin/make  -f test/CMakeFiles/idtable.dir/build.make test/CMakeFiles/idtable.dir/build
[ 91%] Building CXX object test/CMakeFiles/idtable.dir/idtable.cc.o
cd /Users/mkirk/src/headwaymaps/valhalla/build/test && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DAUTO_DOWNLOAD=0 -DBOOST_ALLOW_DEPRECATED_HEADERS -DBOOST_BIND_GLOBAL_PLACEHOLDERS -DBOOST_NO_CXX11_SCOPED_ENUMS -DDATA_TOOLS -DENABLE_GDAL -DENABLE_HTTP -DHAS_REMOTE_API=0 -DPROTOBUF_USE_DLLS -DVALHALLA_BUILD_DIR=\"/Users/mkirk/src/headwaymaps/valhalla/build/\" -DVALHALLA_SOURCE_DIR=\"/Users/mkirk/src/headwaymaps/valhalla/\" -I/Users/mkirk/src/headwaymaps/valhalla/test -I/Users/mkirk/src/headwaymaps/valhalla/test/gurka -I/Users/mkirk/src/headwaymaps/valhalla/test/SYSTEM -I/Users/mkirk/src/headwaymaps/valhalla/third_party/robin-hood-hashing/src/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/just_gtfs/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/protozero/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/libosmium/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/microtar/src -I/Users/mkirk/src/headwaymaps/valhalla -I/Users/mkirk/src/headwaymaps/valhalla/src -I/Users/mkirk/src/headwaymaps/valhalla/valhalla -I/Users/mkirk/src/headwaymaps/valhalla/build/src -I/Users/mkirk/src/headwaymaps/valhalla/build/src/valhalla -I/Users/mkirk/src/headwaymaps/valhalla/third_party/rapidjson/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/date/include -I/Users/mkirk/src/headwaymaps/valhalla/third_party/cpp-statsd-client/include -isystem /opt/homebrew/include -isystem /opt/homebrew/Cellar/lz4/1.9.4/include -isystem /opt/homebrew/Cellar/libspatialite/5.1.0/include -isystem /opt/homebrew/opt/sqlite/include -isystem /opt/homebrew/Cellar/luajit/2.1.1710088188/include/luajit-2.1 -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googletest/include -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googletest -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googlemock/include -isystem /Users/mkirk/src/headwaymaps/valhalla/third_party/googletest/googlemock -isystem /opt/homebrew/Cellar/geos/3.12.1/include -fcolor-diagnostics  -O3 -DNDEBUG -std=gnu++17 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk -F/Library/Frameworks  -Wall -Werror -MD -MT test/CMakeFiles/idtable.dir/idtable.cc.o -MF CMakeFiles/idtable.dir/idtable.cc.o.d -o CMakeFiles/idtable.dir/idtable.cc.o -c /Users/mkirk/src/headwaymaps/valhalla/test/idtable.cc
In file included from /Users/mkirk/src/headwaymaps/valhalla/test/idtable.cc:1:
In file included from /Users/mkirk/src/headwaymaps/valhalla/test/../src/mjolnir/idtable.h:10:
/Users/mkirk/src/headwaymaps/valhalla/third_party/robin-hood-hashing/src/include/robin_hood.h:1445:33: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins]
        Cloner<Table, IsFlat && ROBIN_HOOD_IS_TRIVIALLY_COPYABLE(Node)>()(o, *this);
                                ^
/Users/mkirk/src/headwaymaps/valhalla/third_party/robin-hood-hashing/src/include/robin_hood.h:210:51: note: expanded from macro 'ROBIN_HOOD_IS_TRIVIALLY_COPYABLE'
#    define ROBIN_HOOD_IS_TRIVIALLY_COPYABLE(...) __has_trivial_copy(__VA_ARGS__)
                                                  ^
1 error generated.
make[3]: *** [test/CMakeFiles/idtable.dir/idtable.cc.o] Error 1
make[2]: *** [test/CMakeFiles/idtable.dir/all] Error 2
make[1]: *** [test/CMakeFiles/tests.dir/rule] Error 2
make: *** [tests] Error 2

I think the only difference with #4698 is the inclusion of this arg:
-I/Users/mkirk/src/headwaymaps/valhalla/test/SYSTEM

@michaelkirk
Copy link
Contributor

With this little addendum, #4698 does the trick!

Thanks both. I still don't understand why the the compiler warnings are causing the build to fail when I haven't explicitly set ENABLE_COMPILER_WARNINGS/ENABLE_WERROR, but that's a mystery for another day.

@nilsnolde
Copy link
Member Author

yeah, it's a bit convoluted.. ENABLE_SINGLE_FILES_WERROR is what affects you here and defaults to ON. that exists because the others are much to wide in scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

move from robin-hood-hashing to unordered_dense library valhalla/valhalla
3 participants
@michaelkirk @kevinkreiser @nilsnolde