CRC32 Power Optimization Changes #2353

kamasubb · 2017-05-23T16:15:23Z

Support for PowerPC Architecture
Detecting AltiVec Support

adamretter · 2017-05-23T16:17:55Z

util/crc32c_ppc.c

+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License


I am not sure of the answer to this, but I see that this is GPL licensed. Is introducing GPL code acceptable?

I can't answer it for sure either but can provide some additional info.
https://github.com/facebook/rocksdb/blob/master/COPYING

Contains the GPL v2 license.

This code originally came from
https://github.com/antonblanchard/crc32-vpmsum

Which allows license choice for either GPL or Apache 2.0. If necessary we could probably update these files to choose Apache 2.0 if that is preferred.

@siying Could you please comment on the license used for the new files in this PR?

I just spoke to @antonblanchard who indicated you can merge this under any open source license you choose.

@sagar0 is going to ask our lawyer for that. In the mean time, I think these files should go under third-party/.

@grooverdan what do you mean by "you can merge this under any open source license you choose". Do you mean you can change this header if needed according to Facebook's lawyer?

smatzek · 2017-05-23T18:54:20Z

@grooverdan You may want to take a look at this PR as well.

grooverdan · 2017-05-24T06:08:30Z

util/ppc.cc

+int arch_ppc_crc32 = 0;
+
+#if __linux__ && __powerpc64__
+#include <stdio.h>


stdio not used.

grooverdan · 2017-05-24T06:39:10Z

util/ppc.cc

+#include "util/ppc.h"
+
+/* flags we export */
+int arch_ppc_crc32 = 0;


could be static (or local to arch_ppc_probe as its not exposed).

grooverdan · 2017-05-24T06:42:49Z

util/crc32c_ppc.c

+#define VMX_ALIGN	16
+#define VMX_ALIGN_MASK	(VMX_ALIGN-1)
+
+#ifdef REFLECT


I think the code blocks marked by CRC_XOR and REFLECT aren't needed?

