Updated CRC32 Power Optimization Changes #2716

kamasubb · 2017-08-10T04:55:14Z

Support for PowerPC Architecture
Detecting AltiVec Support

kamasubb · 2017-08-10T05:48:13Z

This pull request is in continuation to the work initiated in pull request #2353

kamasubb · 2017-08-10T05:48:54Z

@siying Kindly review the changes

facebook-github-bot · 2017-08-10T21:54:27Z

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2017-08-10T21:57:02Z

@kamasubb can you figure out in Travis why the same .cc file is still built twice?

g++ -DROCKSDB_USE_RTTI -g -W -Wextra -Wall -Wsign-compare -Wshadow -Wno-unused-parameter -Werror -I. -I./include -std=c++11 -DROCKSDB_PLATFORM_POSIX -DROCKSDB_LIB_IO_POSIX -DOS_LINUX -fno-builtin-memcmp -DROCKSDB_FALLOCATE_PRESENT -DSNAPPY -DGFLAGS=google -DZLIB -DBZIP2 -DROCKSDB_MALLOC_USABLE_SIZE -DROCKSDB_PTHREAD_ADAPTIVE_MUTEX -DROCKSDB_BACKTRACE -DROCKSDB_RANGESYNC_PRESENT -DROCKSDB_SCHED_GETCPU_PRESENT -march=native -DROCKSDB_SUPPORT_THREAD_LOCAL -isystem ./third-party/gtest-1.7.0/fused-src -DTRAVIS -O2 -fno-omit-frame-pointer -momit-leaf-frame-pointer -Woverloaded-virtual -Wnon-virtual-dtor -Wno-missing-field-initializers -c util/threadpool_imp.cc -o util/threadpool_imp.cc.o
...
g++ -DROCKSDB_USE_RTTI -g -W -Wextra -Wall -Wsign-compare -Wshadow -Wno-unused-parameter -Werror -I. -I./include -std=c++11 -DROCKSDB_PLATFORM_POSIX -DROCKSDB_LIB_IO_POSIX -DOS_LINUX -fno-builtin-memcmp -DROCKSDB_FALLOCATE_PRESENT -DSNAPPY -DGFLAGS=google -DZLIB -DBZIP2 -DROCKSDB_MALLOC_USABLE_SIZE -DROCKSDB_PTHREAD_ADAPTIVE_MUTEX -DROCKSDB_BACKTRACE -DROCKSDB_RANGESYNC_PRESENT -DROCKSDB_SCHED_GETCPU_PRESENT -march=native -DROCKSDB_SUPPORT_THREAD_LOCAL -isystem ./third-party/gtest-1.7.0/fused-src -DTRAVIS -O2 -fno-omit-frame-pointer -momit-leaf-frame-pointer -Woverloaded-virtual -Wnon-virtual-dtor -Wno-missing-field-initializers -c -o util/threadpool_imp.o util/threadpool_imp.cc

grooverdan

I recommend taking the C implementation from antonblanchard/crc32-vpmsum#4 to avoid the complexities of adding asm to the implementation.

The POWER8 references will look dated once P9 and future processors come to market.

grooverdan · 2017-08-10T22:50:23Z

db/db_test_util.cc

  rocksdb::SyncPoint::GetInstance()->ClearCallBack(
      "NewRandomAccessFile:O_DIRECT");
-  rocksdb::SyncPoint::GetInstance()->ClearCallBack(
-      "NewWritableFile:O_DIRECT");
+  rocksdb::SyncPoint::GetInstance()->ClearCallBack("NewWritableFile:O_DIRECT");


Don't add pointless whitespace changes.

In the previous pull request I was asked to run "make format"
Below is the output from make format for your reference. I ran make format on my changes and it is adding these white space changes.

build_tools/format-diff.sh
Detect lines that doesn't follow the format rules:
--- db/db_test_util.cc (before formatting)
+++ db/db_test_util.cc (after formatting)
@@ -292,8 +292,7 @@
!defined(OS_AIX)
rocksdb::SyncPoint::GetInstance()->ClearCallBack(
"NewRandomAccessFile:O_DIRECT");

