rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Upgrade to xxhash 0.8.2 as conan dependency, enable SIMD hashing

Open Bronek opened this issue 1 year ago • 1 comments

High Level Overview of Change

Upgrade from xxhash from 0.6.2 to 0.8.2 , switch to conan dependency , enable SIMD

Context of Change

We are currently using old version 0.6.2 of xxhash, as a verbatim copy & paste from its header file xxhash.h. Switch to the more recent version 0.8.2 . Since this version is supported by conan (and properly protects its ABI by keeping the state object incomplete), add it as a conan dependency. Also, since this new version supports SIMD instructions (in the new XXH3 family of functions), switch to the new functions to make use of these more performant CPU instructions.

Type of Change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Refactor (non-breaking change that only restructures code)
  • [x] Performance (increase or change in throughput and/or latency)
  • [ ] Tests (you added tests for code that already exists, or your new feature included in this PR)
  • [ ] Documentation update
  • [ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • [ ] Release

This change requires performance testing.

Internal tracker: RPFC-85

Bronek avatar Jan 19 '24 14:01 Bronek

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 76.94%. Comparing base (22b7518) to head (6843b1e). Report is 4 commits behind head on develop.

Files Patch % Lines
src/ripple/beast/hash/xxhasher.h 85.71% 1 Missing and 1 partial :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #4893       +/-   ##
============================================
+ Coverage    61.65%   76.94%   +15.29%     
============================================
  Files          806     1127      +321     
  Lines        70967   131676    +60709     
  Branches     36690    39515     +2825     
============================================
+ Hits         43752   101323    +57571     
- Misses       19856    24395     +4539     
+ Partials      7359     5958     -1401     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jan 23 '24 23:01 codecov-commenter

This has cleared performance testing in https://ripplelabs.atlassian.net/browse/RPFC-85

Bronek avatar Mar 06 '24 14:03 Bronek

  • awaiting review by @ximinez (or suggestion of alternate reviewer)

intelliot avatar Mar 07 '24 19:03 intelliot

XXH64_update instead of XXH3_64bits_update

Yes, this the upgrade to SIMD instructions part. In both cases (old and new code) we are operating on 64-bit data type.

Bronek avatar Mar 07 '24 22:03 Bronek

Everything looks good. I'm not very familiar with this library, so I took a quick look through the documentation example. I noticed that it does a lot more error checking than this version. Is that something we should be concerned about?

Huh, maybe I need to add handling running out of memory in the constructors, otherwise other operations will silently fail. How do we normally handle it in this project ?

XXH_PUBLIC_API XXH3_state_t* XXH3_createState(void)
{
    XXH3_state_t* const state = (XXH3_state_t*)XXH_alignedMalloc(sizeof(XXH3_state_t), 64);
    if (state==NULL) return NULL;

Bronek avatar Mar 07 '24 22:03 Bronek

No, you don't need to do anything special about running out of memory. If rippled runs out of memory it bursts into flame. There's no special handling for low memory situations.

scottschurr avatar Mar 07 '24 22:03 scottschurr

No, you don't need to do anything special about running out of memory. If rippled runs out of memory it bursts into flame. There's no special handling for low memory situations.

In this case throwing std::bad_alloc is better than failing silently. I will add it.

Bronek avatar Mar 07 '24 22:03 Bronek

@ximinez do you mind checking again ? I added throw std::bad_alloc() where appropriate. There are no other error paths that I can see in in xxhash sources.

Bronek avatar Mar 08 '24 16:03 Bronek