perf: remove remaining variable-size memmove/memset in to_chars - #2808
Open
lemire wants to merge 4 commits into
Open
perf: remove remaining variable-size memmove/memset in to_chars#2808lemire wants to merge 4 commits into
lemire wants to merge 4 commits into
Conversation
Finish the work started in #2805: the cold format_buffer paths for 0.[000]digits and scientific notation still called memmove/memset with runtime lengths. Use fixed-size copies (with a small temp for the overlapping shifts) so those calls can be inlined as well. Document and name the required 40-byte caller scratch as internal::to_chars_buffer_size: only ~24 characters are emitted, but fixed-size 16/17-byte over-writes need the extra room for safety.
inline constexpr is a C++17 feature. CI still builds C++11 targets (readme_examples11, quickstart11, etc.) with -Werror=c++17-extensions, which rejected the constant. Use static constexpr instead.
CI failed on simdjson-singleheader with -Wunused-const-variable: static constexpr to_chars_buffer_size is only referenced from header callers, so it is unused in the amalgamated .cpp TU under -Werror. Use an unscoped enum (C++11-friendly, never "unused") and regenerate the singleheader amalgamation so the committed files match the sources.
just_ascii fails when .cpp sources contain non-ASCII. Re-amalgamation copied an em dash from portability.h into singleheader/simdjson.cpp. Replace it with ASCII -- so CI passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #2805. That PR inlined the hot-path
memcpy/memset/memmoveindragonbox()/format_buffer()by using compile-time sizes and a 40-byte caller scratch buffer. Two cold paths still used variable-lengthmemmove/memset:0.[000]digits(small magnitudes with leading zeros before the digit string)d.igitsE±xx(insert.after the leading digit)This PR replaces those with fixed-size copies (via a small stack temp when the shift overlaps) so all of
to_charsformat paths can be inlined. On big4 (GCC 14, Release),objdumpofsimdjson::internal::to_charsgoes from 2 libcmem*calls to 0.Also documents and names the required caller capacity as
simdjson::internal::to_chars_buffer_size(40): only ~24 characters are ever part of the logical result, but fixed-size 16/17-byte over-writes need the extra scratch for safety. Call sites in DOM serialization andstring_builderuse the constant.Changes
src/to_chars.cpp: remove remaining variable-sizememmove/memset; clarify over-write contractinclude/simdjson/common_defs.h: addto_chars_buffer_size+ API notesserialization-inl.h,json_string_builder-inl.huse the named constantCorrectness (big4)
builder_string_builder_tests: Successrandom_string_number_tests: All tests okb3072d25, post-perf: eliminate libc memcpy/memset/memmove calls in to_chars hotpath #2805) vs this branch for 199906 random finite doubles: identicalstrtodround-trip on the same set: okBenchmarks (big4)
Host: Intel Xeon Gold 6548N, g++ 14.3.1, Release,
taskset -c 4, 5 interleaved rounds × 3 trials ofbenchmark_car_builder.mem*into_charscar_builderis mostly normal magnitudes, so wall time is nearly flat; instruction count drops a bit and the last libcmem*calls disappear fromto_chars.Test plan
builder_string_builder_testsrandom_string_number_testsbenchmark_car_builderA/B on big4