Skip to content

upgrade bundled xxhash to 0.7.4#5366

Merged
ThomasWaldmann merged 2 commits intoborgbackup:1.1-maintfrom
ThomasWaldmann:upgrade-xxhash-1.1
Oct 1, 2020
Merged

upgrade bundled xxhash to 0.7.4#5366
ThomasWaldmann merged 2 commits intoborgbackup:1.1-maintfrom
ThomasWaldmann:upgrade-xxhash-1.1

Conversation

@ThomasWaldmann
Copy link
Member

No description provided.

@ThomasWaldmann
Copy link
Member Author

Hmm, there are compile issues. Master branch did compile. wtf?

https://travis-ci.org/github/borgbackup/borg/builds/731442394

@ThomasWaldmann
Copy link
Member Author

I remember that I already had such issues due to the xxh3 code, so I just left that part of the code away in earlier borg releases.

But now, xxh3 is integrated into xxhash.c/.h, so I can not easily leave it away any more.

@ThomasWaldmann
Copy link
Member Author

Another option, if the compile issue can't be easily fixed, is to just leave this as is and only have this upgrade in master branch.

@elho
Copy link
Contributor

elho commented Oct 1, 2020

I remember that I already had such issues due to the xxh3 code, so I just left that part of the code away in earlier borg releases.

Yes, that error message shows up if functions of extended intructions sets such as SSE and AVX are used, but the according -mXXX option to generate such code is not set. However, for borg it makes no sense to bulid e.g. SSE only, given the speed of xxHash compared to the rest of borg, the benefit would be hardly noticeable, if at all, and of course, being able to backup machines that do not have SSE instructions is a requirement anyway.

The xxHash README discusses the XXH_VECTOR macro. Maybe setting it to XXH_SCALAR scalar is enough to either bulit non-SSE2 variants of XXH3 and XXH128 or not built those. If that does not work, that would be a an issue on the xxHash side.

@Cyan4973
Copy link

Cyan4973 commented Oct 1, 2020

Indeed, XXH_VECTOR=XXH_SCALAR can be used to force the code to only employ regular instructions.
In general, it results in lower performance for XXH3 (but it doesn't impact XXH64).
The resulting binary might still be auto-vectorized, but that's now the responsibility of the compiler.

The one thing that surprises me though is that the compilation error message seems to choke at some SSE2 instructions.

emmintrin.h:1029:1: error: inlining failed in call to always_inline ‘_mm_add_epi64’: target specific option mismatch

But SSE2 is not some kind of "optional" extension : on x64 platforms, it is mandatory, plain and simple. There is no x64 cpu without SSE2. Consequently, all compilers I'm aware of (gcc, clang, visual, icc) default to SSE2 enabled when compiling for x64. And at least some compilers (tested on clang) are unable to generate x64 binaries if one tries to disable SSE2.

SSE2 is not guaranteed on 32-bit x86, so it makes more sense to specifically disable it for this platform. But my understanding is that compilation tests on travisCI target x86-64, so that's not the use case. Moreover, no special action should be needed for 32-bit x86 : XXH3 source code should determine the outcome automatically, and disable vector code by default.

@ThomasWaldmann
Copy link
Member Author

@Cyan4973 @elho thanks for the insights. Still strange that in master branch it just worked.

"as is", needs a small tweak, see next commit.

this is the last version with separate xxh3.h that can easily
get removed. 0.8.0 integrates this and leads to strange compile
issues, see borgbackup#5366 discussion.
@ThomasWaldmann ThomasWaldmann changed the title upgrade bundled xxhash to 0.8.0 upgrade bundled xxhash to 0.7.4 Oct 1, 2020
@ThomasWaldmann
Copy link
Member Author

Guess this might be an issue with the 1.1-maint "build system", master is cleaner there.

I'll updated this PR to just use the latest pre-0.8.0 code, where I can just remove the problematic xxh3.h (we don't use any of the new stuff yet anyway).

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2020

Codecov Report

Merging #5366 into 1.1-maint will increase coverage by 0.31%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##           1.1-maint    #5366      +/-   ##
=============================================
+ Coverage      79.80%   80.12%   +0.31%     
=============================================
  Files             27       27              
  Lines          10302    10396      +94     
  Branches        1824     1829       +5     
=============================================
+ Hits            8222     8330     +108     
+ Misses          1565     1555      -10     
+ Partials         515      511       -4     
Impacted Files Coverage Δ
src/borg/crypto/file_integrity.py 98.63% <0.00%> (+0.01%) ⬆️
src/borg/repository.py 85.42% <0.00%> (+0.02%) ⬆️
src/borg/nanorst.py 88.75% <0.00%> (+0.07%) ⬆️
src/borg/crypto/key.py 87.10% <0.00%> (+0.20%) ⬆️
src/borg/logger.py 72.26% <0.00%> (+0.23%) ⬆️
src/borg/archiver.py 81.95% <0.00%> (+0.30%) ⬆️
src/borg/helpers.py 87.41% <0.00%> (+0.32%) ⬆️
src/borg/remote.py 80.44% <0.00%> (+0.51%) ⬆️
src/borg/archive.py 82.53% <0.00%> (+0.97%) ⬆️
src/borg/upgrader.py 62.06% <0.00%> (+1.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40299d7...0056c59. Read the comment docs.

@ThomasWaldmann ThomasWaldmann merged commit 2333047 into borgbackup:1.1-maint Oct 1, 2020
@ThomasWaldmann ThomasWaldmann deleted the upgrade-xxhash-1.1 branch October 1, 2020 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants