wwbmmm commented on code in PR #3684: URL: https://github.com/apache/brpc/pull/3684#discussion_r4500972745
########## src/butil/third_party/modp_b64/README.chromium: ########## @@ -13,3 +13,8 @@ basictypes.h could be possible. The modp_b64.cc and modp_b64.h files were modified to make them safe on 64-bit systems. + +The AVX2 codec in modp_b64_avx2.cc adapts the AVX2 reshuffle, translate, and +validation routines from https://github.com/aklomp/base64, commit Review Comment: [replied by brpc-oncall robot] Code from aklomp/base64 is now bundled under `src/butil/third_party/modp_b64`, but the top-level `LICENSE` file is not updated. That file already carries one section per bundled third-party component (see the `src/butil/third_party/modp_b64:` section around line 291), and ASF releases must list every bundled third-party license there. Please add the aklomp/base64 copyright lines and the BSD-2-Clause text from `LICENSE.aklomp-base64` to the top-level `LICENSE` (and update `NOTICE` if required), so the new dependency is properly recorded. ########## src/butil/third_party/modp_b64/modp_b64_avx2.cc: ########## @@ -0,0 +1,217 @@ +// The AVX2 routines in this file are derived from aklomp/base64. +// Copyright (c) 2005-2007, Nick Galbreath +// Copyright (c) 2015-2018, Wojciech Muła +// Copyright (c) 2016-2017, Matthieu Darbois +// Copyright (c) 2013-2022, Alfred Klomp +// SPDX-License-Identifier: BSD-2-Clause + +#include "modp_b64_avx2.h" + +#if MODP_B64_HAS_AVX2 + +#include <immintrin.h> +#include <stdint.h> + +namespace { + +#define MODP_B64_AVX2_TARGET __attribute__((target("avx2"))) +#define BASE64_FORCE_INLINE MODP_B64_AVX2_TARGET inline + +const char kAlphabet[] = + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; + +static BASE64_FORCE_INLINE __m256i +enc_reshuffle(const __m256i input) { + const __m256i in = _mm256_shuffle_epi8(input, _mm256_set_epi8( + 10, 11, 9, 10, 7, 8, 6, 7, 4, 5, 3, 4, 1, 2, 0, 1, + 14, 15, 13, 14, 11, 12, 10, 11, 8, 9, 7, 8, 5, 6, 4, 5)); + const __m256i t0 = _mm256_and_si256(in, _mm256_set1_epi32(0x0FC0FC00)); + const __m256i t1 = _mm256_mulhi_epu16(t0, _mm256_set1_epi32(0x04000040)); + const __m256i t2 = _mm256_and_si256(in, _mm256_set1_epi32(0x003F03F0)); + const __m256i t3 = _mm256_mullo_epi16(t2, _mm256_set1_epi32(0x01000010)); + return _mm256_or_si256(t1, t3); +} + +static BASE64_FORCE_INLINE __m256i +enc_translate(const __m256i in) { + const __m256i lut = _mm256_setr_epi8( + 65, 71, -4, -4, -4, -4, -4, -4, -4, -4, -4, -4, -19, -16, 0, 0, + 65, 71, -4, -4, -4, -4, -4, -4, -4, -4, -4, -4, -19, -16, 0, 0); + __m256i indices = _mm256_subs_epu8(in, _mm256_set1_epi8(51)); + const __m256i mask = _mm256_cmpgt_epi8(in, _mm256_set1_epi8(25)); + indices = _mm256_sub_epi8(indices, mask); + return _mm256_add_epi8(in, _mm256_shuffle_epi8(lut, indices)); +} + +static BASE64_FORCE_INLINE __m256i +dec_reshuffle(const __m256i in) { + const __m256i merge_ab_and_bc = + _mm256_maddubs_epi16(in, _mm256_set1_epi32(0x01400140)); + __m256i out = + _mm256_madd_epi16(merge_ab_and_bc, _mm256_set1_epi32(0x00011000)); + out = _mm256_shuffle_epi8(out, _mm256_setr_epi8( + 2, 1, 0, 6, 5, 4, 10, 9, 8, 14, 13, 12, -1, -1, -1, -1, + 2, 1, 0, 6, 5, 4, 10, 9, 8, 14, 13, 12, -1, -1, -1, -1)); + return _mm256_permutevar8x32_epi32( + out, _mm256_setr_epi32(0, 1, 2, 4, 5, 6, -1, -1)); +} + +inline int DecodeChar(uint8_t c) { + if (c >= 'A' && c <= 'Z') return c - 'A'; + if (c >= 'a' && c <= 'z') return c - 'a' + 26; + if (c >= '0' && c <= '9') return c - '0' + 52; + if (c == '+') return 62; + if (c == '/') return 63; + return -1; +} + +inline bool DecodeQuad(const uint8_t* src, uint8_t* dest) { + const int a = DecodeChar(src[0]); + const int b = DecodeChar(src[1]); + const int c = DecodeChar(src[2]); + const int d = DecodeChar(src[3]); + if (a < 0 || b < 0 || c < 0 || d < 0) return false; + dest[0] = static_cast<uint8_t>((a << 2) | (b >> 4)); + dest[1] = static_cast<uint8_t>(((b & 15) << 4) | (c >> 2)); + dest[2] = static_cast<uint8_t>(((c & 3) << 6) | d); + return true; +} + +} // namespace + +extern "C" int modp_b64_avx2_supported(void) { + __builtin_cpu_init(); + return __builtin_cpu_supports("avx2"); +} + +extern "C" MODP_B64_AVX2_TARGET size_t modp_b64_avx2_encode( + char* dest, const char* src, size_t len) { + size_t input = 0; + uint8_t* output = reinterpret_cast<uint8_t*>(dest); + size_t rounds = len >= 32 ? (len - 4) / 24 : 0; + if (rounds != 0) { + // The first shuffle needs four zero bytes at the start of its lower Review Comment: [replied by brpc-oncall robot] The comment says the first shuffle "needs four zero bytes at the start of its lower lane", but no zeros are involved: `_mm256_permutevar8x32_epi32(..., {0,0,1,2,3,4,5,6})` duplicates dword 0 so that this block encodes `src[0..24)` exactly as a plain (non-permuted) block loaded at `src - 4`. That 4-byte shift is also the invariant behind `input = 20` and the later `input += 4`. Since this is the subtlest part of the file, please rewrite the comment to state the real invariant (first block emulates a load 4 bytes earlier; following blocks are loaded at 20 + 24*k; the trailing +4 converts back to the actually consumed length) instead of mentioning zero bytes. ########## CMakeLists.txt: ########## @@ -217,6 +218,24 @@ set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/output/lib) set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/output/lib) add_subdirectory(src) + +if(BUILD_BASE64_BENCHMARKS) + add_executable(base64_benchmark_scalar Review Comment: [replied by brpc-oncall robot] These two targets do not link `brpc_common_config`, so they do not inherit `cxx_std_${BRPC_CXX_STANDARD}` (set at line 214), while `test/base64_benchmark.cpp` requires at least C++11 (`std::chrono`, lambdas). It compiles today only because most compilers default to a new enough standard; on toolchains with an older default it will fail. Please add `target_compile_features(base64_benchmark_scalar PRIVATE cxx_std_${BRPC_CXX_STANDARD})` (same for `base64_benchmark_avx2`), or link `brpc_common_config`. ########## src/butil/third_party/modp_b64/modp_b64_avx2.cc: ########## @@ -0,0 +1,217 @@ +// The AVX2 routines in this file are derived from aklomp/base64. +// Copyright (c) 2005-2007, Nick Galbreath +// Copyright (c) 2015-2018, Wojciech Muła +// Copyright (c) 2016-2017, Matthieu Darbois +// Copyright (c) 2013-2022, Alfred Klomp +// SPDX-License-Identifier: BSD-2-Clause + +#include "modp_b64_avx2.h" + +#if MODP_B64_HAS_AVX2 + +#include <immintrin.h> +#include <stdint.h> + +namespace { + +#define MODP_B64_AVX2_TARGET __attribute__((target("avx2"))) +#define BASE64_FORCE_INLINE MODP_B64_AVX2_TARGET inline + +const char kAlphabet[] = + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; + +static BASE64_FORCE_INLINE __m256i +enc_reshuffle(const __m256i input) { + const __m256i in = _mm256_shuffle_epi8(input, _mm256_set_epi8( + 10, 11, 9, 10, 7, 8, 6, 7, 4, 5, 3, 4, 1, 2, 0, 1, + 14, 15, 13, 14, 11, 12, 10, 11, 8, 9, 7, 8, 5, 6, 4, 5)); + const __m256i t0 = _mm256_and_si256(in, _mm256_set1_epi32(0x0FC0FC00)); + const __m256i t1 = _mm256_mulhi_epu16(t0, _mm256_set1_epi32(0x04000040)); + const __m256i t2 = _mm256_and_si256(in, _mm256_set1_epi32(0x003F03F0)); + const __m256i t3 = _mm256_mullo_epi16(t2, _mm256_set1_epi32(0x01000010)); + return _mm256_or_si256(t1, t3); +} + +static BASE64_FORCE_INLINE __m256i +enc_translate(const __m256i in) { + const __m256i lut = _mm256_setr_epi8( + 65, 71, -4, -4, -4, -4, -4, -4, -4, -4, -4, -4, -19, -16, 0, 0, + 65, 71, -4, -4, -4, -4, -4, -4, -4, -4, -4, -4, -19, -16, 0, 0); + __m256i indices = _mm256_subs_epu8(in, _mm256_set1_epi8(51)); + const __m256i mask = _mm256_cmpgt_epi8(in, _mm256_set1_epi8(25)); + indices = _mm256_sub_epi8(indices, mask); + return _mm256_add_epi8(in, _mm256_shuffle_epi8(lut, indices)); +} + +static BASE64_FORCE_INLINE __m256i +dec_reshuffle(const __m256i in) { + const __m256i merge_ab_and_bc = + _mm256_maddubs_epi16(in, _mm256_set1_epi32(0x01400140)); + __m256i out = + _mm256_madd_epi16(merge_ab_and_bc, _mm256_set1_epi32(0x00011000)); + out = _mm256_shuffle_epi8(out, _mm256_setr_epi8( + 2, 1, 0, 6, 5, 4, 10, 9, 8, 14, 13, 12, -1, -1, -1, -1, + 2, 1, 0, 6, 5, 4, 10, 9, 8, 14, 13, 12, -1, -1, -1, -1)); + return _mm256_permutevar8x32_epi32( + out, _mm256_setr_epi32(0, 1, 2, 4, 5, 6, -1, -1)); +} + +inline int DecodeChar(uint8_t c) { + if (c >= 'A' && c <= 'Z') return c - 'A'; + if (c >= 'a' && c <= 'z') return c - 'a' + 26; + if (c >= '0' && c <= '9') return c - '0' + 52; + if (c == '+') return 62; + if (c == '/') return 63; + return -1; +} + +inline bool DecodeQuad(const uint8_t* src, uint8_t* dest) { + const int a = DecodeChar(src[0]); + const int b = DecodeChar(src[1]); + const int c = DecodeChar(src[2]); + const int d = DecodeChar(src[3]); + if (a < 0 || b < 0 || c < 0 || d < 0) return false; + dest[0] = static_cast<uint8_t>((a << 2) | (b >> 4)); + dest[1] = static_cast<uint8_t>(((b & 15) << 4) | (c >> 2)); + dest[2] = static_cast<uint8_t>(((c & 3) << 6) | d); + return true; +} + +} // namespace + +extern "C" int modp_b64_avx2_supported(void) { Review Comment: [replied by brpc-oncall robot] `modp_b64_avx2_supported()` is called from `modp_b64_encode()` / `modp_b64_decode()` on every call above the threshold, i.e. exactly on the path this PR is trying to speed up. With GCC `__builtin_cpu_init()` is a real call (and `__builtin_cpu_supports()` reads a global), which is not negligible when the encode threshold is only 32 bytes. Please cache the result once, e.g. `static const bool kSupported = [] { __builtin_cpu_init(); return __builtin_cpu_supports("avx2"); }(); return kSupported;` (function-local static, thread-safe initialization in C++11 and later). ########## test/base64_unittest.cc: ########## @@ -24,4 +24,28 @@ TEST(Base64Test, Basic) { EXPECT_EQ(kText, decoded); } +TEST(Base64Test, RoundTripBinaryPayloads) { + for (size_t size = 0; size <= 257; ++size) { + std::string input; + input.reserve(size); + for (size_t i = 0; i < size; ++i) { + input.push_back(static_cast<char>((i * 37 + size) & 0xff)); + } + + std::string encoded; + std::string decoded; + Base64Encode(input, &encoded); + ASSERT_TRUE(Base64Decode(encoded, &decoded)) << "size=" << size; + EXPECT_EQ(input, decoded) << "size=" << size; + } +} + +TEST(Base64Test, RejectsInvalidInput) { Review Comment: [replied by brpc-oncall robot] All the negative cases here are shorter than 45 characters, so `modp_b64_avx2_decode()` handles them entirely with the scalar tail; the vectorized validation (the `lut_lo`/`lut_hi` + `_mm256_testz_si256` check) is never exercised with invalid input. Please add cases on AVX2 hosts that reach it, e.g. a 1KB string of 'A' with a single invalid byte ('!', a space, or a byte >= 0x80) in the middle, one with '=' in the middle of a long input, and one ending with valid padding - so a regression in the vector validation path is caught by CI instead of by users. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
