From 19cd4ffed3d69781b2f922fb5faf3fe669d5c9ca Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Wed, 12 Feb 2020 11:43:33 -0800 Subject: [PATCH] xxhash can be inlined even if it was previously included Inlining hash functions is generally beneficial for performance. It becomes extremely beneficial whenever input size is a compile-time constant. To inline xxhash functions, one just needs to include it this way : One potential issue is that "xxhash.h" may have already been included previously, typically as part of another included `*.h` . In which case, the second `#include` statement will have no effect. This patch fixes this situation : now, when XXH_INLINE_ALL is defined, all identifiers are renamed, in order to avoid name collision and confusion with already included indentifiers. The renaming process uses XXH_NAMESPACE. XXH_NAMESPACE must be available (i.e. not already used) for renaming to work. A test has been added in `tests/` to ensure this scenario works correctly. --- Makefile | 6 +++- tests/Makefile | 45 ++++++++++++++++++++++++++ tests/multiInclude.c | 58 ++++++++++++++++++++++++++++++++++ xxh3.h | 2 +- xxhash.c | 2 +- xxhash.h | 75 ++++++++++++++++++++++++++++++++++---------- 6 files changed, 169 insertions(+), 19 deletions(-) create mode 100644 tests/Makefile create mode 100644 tests/multiInclude.c diff --git a/Makefile b/Makefile index bc518bf..779ae11 100644 --- a/Makefile +++ b/Makefile @@ -282,9 +282,13 @@ preview-man: man test: DEBUGFLAGS += -DDEBUGLEVEL=1 test: all namespaceTest check test-xxhsum-c c90test test-tools +.PHONY: test-inline +test-inline: + $(MAKE) -C tests test + .PHONY: test-all test-all: CFLAGS += -Werror -test-all: test test32 clangtest cxxtest usan listL120 trailingWhitespace staticAnalyze +test-all: test test32 clangtest cxxtest usan test-inline listL120 trailingWhitespace staticAnalyze .PHONY: test-tools test-tools: diff --git a/tests/Makefile b/tests/Makefile new file mode 100644 index 0000000..2e4d7de --- /dev/null +++ b/tests/Makefile @@ -0,0 +1,45 @@ +CFLAGS += -Wall -Wextra -g + +NM = nm +GREP = grep + +.PHONY: default +default: all + +.PHONY: all +all: test + +.PHONY: test +test: test_multiinclude + +.PHONY: test_multiinclude +test_multiinclude: + @$(MAKE) clean + # compile without xxhash.o, ensure symbols exist within target + # note : built using only default rules + $(MAKE) multiInclude + @$(MAKE) clean + # compile with xxhash.o, to detect duplicated symbols + $(MAKE) multiInclude_withxxhash + @$(MAKE) clean + # Note : XXH_INLINE_ALL with XXH_NAMESPACE is currently disabled + # compile with XXH_NAMESPACE + # CPPFLAGS=-DXXH_NAMESPACE=TESTN_ $(MAKE) multiInclude_withxxhash + # no symbol prefixed TESTN_ should exist + # ! $(NM) multiInclude_withxxhash | $(GREP) TESTN_ + #$(MAKE) clean + # compile with XXH_NAMESPACE and without xxhash.o + # CPPFLAGS=-DXXH_NAMESPACE=TESTN_ $(MAKE) multiInclude + # no symbol prefixed TESTN_ should exist + # ! $(NM) multiInclude | $(GREP) TESTN_ + #@$(MAKE) clean + +xxhash.o: ../xxhash.c ../xxhash.h + $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) -c -o $@ $< + +multiInclude_withxxhash: multiInclude.o xxhash.o + $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) -o $@ $^ + +clean: + @$(RM) *.o + @$(RM) multiInclude multiInclude_withxxhash diff --git a/tests/multiInclude.c b/tests/multiInclude.c new file mode 100644 index 0000000..7a81305 --- /dev/null +++ b/tests/multiInclude.c @@ -0,0 +1,58 @@ +/* +* test_multiinclude +* test program, just to validate including multiple times in any order +* +* Copyright (C) Yann Collet 2013-present +* +* GPL v2 License +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, write to the Free Software Foundation, Inc., +* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +* +* You can contact the author at : +* - xxHash homepage : http://www.xxhash.com +* - xxHash source repository : https://github.com/Cyan4973/xxHash +*/ + +#include // printf + +/* normal include, gives access to public symbols */ +#include "../xxhash.h" + +/* advanced include, gives access to experimental symbols */ +/* note : without specific effort, experimental symbols might be unavailable */ +#define XXH_STATIC_LINKING_ONLY +#include "../xxhash.h" + +/* inlining : re-define all symbols, keep them private to the unit. + * note : without specific efforts, symbol names will collide + * note 2 : no linking to xxhash.o, so that link stage will fail if inline is ignored */ +#define XXH_INLINE_ALL +#include "../xxhash.h" + + +int main(void) +{ + XXH3_state_t state; /* part of experimental */ + + XXH3_64bits_reset(&state); + const char input[] = "Hello World !"; + + XXH3_64bits_update(&state, input, sizeof(input)); + + XXH64_hash_t const h = XXH3_64bits_digest(&state); + printf("hash '%s' : %0llx \n", input, (unsigned long long)h); + + return 0; +} diff --git a/xxh3.h b/xxh3.h index 8961d71..1f0592c 100644 --- a/xxh3.h +++ b/xxh3.h @@ -1654,7 +1654,7 @@ XXH_PUBLIC_API XXH128_hash_t XXH3_128bits_digest (const XXH3_state_t* state) /* 128-bit utility functions */ -#include /* memcmp */ +#include /* memcmp, memcpy */ /* return : 1 is equal, 0 if different */ XXH_PUBLIC_API int XXH128_isEqual(XXH128_hash_t h1, XXH128_hash_t h2) diff --git a/xxhash.c b/xxhash.c index a17d836..80631eb 100644 --- a/xxhash.c +++ b/xxhash.c @@ -33,7 +33,7 @@ */ -/* xxhash.c only instantiates functions defined in xxhash.h +/* xxhash.c instantiates functions defined in xxhash.h */ #define XXH_STATIC_LINKING_ONLY /* access advanced declarations */ diff --git a/xxhash.h b/xxhash.h index a2323c3..988ee33 100644 --- a/xxhash.h +++ b/xxhash.h @@ -73,29 +73,29 @@ XXH32 6.8 GB/s 6.0 GB/s extern "C" { #endif - -#ifndef XXHASH_H_5627135585666179 -#define XXHASH_H_5627135585666179 1 - /* **************************** * API modifier ******************************/ /** XXH_INLINE_ALL (and XXH_PRIVATE_API) - * This build macro includes xxhash functions in `static` mode - * in order to inline them, and remove their symbol from the public list. - * Inlining offers great performance improvement on small keys, + * Implement requested xxhash functions directly in the unit. + * Inlining offers great performance improvement on small inputs, * and dramatic ones when length is expressed as a compile-time constant. * See https://fastcompression.blogspot.com/2018/03/xxhash-for-small-keys-impressive-power.html . + * It also removes all symbols from the public list. * Methodology : * #define XXH_INLINE_ALL * #include "xxhash.h" - * `xxhash.c` is automatically included. - * It's not useful to compile and link it as a separate object. + * Do not compile and link xxhash.o as a separate object (not useful) */ -#if defined(XXH_INLINE_ALL) || defined(XXH_PRIVATE_API) -# ifndef XXH_STATIC_LINKING_ONLY -# define XXH_STATIC_LINKING_ONLY -# endif +#if (defined(XXH_INLINE_ALL) || defined(XXH_PRIVATE_API)) \ + && !defined(XXH_INLINE_ALL_31684351384) + /* this section should be traversed only once */ +# define XXH_INLINE_ALL_31684351384 + /* give access to advanced API, required to compile implementations */ +# undef XXH_STATIC_LINKING_ONLY /* avoid macro redef */ +# define XXH_STATIC_LINKING_ONLY + /* make functions private */ +# undef XXH_PUBLIC_API # if defined(__GNUC__) # define XXH_PUBLIC_API static __inline __attribute__((unused)) # elif defined (__cplusplus) || (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) /* C99 */) @@ -103,10 +103,48 @@ extern "C" { # elif defined(_MSC_VER) # define XXH_PUBLIC_API static __inline # else - /* this version may generate warnings for unused static functions */ + /* note : this version may generate warnings for unused static functions */ # define XXH_PUBLIC_API static # endif -#else + + /* prefix all names, to avoid symbol duplicates with potential library */ +# ifdef XXH_NAMESPACE +# error "XXH_INLINE_ALL with XXH_NAMESPACE is not supported" +# /* Note : Alternative is to #undef all symbols (it's a pretty large list). + * If doing nothing : it compiles, but functions are actually Not inlined. + * */ +# endif +# define XXH_NAMESPACE XXH_INLINE_ + /* some identifiers are not symbols, + * they must nonetheless be renamed to avoid double declaration + * Alternative : do not redeclare them, + * which requires some #ifdef, and is more dispersed in the file + * while renaming can be achieved in a single place */ +# define XXH_IPREF(Id) XXH_INLINE_ ## Id +# define XXH_OK XXH_IPREF(XXH_OK) +# define XXH_ERROR XXH_IPREF(XXH_ERROR) +# define XXH_errorcode XXH_IPREF(XXH_errorcode) +# define XXH32_canonical_t XXH_IPREF(XXH32_canonical_t) +# define XXH64_canonical_t XXH_IPREF(XXH64_canonical_t) +# define XXH128_canonical_t XXH_IPREF(XXH128_canonical_t) +# define XXH32_state_s XXH_IPREF(XXH32_state_s) +# define XXH32_state_t XXH_IPREF(XXH32_state_t) +# define XXH64_state_s XXH_IPREF(XXH64_state_s) +# define XXH64_state_t XXH_IPREF(XXH64_state_t) +# define XXH3_state_s XXH_IPREF(XXH3_state_s) +# define XXH3_state_t XXH_IPREF(XXH3_state_t) +# define XXH128_hash_t XXH_IPREF(XXH128_hash_t) + /* Ensure header is parsed again, even if it was previously included */ +# undef XXHASH_H_5627135585666179 +# undef XXHASH_H_STATIC_13879238742 +#endif /* XXH_INLINE_ALL || XXH_PRIVATE_API */ + + +#ifndef XXHASH_H_5627135585666179 +#define XXHASH_H_5627135585666179 1 + +/* specific declaration modes for Windows */ +#if !defined(XXH_INLINE_ALL) && !defined(XXH_PRIVATE_API) # if defined(WIN32) && defined(_MSC_VER) && (defined(XXH_IMPORT) || defined(XXH_EXPORT)) # ifdef XXH_EXPORT # define XXH_PUBLIC_API __declspec(dllexport) @@ -116,7 +154,7 @@ extern "C" { # else # define XXH_PUBLIC_API /* do nothing */ # endif -#endif /* XXH_INLINE_ALL || XXH_PRIVATE_API */ +#endif /*! XXH_NAMESPACE, aka Namespace Emulation : * @@ -577,9 +615,14 @@ XXH_PUBLIC_API XXH128_hash_t XXH128_hashFromCanonical(const XXH128_canonical_t* #endif /* defined(XXH_STATIC_LINKING_ONLY) && !defined(XXHASH_H_STATIC_13879238742) */ +/* ======================================================================== */ +/* ======================================================================== */ +/* ======================================================================== */ + /*-********************************************************************** * xxHash implementation +* -********************************************************************** * Functions implementation used to be hosted within xxhash.c . * However, code inlining requires to place implementation in the header file. * As a consequence, xxhash.c used to be included within xxhash.h .