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

problems with fast crc support #2488

Closed
mdcallag opened this issue Jun 23, 2017 · 7 comments
Closed

problems with fast crc support #2488

mdcallag opened this issue Jun 23, 2017 · 7 comments
Labels
bug Confirmed RocksDB bugs

Comments

@mdcallag
Copy link
Contributor

mdcallag commented Jun 23, 2017

There are a few problems with fast crc support in RocksDB:

  1. LOG can claim the support is enabled even though it isn't used at run time
  2. build_tools/build_detect_platform fails to compile the test code that detects SSE support and -DHAVE_SSE42 isn't added to the compiler command line. Note that RocksDB uses different methods at compile time and run time to determine whether fast crc support is compiled in vs whether it is selected at run time.

Note that an internal bug has already been opened for this.

This is a hack to get the -DHAVE_SSE42 flag added to the build command line. The problem is that the test code won't compile with -std=c++11. Adding $PLATFORM_CXXFLAGS might also fix this.

--- a/build_tools/build_detect_platform
+++ b/build_tools/build_detect_platform
@@ -456,7 +456,7 @@ elif test -z "$PORTABLE"; then
fi
fi

-$CXX $COMMON_FLAGS -x c++ - -o /dev/null 2>/dev/null <<EOF
+$CXX $COMMON_FLAGS -std=c++11 -x c++ - -o /dev/null 2>/dev/null <<EOF
#include
#include <nmmintrin.h>
int main() {

Support for fast crc in RocksDB is complex:

  • HAVE_SSE42 macro used at compile time to determine whether fast crc32 support is compiled
  • SSE4_2 macro used at compile time to determine whether isSSE42 function will be called at run time to decide whether fast crc is used.
  • The isSSE42 function decides at runtime whether fast crc is used

From the above, if the HAVE_SSE42 macro isn't defined then the fast crc function will use slow crc implementation. But LOG depends on output from isSSE42 function that depends on SSE4_2 macro which claims fast crc is supported.

@mdcallag mdcallag added the bug Confirmed RocksDB bugs label Jun 23, 2017
@mdcallag
Copy link
Contributor Author

I wonder whether this is an issue for the distros that bundle RocksDB?

@vadimtk
Copy link

vadimtk commented Jun 23, 2017

Yes, we had problems with fast CRC support in Percona Server build.

@mdcallag
Copy link
Contributor Author

mdcallag commented Jun 23, 2017

This line in RocksDB LOG explains whether fast CRC support might be used. It can say yes when support isn't used because of this bug.
Fast CRC32 supported: 1

Code is here:
https://github.com/facebook/rocksdb/blob/master/util/crc32c.cc

To get fast crc32 support you want -DHAVE_SSE42 and probably some of -msse, -msse4.2 on compiler command line. Which mean you might need USE_SSE=1 set as an environment variable when building MyRocks.

grep for SSE in https://github.com/facebook/rocksdb/blob/master/build_tools/build_detect_platform

@bdarnell
Copy link
Contributor

bdarnell commented Jul 4, 2017

This has been a headache for CockroachDB too (cockroachdb/cockroach#15589). Compiling with -msse4.2 means that our binary releases don't work on older CPUs. We would prefer to compile without that flag (so that the compiler generates code for any amd64 cpu), then use runtime feature detection to enable the SSE4.2 fast CRC.

@siying
Copy link
Contributor

siying commented Jul 5, 2017

@bdarnell you are welcome to send a PR to add a flag to achieve what you want.

benesch added a commit to benesch/rocksdb that referenced this issue Jul 8, 2017
With some compilers, `-std=c++11` is necessary for <cstdint> to be
available. Pass this flag via $PLATFORM_CXXFLAGS. Fixes facebook#2488.
@benesch
Copy link
Contributor

benesch commented Jul 8, 2017

@mdcallag can you verify that #2545 solves your second problem? I think #2513 has already fixed the first problem.

zhangjinpeng87 pushed a commit to tikv/rocksdb that referenced this issue Sep 18, 2017
Summary:
With some compilers, `-std=c++11` is necessary for <cstdint> to be
available. Pass this flag via $PLATFORM_CXXFLAGS. Fixes facebook#2488.
Closes facebook#2545

Differential Revision: D5620610

Pulled By: yiwu-arbug

fbshipit-source-id: 2f975b8c1ad52e283e677d9a33543abd064f13ce
zhangjinpeng87 pushed a commit to tikv/rocksdb that referenced this issue Sep 19, 2017
Summary:
With some compilers, `-std=c++11` is necessary for <cstdint> to be
available. Pass this flag via $PLATFORM_CXXFLAGS. Fixes facebook#2488.
Closes facebook#2545

Differential Revision: D5620610

Pulled By: yiwu-arbug

fbshipit-source-id: 2f975b8c1ad52e283e677d9a33543abd064f13ce
@socketpair
Copy link

possibly fixed by 55d58d9 (#10667)

in versions >= 7.9.2

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

No branches or pull requests

6 participants