rocksdb::SyncPoint::GetInstance()->ClearCallBack(

"NewWritableFile:O_DIRECT");

rocksdb::SyncPoint::GetInstance()->ClearCallBack("NewWritableFile:O_DIRECT");
#endif

bool can_allow_mmap = IsMemoryMappedAccessSupported();
--- tools/db_stress.cc (before formatting)
+++ tools/db_stress.cc (after formatting)
@@ -2370,15 +2370,15 @@
#if !defined(NDEBUG) && !defined(OS_MACOSX) && !defined(OS_WIN) &&
!defined(OS_SOLARIS) && !defined(OS_AIX)
rocksdb::SyncPoint::GetInstance()->SetCallBack(

"NewWritableFile:O_DIRECT", [&](void* arg) {

int* val = static_cast<int*>(arg);

*val &= ~O_DIRECT;

});

"NewWritableFile:O_DIRECT", [&](void* arg) {

int* val = static_cast<int*>(arg);

*val &= ~O_DIRECT;

});
rocksdb::SyncPoint::GetInstance()->SetCallBack(

"NewRandomAccessFile:O_DIRECT", [&](void* arg) {

int* val = static_cast<int*>(arg);

*val &= ~O_DIRECT;

});

"NewRandomAccessFile:O_DIRECT", [&](void* arg) {

int* val = static_cast<int*>(arg);

*val &= ~O_DIRECT;

});
rocksdb::SyncPoint::GetInstance()->EnableProcessing();
#endif

Does this mean that either the code that got merged did was not run with the "make format" or there is a code bug the "make format"

grooverdan · 2017-08-10T22:50:42Z

tools/db_stress.cc

