pack: Implement SIMD fast paths #10848
Open
Conversation
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
WalkthroughIntroduces SIMD-assisted detection and fast paths in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant pack_string_token
participant json_find_escapable_simd as json_find_escapable_simd
participant Unescape as Unescape/Pack
Caller->>pack_string_token: pack_string_token(str, len, ...)
alt FLB_HAVE_SIMD enabled
pack_string_token->>json_find_escapable_simd: scan for escapables
alt No escapables found
pack_string_token->>Caller: pack direct (no unescape)
else Escapable found
pack_string_token->>Unescape: unescape and pack
Unescape-->>Caller: result
end
else No SIMD
pack_string_token->>Unescape: scalar path (existing logic)
Unescape-->>Caller: result
end
sequenceDiagram
autonumber
participant Caller
participant is_float
Caller->>is_float: is_float(p, end)
alt FLB_HAVE_SIMD enabled
is_float->>is_float: SIMD scan for '.' / 'e' / 'E' (+/-)
note right of is_float: On match jump to scalar verification
end
is_float->>is_float: Scalar verification (tail / edge cases)
is_float-->>Caller: boolean result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Hiroshi Hatake <[email protected]>
cosmo0920
force-pushed
the
cosmo0920-implement-simd-fast-paths-for-packing-msgpack
branch
from
2835295 to
a7d579b
Compare
cosmo0920
requested review from
edsiper,
fujimotos and
koleini
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/flb_pack.c (1)
287-289: Minor: widen buf_size type on assignmentstate->buf_size appears to be size_t; assigning int s is fine on typical platforms but consider making s size_t to avoid narrowing on large inputs.
- int s; + size_t s; @@ - s = len + 1; + s = (size_t)len + 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_pack.c(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
PR: fluent/fluent-bit#10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/flb_pack.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
PR: fluent/fluent-bit#10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/flb_pack.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
PR: fluent/fluent-bit#9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/flb_pack.c
🧬 Code graph analysis (1)
src/flb_pack.c (2)
include/fluent-bit/flb_simd.h (4)
flb_vector8_broadcast(228-239)flb_vector8_load(140-151)flb_vector8_is_highbit_set(244-257)flb_vector8_eq(194-206)src/flb_unescape.c (1)
flb_unescape_string_utf8(186-277)
🔇 Additional comments (3)
src/flb_pack.c (3)
32-32: Header include looks fineUnconditional include is OK since flb_simd.h provides non-SIMD fallbacks; no guard needed here.
270-279: Fast-path pack for strings is good, contingent on escapable finder correctnessBypassing unescape when no '"' '' or control chars are present is a clear win. Once json_find_escapable_simd is fixed to catch control chars inside blocks, this path is safe.
Please add a regression case: a long string containing a lone 0x1F byte in the middle should take the slow path and be unescaped; a purely ASCII string should take the fast path.
57-109: The above script will list everyflb_vector8_…symbol so we can confirm if any compare/bitwise ops exist. Once we see the available helpers, I’ll finalize whether a fully SIMD-based control‐char test is feasible or if the minimal scalar‐in‐block fix is the correct recommendation.
Signed-off-by: Hiroshi Hatake <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit