Copilot commented on code in PR #13166:
URL: https://github.com/apache/trafficserver/pull/13166#discussion_r3262554833
##########
CMakeLists.txt:
##########
@@ -281,6 +281,7 @@ include(CheckOpenSSLIsBoringSSL)
include(CheckOpenSSLIsQuictls)
include(CheckOpenSSLIsAwsLc)
find_package(OpenSSL REQUIRED)
+auto_option(SIMDUTF FEATURE_VAR TS_USE_SIMDUTF PACKAGE_DEPENDS simdutf)
Review Comment:
This enables TS_USE_SIMDUTF for any simdutf package that CMake can find, but
ink_base64.cc uses newer base64 APIs/options. Older installed simdutf packages
can therefore pass configure and then fail at compile time. Please require a
minimum simdutf version or add a compile-symbol check before enabling the
feature.
##########
tools/benchmark/CMakeLists.txt:
##########
@@ -36,6 +36,9 @@ target_link_libraries(benchmark_SharedMutex PRIVATE
Catch2::Catch2 ts::tscore li
add_executable(benchmark_Random benchmark_Random.cc)
target_link_libraries(benchmark_Random PRIVATE Catch2::Catch2WithMain
ts::tscore)
+add_executable(benchmark_ink_base64 benchmark_ink_base64.cc)
+target_link_libraries(benchmark_ink_base64 PRIVATE Catch2::Catch2WithMain
ts::tscore)
Review Comment:
Adding the correctness assertions only in this benchmark target leaves the
production base64 change uncovered by the normal test suite: tools/benchmark is
built only when ENABLE_BENCHMARKS is enabled and this executable is not
registered with add_catch2_test. Please add the scalar/SIMD contract cases,
including invalid truncation, URL-safe input, and in-place decode, to the
regular tscore tests or register a non-benchmark test target.
##########
tools/benchmark/benchmark_ink_base64.cc:
##########
@@ -0,0 +1,205 @@
+/** @file
+
+ Micro benchmark for ats_base64_encode / ats_base64_decode and the bulk
+ scalar tolower path used by URL canonicalization. Establishes a baseline
+ prior to any SIMD work.
Review Comment:
This file header is stale: it describes a pre-SIMD baseline and URL tolower
work, but the new benchmark is being added with the simdutf base64 backend
already wired in. Please update the description so the benchmark's purpose
matches the current code.
##########
src/tscore/ink_base64.cc:
##########
@@ -163,6 +174,69 @@ ats_base64_decode(const char *inBuffer, size_t
inBufferSize, unsigned char *outB
if (length) {
*length = decodedBytes;
}
+}
+
+} // namespace
+
+bool
+ats_base64_encode(const unsigned char *inBuffer, size_t inBufferSize, char
*outBuffer, size_t outBufSize, size_t *length)
+{
+ if (outBufSize < ats_base64_encode_dstlen(inBufferSize)) {
+ return false;
+ }
+
+#if TS_USE_SIMDUTF
+ if (inBufferSize > BASE64_ENCODE_SIMD_THRESHOLD) {
+ size_t written = simdutf::binary_to_base64(reinterpret_cast<const char
*>(inBuffer), inBufferSize, outBuffer);
+ outBuffer[written] = '\0';
+ if (length) {
+ *length = written;
+ }
+ return true;
+ }
+#endif
+
+ encode_scalar(inBuffer, inBufferSize, outBuffer, length);
+ return true;
+}
+
+bool
+ats_base64_encode(const char *inBuffer, size_t inBufferSize, char *outBuffer,
size_t outBufSize, size_t *length)
+{
+ return ats_base64_encode(reinterpret_cast<const unsigned char *>(inBuffer),
inBufferSize, outBuffer, outBufSize, length);
+}
+
+bool
+ats_base64_decode(const char *inBuffer, size_t inBufferSize, unsigned char
*outBuffer, size_t outBufSize, size_t *length)
+{
+ if (outBufSize < ats_base64_decode_dstlen(inBufferSize)) {
+ return false;
+ }
+
+#if TS_USE_SIMDUTF
+ if (inBufferSize > BASE64_DECODE_SIMD_THRESHOLD) {
+ // Reserve one byte for the trailing NUL we always emit.
+ size_t out_len = outBufSize - 1;
+ auto r = simdutf::base64_to_binary_safe(inBuffer, inBufferSize,
reinterpret_cast<char *>(outBuffer), out_len,
+
simdutf::base64_default_or_url, simdutf::last_chunk_handling_options::loose,
+
/*decode_up_to_bad_char=*/true);
Review Comment:
The SIMD decode path changes public TSBase64Decode behavior for inputs
containing ASCII whitespace: this simdutf mode skips it, while the scalar path
stops at the first such byte, so results now depend on build configuration and
input size. Please keep the two paths semantically aligned or make the API
contract explicitly opt into the new whitespace-tolerant behavior.
--
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]