-    });
+      "NewWritableFile:O_DIRECT", [&](void* arg) {
+        int* val = static_cast<int*>(arg);
+        *val &= ~O_DIRECT;


Don't add pointless whitespace changes.

See my response above for "db/db_test_util.cc"

grooverdan · 2017-08-10T22:52:06Z

util/crc32c.cc

@@ -371,6 +394,7 @@ uint32_t ExtendImpl(uint32_t crc, const char* buf, size_t size) {
 }

 // Detect if SS42 or not.
+#ifndef HAVE_POWER8


There's no point wrapping this. There already is a HAVE_SSE42 inside the function.

This wrapping ensures that the function isSSE42() is not defined on Power platform.
If we don't add this, then the isSSE42() is defined which does not have relevance.

grooverdan · 2017-08-10T22:53:30Z

util/crc32c.cc

+}
+#endif  // __linux__
+
+static bool isAltiVec() {


return arch_ppc_probe(); would of been simpler and if its that simple why is this wrapped in a different function?

grooverdan · 2017-08-10T22:56:42Z

util/crc32c.cc

 }

 bool IsFastCrc32Supported() {
-  return isSSE42();


DumpSupportInfo is the only place this is used. I suggest that maybe a string describing the implementation is now a better choice for its output now that there are a variety of implementations.

I like the idea.

grooverdan · 2017-08-10T23:13:23Z

util/crc32c.cc

+#define PPC_FEATURE2_VEC_CRYPTO 0x02000000
+#endif
+
+#ifndef AT_HWCAP2


This is always defined on linux. - can be removed.

See my above comment on freebsd.

grooverdan · 2017-08-10T23:15:04Z

util/crc32c.cc


 typedef uint32_t (*Function)(uint32_t, const char*, size_t);

+#if defined(HAVE_POWER8) && defined(HAS_ALTIVEC)
+uint32_t ExtendPPCImpl(uint32_t crc, const char *buf, size_t size) {
+  return crc32c_ppc(crc, (const unsigned char *)buf, size);


not sure what the point of function wrapping here is.

grooverdan · 2017-08-10T23:15:51Z

util/crc32c.cc

+#include <sys/auxv.h>
+
+#ifndef PPC_FEATURE2_VEC_CRYPTO
+#define PPC_FEATURE2_VEC_CRYPTO 0x02000000


This is always defined on powerpc64 linux.

These symbols are present in the header files just above. During an earlier round of review Daniel Axtens had me add these as a backup in case the headers are not found. For example, on powerpc64 freebsd (which doesn't have all the same headers as linux) we hit compilation errors in ceph and had to fix things like this to be more careful.

kamasubb · 2017-08-14T17:09:25Z

@siying
The double build is because of
BENCHTOOLOBJECTS = $(BENCH_LIB_SOURCES:.cc=.o) $(LIBOBJECTS) $(TESTUTIL)
EXPOBJECTS = $(EXP_LIB_SOURCES:.cc=.o) $(LIBOBJECTS) $(TESTUTIL)
$(TOOLS_LIBRARY): $(BENCH_LIB_SOURCES:.cc=.o) $(TOOL_LIB_SOURCES:.cc=.o) $(LIB_SOURCES:.cc=.o) $(TESTUTIL)

The fix to it is
BENCHTOOLOBJECTS = $(BENCH_LIB_SOURCES:.cc=.cc.o) $(LIBOBJECTS) $(TESTUTIL)
EXPOBJECTS = $(EXP_LIB_SOURCES:.cc=.cc.o) $(LIBOBJECTS) $(TESTUTIL)
$(TOOLS_LIBRARY): $(BENCH_LIB_SOURCES:.cc=.cc.o) $(TOOL_LIB_SOURCES:.cc=.cc.o) $(LIB_SOURCES:.cc=.cc.o) $(TESTUTIL)

siying · 2017-08-17T00:28:03Z

@kamasubb the recent Travis build still has it built twice.

kamasubb · 2017-08-17T17:01:28Z

@siying I am working on an approach that would reduce the changes required to Makefile.
The idea is to have explicit rules defined for crc32c_ppc.c and crc32c_ppc_asm.S.
These rules will be applied only for PowerPC architecture while the non-PowerPC architecture will remain as is. These explicit rules will be defined for shared_lib, static_lib and rocksdb_java.
Updated patch will be submitted for review in couple of days.

facebook-github-bot · 2017-08-18T13:46:39Z

@kamasubb updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-08-18T14:06:19Z

@kamasubb updated the pull request - view changes - changes since last import

kamasubb · 2017-08-18T14:27:24Z

@sagar0 AppVeyor build failed due to heap issue. Kindly resolve the resource/memory issue
Below is snippet from log indicating the error for your reference
c:\projects\rocksdb\utilities\transactions\pessimistic_transaction_db.h(243): fatal error C1060: compiler is out of heap space [C:\projects\rocksdb\build\rocksdb-shared.vcxproj]

kamasubb · 2017-08-22T09:50:09Z

@sagar0 If the AppVeyor build fail issue has been resolved, kindly re-run the AppVeyor build

sagar0 · 2017-08-22T19:09:54Z

@kamasubb Our Appveyor builds have been failing continuously for a few days now, and we haven't gotten to fixing it yet.

kestrels · 2017-08-22T20:35:16Z

@siying When you get a chance, please let us know if you agree this approach is better. This should hopefully be easier to maintain and should no longer have any problems with ccache or objects being built multiple times. The Makefile changes where the object files were being named .cc.o, .c.o, and .S.o are gone. Now they should all be named .o.

siying · 2017-08-22T21:29:57Z

@kestrels yes, I think not having two files with the same name but different extension .cc and .c sounds a good idea anyway.

Support for PowerPC Architecture Detecting AltiVec Support

facebook-github-bot · 2017-08-29T10:23:17Z

@kamasubb updated the pull request - view changes - changes since last import

kamasubb · 2017-08-30T06:03:10Z

@siying When you get a chance, please review the new patch

facebook-github-bot · 2017-08-31T20:46:46Z

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Java build on PPC64le has been broken since a few months, due to #2716. Fixing it with the least amount of changes. (We should cleanup a little around this code when time permits). This should fix the build failures seen in http://140.211.168.68:8080/job/Rocksdb/ . Closes #3359 Differential Revision: D6712938 Pulled By: sagar0 fbshipit-source-id: 3046e8f072180693de2af4762934ec1ace309ca4

Summary: Java build on PPC64le has been broken since a few months, due to facebook#2716. Fixing it with the least amount of changes. (We should cleanup a little around this code when time permits). This should fix the build failures seen in http://140.211.168.68:8080/job/Rocksdb/ . Closes facebook#3359 Differential Revision: D6712938 Pulled By: sagar0 fbshipit-source-id: 3046e8f072180693de2af4762934ec1ace309ca4

facebook-github-bot added the CLA Signed label Aug 10, 2017

grooverdan suggested changes Aug 10, 2017

View reviewed changes

kamasubb force-pushed the wip_crc_opt_2 branch from 38bca31 to 95da7e1 Compare August 18, 2017 13:46

kamasubb force-pushed the wip_crc_opt_2 branch from 95da7e1 to de69ecf Compare August 18, 2017 14:06

CRC32 Power Optimization Changes

8310620

Support for PowerPC Architecture Detecting AltiVec Support

kamasubb force-pushed the wip_crc_opt_2 branch from de69ecf to 8310620 Compare August 29, 2017 10:23

facebook-github-bot closed this in e612e31 Aug 31, 2017

kestrels mentioned this pull request Oct 17, 2017

CRC32 PPC Fast Zero Optimization #2966

Closed

sagar0 mentioned this pull request Jan 12, 2018

Fix PowerPC dynamic java build #3359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated CRC32 Power Optimization Changes #2716

Updated CRC32 Power Optimization Changes #2716

kamasubb commented Aug 10, 2017

kamasubb commented Aug 10, 2017

kamasubb commented Aug 10, 2017

facebook-github-bot commented Aug 10, 2017

siying commented Aug 10, 2017

grooverdan left a comment

grooverdan Aug 10, 2017

kamasubb Aug 11, 2017

grooverdan Aug 10, 2017

kamasubb Aug 14, 2017

grooverdan Aug 10, 2017

kamasubb Aug 18, 2017

grooverdan Aug 10, 2017

grooverdan Aug 10, 2017

siying Aug 17, 2017

grooverdan Aug 10, 2017

kestrels Aug 11, 2017

grooverdan Aug 10, 2017

grooverdan Aug 10, 2017

kestrels Aug 11, 2017

kamasubb commented Aug 14, 2017

siying commented Aug 17, 2017

kamasubb commented Aug 17, 2017

facebook-github-bot commented Aug 18, 2017

facebook-github-bot commented Aug 18, 2017

kamasubb commented Aug 18, 2017

kamasubb commented Aug 22, 2017

sagar0 commented Aug 22, 2017

kestrels commented Aug 22, 2017

siying commented Aug 22, 2017

facebook-github-bot commented Aug 29, 2017

kamasubb commented Aug 30, 2017

facebook-github-bot commented Aug 31, 2017

Updated CRC32 Power Optimization Changes #2716

Updated CRC32 Power Optimization Changes #2716

Conversation

kamasubb commented Aug 10, 2017

kamasubb commented Aug 10, 2017

kamasubb commented Aug 10, 2017

facebook-github-bot commented Aug 10, 2017

siying commented Aug 10, 2017

grooverdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamasubb commented Aug 14, 2017

siying commented Aug 17, 2017

kamasubb commented Aug 17, 2017

facebook-github-bot commented Aug 18, 2017

facebook-github-bot commented Aug 18, 2017

kamasubb commented Aug 18, 2017

kamasubb commented Aug 22, 2017

sagar0 commented Aug 22, 2017

kestrels commented Aug 22, 2017

siying commented Aug 22, 2017

facebook-github-bot commented Aug 29, 2017

kamasubb commented Aug 30, 2017

facebook-github-bot commented Aug 31, 2017