Those values (#define REFLECT and #define CRC_XOR) are defined in crc32c_ppc_constants.h and those code blocks are needed. Without them you'll get different output values.

grooverdan

minor changes only.

Apologies for the ones that go a bit off topic.

grooverdan · 2017-05-24T06:52:42Z

util/crc32c.cc

+  }
+}
+# endif
+
 static inline Function Choose_Extend() {


FB folks - what's your opinion on replacing this with an IFUNC?

ref: https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html#index-g_t_0040code_007bifunc_007d-attribute-2529 (binutils-2.20.1+ GNU C library (at least version 2.11.1).

Some old syntax may be needed to support older clang versions (added https://reviews.llvm.org/D15525)

never mind. saw Windows support.

grooverdan · 2017-05-24T07:01:30Z

util/crc32c_ppc.c

+{
+  unsigned char *buf2;
+
+  if (!data) {


I couldn't see a case where data is a null pointer. Perhaps crc32c::Extend can have an attribute nonnull to enforce this.

It isn't clear to me either whether rocksdb currently has any callers passing a NULL data pointer to the crc function or not. Even if there are none now, there could be some in the future. The question is what should the crc service we're providing do about it. The way this code is dropped now, we handle it gracefully but slowly (because of the malloc/free overhead).

In a separate, future pull request we were planning to add code that will take a very fast shortcut if the data pointer is NULL (ie handle it gracefully and quickly). The fastzero experiments we've done are shown in this github repo:
https://github.com/kestrels/crc32c-fastzero/blob/master/output-data/example-output.txt

The very first part of the page shows that to actually allocate a 1GB buffer and crc it takes 137613320 nsec but using the fastzero shortcut (-d) takes only 809 nsec.

grooverdan · 2017-05-24T07:13:14Z

util/crc32c.cc

  return isSSE42() ? ExtendImpl<Fast_CRC32> : ExtendImpl<Slow_CRC32>;
+#else
+  return isAltiVec() ? ExtendPPCImpl : ExtendImpl<Slow_CRC32>;
+#endif
 }

 bool IsFastCrc32Supported() {


Its use by DumpSupportInfo and potential for future enhancement this probably should return a const char * describing the implementation rather than a bool. Bit outside the scope of the PR however.

grooverdan · 2017-05-24T07:17:09Z

Makefile

+HAS_ALTIVEC=1
+endif
+
+ifeq (,$(shell $(CXX) -fsyntax-only -mcpu=power8 -xc /dev/null 2>&1))


looks rather tied to current architecture.

Can ifdef(__powerpc64__) be used instead of HAVE_POWER8?

This path is deliberately tied to current architecture, yes. We needed to disallow this code from running on POWER7 and earlier models because those older hardware models don't have the special vector instructions that are used here to compute the CRC in crc32c_ppc_asm.S. Those special vector instructions were introduced in POWER8.

So this happens both in the Makefile as we check the compiler capabilities here, and also the architecture probe code (arch_ppc_probe function) checks the hardware capability.

So that is why we couldn't use ifdef(powerpc64) in this case.

grooverdan · 2017-05-24T10:21:22Z

note #2330 refactors bits of the same code. util/ppc.{h,cc} could be moved into util/crc32c.cc too.

kestrels · 2017-05-25T03:50:07Z

This same optimized assembly crc algorithm was integrated into Ceph as well:
ceph/ceph#13909

We'd like to get the same performance improvement for the PowerPC architecture into rocksdb as well, since Ceph's Bluestore feature relies heavily on rocksdb.

facebook-github-bot · 2017-05-25T08:30:01Z

@kamasubb updated the pull request - view changes

facebook-github-bot · 2017-05-30T07:18:32Z

@kamasubb updated the pull request - view changes

facebook-github-bot · 2017-06-01T12:33:39Z

@kamasubb updated the pull request - view changes

kamasubb · 2017-06-01T17:07:08Z

@yiwu-arbug Could you please have a look at this? We integrated this code into
Ceph to improve the performance on PowerPC architecture, and would like to do the same
for rocksdb so that we get better performance out of the Ceph BlueStore feature which
relies heavily on Rocksdb

yiwu-arbug · 2017-06-01T17:19:25Z

I'm probably not the best person to review. cc @siying and current oncall @maysamyabandeh

siying · 2017-06-01T18:40:46Z

util/crc32c_ppc.c

+//  LICENSE file in the root directory of this source tree. An additional grant
+//  of patent rights can be found in the PATENTS file in the same directory.
+//  This source code is also licensed under the GPLv2 license found in the
+//  COPYING file in the root directory of this source tree.


Can we move all those files to under third-party/?

Or, if you want to put it here, is there a way to change it to the standard RocksDB header?

I think it would be nice to leave it here as this directory is where the Intel CRC32 code lives.

As for the header I think the intent was to make it be the standard header. It looks like the header on the other files with the exception of the copyright. This is following the example of OpenStack where larger LOC contributions will add copyrights to the top. For example this file is copyrighted by 5 different contributing entities: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py

I am still waiting to hear back from FB open-source legal team on the question of whether this kind of copyright header is acceptable.

FB open-source legal team approved the copyright headers in this PR, and we are good to go here as far as license/copyright is concerned.

maysamyabandeh · 2017-06-02T01:36:39Z

Makefile

@@ -545,14 +557,27 @@ $(SHARED2): $(SHARED4)
 $(SHARED3): $(SHARED4)
 	ln -fs $(SHARED4) $(SHARED3)
 endif
+shared_cc_objects = $(filter %.o,$(LIB_SOURCES:.cc=.cc.o))


Can we use capital letters to be consistent with the rest of Makefile?

maysamyabandeh · 2017-06-02T01:40:05Z

Makefile

-LIBOBJECTS = $(LIB_SOURCES:.cc=.o)
-LIBOBJECTS += $(TOOL_LIB_SOURCES:.cc=.o)
-MOCKOBJECTS = $(MOCK_LIB_SOURCES:.cc=.o)
+LIBOBJECTS = $(filter %.o,$(LIB_SOURCES:.cc=.cc.o) $(LIB_SOURCES_C:.c=.c.o) $(LIB_SOURCES_ASM:.S=.S.o))


Why is the we need filter here? Is there any corner case in LIB_SOURCES that does not end with ".cc"?

maysamyabandeh · 2017-06-02T01:49:01Z

Makefile

-java_libobjects = $(patsubst %,jl/%,$(LIBOBJECTS))
+java_cc_objects = $(filter %.o,$(LIB_SOURCES:.cc=.cc.o))
+java_c_objects = $(filter %.o,$(LIB_SOURCES_C:.c=.c.o))
+java_asm_objects = $(filter %.o,$(LIB_SOURCES_ASM:.S=.S.o))


Why not reusing shared_asm_objects variable?
Same for the two lines above.

maysamyabandeh · 2017-06-02T02:02:33Z

util/crc32c.cc

+#include "util/crc32c_ppc_constants.h"
+#include "util/crc32c_ppc.h"
+
+#if __linux__ && __powerpc64__


Is not it already inside ifdef powerpc64 clause?

maysamyabandeh · 2017-06-02T02:05:06Z

util/crc32c.cc

+    return false;
+  }
+}
+# endif


can you make sure that you have run "make format"?

facebook-github-bot · 2017-06-02T10:16:27Z

@kamasubb updated the pull request - view changes

facebook-github-bot · 2017-06-02T10:21:02Z

@kamasubb updated the pull request - view changes

kamasubb · 2017-06-05T19:32:43Z

@maysamyabandeh - Created a new branch in my local workspace with updated to master of rocksdb and updated with all comments addressed.
Created another pull request. #2409

facebook-github-bot · 2017-06-08T10:01:39Z

@kamasubb updated the pull request - view changes

maysamyabandeh · 2017-06-08T18:47:43Z

Makefile

-LIBOBJECTS = $(LIB_SOURCES:.cc=.o)
-LIBOBJECTS += $(TOOL_LIB_SOURCES:.cc=.o)
-MOCKOBJECTS = $(MOCK_LIB_SOURCES:.cc=.o)
+#LIBOBJECTS = $(filter %.o,$(LIB_SOURCES:.cc=.cc.o) $(LIB_SOURCES_C:.c=.c.o) $(LIB_SOURCES_ASM:.S=.S.o))


Remove the commented line.

facebook-github-bot · 2017-06-08T18:49:30Z

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

maysamyabandeh · 2017-06-08T18:50:21Z

LGTM. I let @siying land it after copyright issue is resolved.

maysamyabandeh · 2017-06-08T23:36:44Z

util/crc32c_ppc_constants.h

+//  of patent rights can be found in the PATENTS file in the same directory.
+//  This source code is also licensed under the GPLv2 license found in the
+//  COPYING file in the root directory of this source tree.
+#define CRC 0x1edc6f41


It is missing an include guard.

facebook-github-bot · 2017-06-27T06:28:08Z

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

kamasubb · 2017-06-28T05:32:21Z

Travis build failed due to lack of space on the file system.
CI build need to be re-triggered once the space issue is resolved.

sagar0 · 2017-06-28T22:25:35Z

@kamasubb Re-kicked the test that failed due to lack of space, and it passed this time.

Support for PowerPC Architecture Detecting AltiVec Support

facebook-github-bot · 2017-06-29T17:10:06Z

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

kamasubb · 2017-06-30T08:58:39Z

@sagar0 Travis build failed due to lack of space on the file system.
Kindly re-trigger once the space issue is resolved.

kamasubb · 2017-07-10T11:15:42Z

Travis build failed due to lack of space on the file system.
Kindly re-trigger once the space issue is resolved.

sagar0 · 2017-07-10T18:47:08Z

Re-triggered the test, and it succeeded this time.
(Need to investigate why the tests are frequently running into out-of-space issue).

smatzek · 2017-07-26T13:10:51Z

@siying It looks like the copyright issue is resolved, all checks passed and maysamyabandeh said it LTGM. Can you provide an outlook on when this may merge or anything else that needs to be done on @kamasubb 's side?

siying · 2017-07-26T22:21:46Z

Makefile

-LIBOBJECTS = $(LIB_SOURCES:.cc=.o)
-LIBOBJECTS += $(TOOL_LIB_SOURCES:.cc=.o)
-MOCKOBJECTS = $(MOCK_LIB_SOURCES:.cc=.o)
+LIBOBJECTS = $(LIB_SOURCES:.cc=.cc.o) $(LIB_SOURCES_C:.c=.c.o) $(LIB_SOURCES_ASM:.S=.S.o)


Can you explain the ration behind changing from ".o" to ".cc.o"?
I saw in the build script both were built, so it's likely we have missed something.

siying · 2017-07-27T01:44:23Z

Sorry but I need to revert it first. Travis build fails after this: https://travis-ci.org/facebook/rocksdb/jobs/257805952

It's likely to be related to the change in Makefile with the .o -> .cc.o change. Please resummit a pull request with this problem resolved after I revert it. Sorry about that.

siying · 2017-07-27T01:47:55Z

I think it's a better idea to keep the ".o" convention. If there is a file name conflict for a .c and a .cc file, it's OK to rename the RocksDB files.

kamasubb · 2017-07-28T08:50:08Z

@siying We have three file extensions .cc, .c and .S
The Altivector related header files are included only when the source code is C.
The implementation of CRC is achieved using assembly instructions. This implementation has file extension .S which indicates the compiler that the source is assembly instruction.

kamasubb · 2017-07-28T11:26:04Z

@siying We need to differentiate these file extensions. Makefile right now does not differentiate it as LIBSOURCES has only files with .cc extension. LIBOBJECTS will have object list of .cc files with extension .o. If we rename the .S file to .cc, compiler will not recognize the assembly instructions and report error.
Below is the weblink that lists the mapping of file type and associated extension
https://gcc.gnu.org/onlinedocs/gcc-3.3/gcc/Overall-Options.html

siying · 2017-07-28T16:26:26Z

@kamasubb is there a way to make sure that all the file names are different for .cc, .c, and .S? Just rename one of them if they are the same name. Another thing to consider is to roll forward and make everything consistent in this Makefile to avoid we build the same file twice, one to ".o" and one to ".cc.o" (which was what was happening). I tried to fix it here: #2651 . It became better but there may be bugs there. It's something can be considered too if renaming is not possible because of some reason.

kamasubb · 2017-08-07T16:33:34Z

@siying, @sagar0 I have been trying to re-create the build failure on the PowerPC system and haven't been successful. The CRC32 patch build did not fail.
I tried re-creating the build failure on x86_64 Ubuntu trusty. The build succeeded. Below is the command that I am running to build
OPT=-DTRAVIS V=1 make -j4 all; OPT=-DTRAVIS V=1 ROCKSDBTESTS_END=db_block_cache_test make -j4 check_some;
I am using the distro provided build tools.

Can you share details regarding the build environment and build tools of Travis?

siying · 2017-08-07T16:55:19Z

@kamasubb what I saw previously was that some of the .cc files were compiled twice, for example, one for threadpool_imp.cc.o and one for threadpool_impl.cc.o. This didn't cause problem on my previous environment, but seem to have some problem with ccache in Travis (just a speculation of the reason of ccache failure). It's anyway not efficient to have the same .cc file to be compiled twice. If you are able to fix this double compiling, we are good to go and we can merge it. Sorry for the inconvenience of the reverting.

kamasubb · 2017-08-08T13:02:55Z

@siying Thank you for pointing out ccache. I am able to re-create the issue after I installed ccache.
The duplicate compilation is due to code bug in DEPFILES, DEPFILES_C, DEPFILES_ASM target.
I shall open a new pull request once I have the fix tested

…acebook#2353 is not compatible with Clang 11. The code relies on a GCC header file ppc-asm.h. To solve this, I have taken the same approach as the the upstream project from which the CRC code came antonblanchard/crc32-vpmsum@ffc8018#diff-ec3e62c56fbcddeb07230f2a4673c1abd7f0f1cc8e48a2aa560056cfc1b25d60 and imported a copy of the GCC header file into our code-base which will be used when Clang is the compiler. NOTE: This may have licensing implications which need to be approved by FB before this is merged.

Summary: Closes #7691 The optimised CRC code for PPC64le which was originally imported in #2353 is not compatible with Clang 11. It looks like the code most likely originated from https://github.com/antonblanchard/crc32-vpmsum. The code relied on a GCC header file `ppc-asm.h` which is not available in Clang. To solve this, I have taken the same approach as the the upstream project from which the CRC code came antonblanchard/crc32-vpmsum@ffc8018#diff-ec3e62c56fbcddeb07230f2a4673c1abd7f0f1cc8e48a2aa560056cfc1b25d60 and simply imported a copy of the GCC header file into our code-base which will be used when Clang is the compiler on pcc64le. **NOTE**: The new file `util/ppc-asm.h` may have licensing implications which I guess need to be approved by RocksDB/Facebook before this is merged Pull Request resolved: #7713 Reviewed By: jay-zhuang Differential Revision: D25222645 Pulled By: pdillinger fbshipit-source-id: e3fec9136f26ce1eb7a027048bcf77a6cb3c769c

Summary: Closes facebook#7691 The optimised CRC code for PPC64le which was originally imported in facebook#2353 is not compatible with Clang 11. It looks like the code most likely originated from https://github.com/antonblanchard/crc32-vpmsum. The code relied on a GCC header file `ppc-asm.h` which is not available in Clang. To solve this, I have taken the same approach as the the upstream project from which the CRC code came antonblanchard/crc32-vpmsum@ffc8018#diff-ec3e62c56fbcddeb07230f2a4673c1abd7f0f1cc8e48a2aa560056cfc1b25d60 and simply imported a copy of the GCC header file into our code-base which will be used when Clang is the compiler on pcc64le. **NOTE**: The new file `util/ppc-asm.h` may have licensing implications which I guess need to be approved by RocksDB/Facebook before this is merged Pull Request resolved: facebook#7713 Reviewed By: jay-zhuang Differential Revision: D25222645 Pulled By: pdillinger fbshipit-source-id: e3fec9136f26ce1eb7a027048bcf77a6cb3c769c

facebook-github-bot added the CLA Signed label May 23, 2017

adamretter reviewed May 23, 2017

View reviewed changes

grooverdan reviewed May 24, 2017

View reviewed changes

util/ppc.cc Outdated

int arch_ppc_crc32 = 0;

#if __linux__ && __powerpc64__

#include <stdio.h>

Copy link

Contributor

grooverdan May 24, 2017

Choose a reason for hiding this comment

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

stdio not used.

grooverdan reviewed May 24, 2017

View reviewed changes

kamasubb force-pushed the wip_crc_opt_2 branch from 93c7226 to 5e13eeb Compare May 25, 2017 08:29

kamasubb force-pushed the wip_crc_opt_2 branch from 5e13eeb to 32eb688 Compare May 30, 2017 07:18

kamasubb force-pushed the wip_crc_opt_2 branch from 32eb688 to 514f889 Compare June 1, 2017 12:33

siying reviewed Jun 1, 2017

View reviewed changes

maysamyabandeh reviewed Jun 2, 2017

View reviewed changes

kamasubb force-pushed the wip_crc_opt_2 branch from 514f889 to 7717148 Compare June 2, 2017 10:16

kamasubb mentioned this pull request Jun 5, 2017

CRC32 Power Optimization Changes #2409

Closed

kamasubb force-pushed the wip_crc_opt_2 branch from 1adc812 to 52d24c9 Compare June 8, 2017 10:01

maysamyabandeh reviewed Jun 8, 2017

View reviewed changes

kamasubb force-pushed the wip_crc_opt_2 branch from 353deb3 to efb1061 Compare June 27, 2017 06:28

CRC32 Power Optimization Changes

fa5a6c6

Support for PowerPC Architecture Detecting AltiVec Support

kamasubb force-pushed the wip_crc_opt_2 branch from efb1061 to fa5a6c6 Compare June 29, 2017 17:10

facebook-github-bot closed this in 2289d38 Jul 26, 2017

siying reviewed Jul 26, 2017

View reviewed changes

siying mentioned this pull request Jul 27, 2017

Unify all target names to .cc.o #2651

Closed

kamasubb mentioned this pull request Aug 10, 2017

Updated CRC32 Power Optimization Changes #2716

Closed

kestrels mentioned this pull request Oct 17, 2017

CRC32 PPC Fast Zero Optimization #2966

Closed

adamretter mentioned this pull request Nov 25, 2020

Fix Compilation on ppc64le using Clang 11 #7713

Closed

CRC32 Power Optimization Changes #2353

CRC32 Power Optimization Changes #2353

Conversation

kamasubb commented May 23, 2017

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

smatzek commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grooverdan May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

grooverdan May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grooverdan commented May 24, 2017

kestrels commented May 25, 2017

facebook-github-bot commented May 25, 2017

facebook-github-bot commented May 30, 2017

facebook-github-bot commented Jun 1, 2017

kamasubb commented Jun 1, 2017

yiwu-arbug commented Jun 1, 2017

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

facebook-github-bot commented Jun 2, 2017

facebook-github-bot commented Jun 2, 2017

kamasubb commented Jun 5, 2017

facebook-github-bot commented Jun 8, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2017

maysamyabandeh commented Jun 8, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Jun 27, 2017

kamasubb commented Jun 28, 2017

sagar0 commented Jun 28, 2017

facebook-github-bot commented Jun 29, 2017

kamasubb commented Jun 30, 2017

kamasubb commented Jul 10, 2017

sagar0 commented Jul 10, 2017

smatzek commented Jul 26, 2017

Choose a reason for hiding this comment

siying commented Jul 27, 2017

siying commented Jul 27, 2017

kamasubb commented Jul 28, 2017

kamasubb commented Jul 28, 2017

siying commented Jul 28, 2017

kamasubb commented Aug 7, 2017

siying commented Aug 7, 2017

kamasubb commented Aug 8, 2017

grooverdan May 24, 2017 •

edited

Loading

grooverdan May 24, 2017 •

edited

Loading