Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR reduces the size of the fastbase64 and sutf tools by linking them against feature-specific simdutf library variants and toggling off unused features at compile time.
Changes:
- Link
fastbase64against a base64-onlysimdutfstatic library and disable UTF-related features. - Link
sutfagainst a “no base64”simdutfstatic library and disable encoding detection/base64. - Make feature macros in
implementation.hoverridable via compile definitions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tools/CMakeLists.txt | Switch tools to link against feature-specific simdutf variants and add tool-level feature defines. |
| src/CMakeLists.txt | Add simdutf-base64 and simdutf-nobase64 static library targets built from simdutf.cpp. |
| include/simdutf/implementation.h | Change feature macro defaults to be conditional, allowing overrides from build flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(SIMDUTF_TOOLS) | ||
| # --- simdutf-base64: only base64 support (for fastbase64) --- | ||
| add_library(simdutf-base64 STATIC simdutf.cpp) | ||
| target_include_directories(simdutf-base64 PRIVATE $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>) | ||
| target_include_directories(simdutf-base64 PUBLIC "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>") | ||
| target_compile_definitions(simdutf-base64 PRIVATE | ||
| SIMDUTF_FEATURE_BASE64=1 | ||
| SIMDUTF_FEATURE_UTF8=0 | ||
| SIMDUTF_FEATURE_UTF16=0 | ||
| SIMDUTF_FEATURE_UTF32=0 | ||
| SIMDUTF_FEATURE_ASCII=0 | ||
| SIMDUTF_FEATURE_LATIN1=0 | ||
| SIMDUTF_FEATURE_DETECT_ENCODING=0 | ||
| ) | ||
| set_target_properties(simdutf-base64 PROPERTIES POSITION_INDEPENDENT_CODE ON) | ||
|
|
||
| # --- simdutf-nobase64: everything except base64 (for sutf) --- | ||
| add_library(simdutf-nobase64 STATIC simdutf.cpp) | ||
| target_include_directories(simdutf-nobase64 PRIVATE $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>) | ||
| target_include_directories(simdutf-nobase64 PUBLIC "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>") | ||
| target_compile_definitions(simdutf-nobase64 PRIVATE | ||
| SIMDUTF_FEATURE_BASE64=0 | ||
| SIMDUTF_FEATURE_UTF8=1 | ||
| SIMDUTF_FEATURE_UTF16=1 | ||
| SIMDUTF_FEATURE_UTF32=1 | ||
| SIMDUTF_FEATURE_ASCII=1 | ||
| SIMDUTF_FEATURE_LATIN1=1 | ||
| SIMDUTF_FEATURE_DETECT_ENCODING=0 | ||
| ) | ||
| set_target_properties(simdutf-nobase64 PROPERTIES POSITION_INDEPENDENT_CODE ON) | ||
| endif() |
There was a problem hiding this comment.
The new simdutf-base64 / simdutf-nobase64 targets don’t inherit the same platform/compiler-specific options and defines applied to the main simdutf target earlier in this file (e.g., PowerPC -maltivec/-mvsx/-mcpu=power8, fallback-related defines, etc.). This can cause build failures or behavior/perf differences when SIMDUTF_TOOLS is enabled. A concrete fix is to factor common configuration into a helper (e.g., a CMake function) and apply it to all three targets, so only the feature toggles differ.
|
I think that copilot is drunk. |
|
interesting experiment! If I build like this: #!/bin/sh
singleheader/amalgamate.py --with-base64
(cat singleheader/simdutf.h
grep -v '#include "simdutf.h"' singleheader/simdutf.cpp
tail -n +2 tools/fastbase64.cpp ) >main.cpp
/usr/lib/ccache/g++-14 -march=native -O3 -DNDEBUG -std=c++17 main.cppthe stripped result is 119 Kb. |
|
@pauldreik Really? Here is what I get on Rocky Linux (x86)... Before this PR: with this PR... So a dramatic reduction. On this machine This system assumes I think Debian still seek to support everything all the way back to the Pentium 4 so you are going to have 4 kernels. It might be up to 2x larger. Still, you should see a massive reduction in the size of the binary with this PR. Can you double check? |
|
I am sorry, I should have been clearer that I ran on master. I checked the flags used by a default build and repeated the test. Here is my full test script. #!/bin/sh
singleheader/amalgamate.py --with-base64
(cat singleheader/simdutf.h
grep -v '#include "simdutf.h"' singleheader/simdutf.cpp
tail -n +2 tools/fastbase64.cpp ) >main.cpp
c++ -O3 -DNDEBUG -std=c++17 -mno-avx256-split-unaligned-load -mno-avx256-split-unaligned-store main.cpp -o fastbase64.onetu
strip fastbase64.onetu
ls -lah fastbase64.onetu # 243K
c++ -O3 -march=native -DNDEBUG -std=c++17 -mno-avx256-split-unaligned-load -mno-avx256-split-unaligned-store main.cpp -o fastbase64.onetu-native
strip fastbase64.onetu-native
ls -lah fastbase64.onetu-native # 119K
cmake -B /tmp/blaha -S .
cmake --build /tmp/blaha/
strip /tmp/blaha/tools/fastbase64
ls -lah /tmp/blaha/tools/fastbase64 # 1.1 M before PR, 772K afterSo this PR indeed reduces the size (from 1.1M to 772K). On my machine, the single translation unit gets it down to 243K and to 119K if compiling for the current architecture. |
|
Ok. I am merging this since it is beneficial (less bloat). |
On my mac, I get this...
Interestingly, I get
vs
This is possible through the magic of features (thanks to @WojciechMula for the idea).
So, on my system,
fastbase64ends up SMALLER.This makes no sense to me. We should be much fatter.
Update: the trick is that we depend on the C++ lib which is a dynamic library under macOS.