Skip to content

FFI: replace vx_string/vx_binary with vx_view#8668

Merged
myrrc merged 1 commit into
developfrom
myrrc/ffi-vxstr
Jul 7, 2026
Merged

FFI: replace vx_string/vx_binary with vx_view#8668
myrrc merged 1 commit into
developfrom
myrrc/ffi-vxstr

Conversation

@myrrc

@myrrc myrrc commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

vx_string/vx_binary is not a zero-copy abstraction. For every utf/binary view you allocate memory. This PR replaces both with a vx_view non-owning view akin to C++'s string_view.

@myrrc myrrc added changelog/break A breaking API change lang/c Relates to the Vortex C (FFI) API labels Jul 7, 2026
@myrrc myrrc changed the title FFI: remove vx_string/vx_binary with vx_str FFI: replace vx_string/vx_binary with vx_str Jul 7, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 7, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚡ 3 improved benchmarks
❌ 2 regressed benchmarks
✅ 1602 untouched benchmarks
⏩ 42 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_varbinview_into_canonical[(1000, 10)] 170.1 µs 206.5 µs -17.62%
Simulation compact[(4096, 90)] 750.6 ns 867.2 ns -13.45%
Simulation chunked_varbinview_canonical_into[(1000, 10)] 190.5 µs 153.9 µs +23.72%
Simulation chunked_varbinview_into_canonical[(100, 100)] 307.7 µs 272.5 µs +12.93%
Simulation bitwise_not_vortex_buffer_mut[128] 273.6 ns 244.4 ns +11.93%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing myrrc/ffi-vxstr (deb614b) with develop (0d50342)2

Open in CodSpeed

Footnotes

  1. 42 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on develop (4abe3d0) during the generation of this report, so 0d50342 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@myrrc myrrc force-pushed the myrrc/ffi-vxstr branch 2 times, most recently from fd4843b to dfd9d1d Compare July 7, 2026 12:25
@myrrc myrrc requested a review from 0ax1 July 7, 2026 12:27
@myrrc myrrc marked this pull request as ready for review July 7, 2026 12:27
@myrrc myrrc enabled auto-merge (squash) July 7, 2026 12:27

@0ax1 0ax1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the value of splitting these types. It seems we now use str also for buffers that are just bytes.

Comment thread vortex-ffi/cinclude/vortex.h Outdated
Comment thread vortex-ffi/cinclude/vortex.h
@0ax1 0ax1 disabled auto-merge July 7, 2026 14:50

@0ax1 0ax1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but lets rename to vx_view or span as discussed.

@myrrc myrrc force-pushed the myrrc/ffi-vxstr branch 2 times, most recently from 5d4c34f to 6bad344 Compare July 7, 2026 15:57
@myrrc myrrc changed the title FFI: replace vx_string/vx_binary with vx_str FFI: replace vx_string/vx_binary with vx_view Jul 7, 2026
@myrrc myrrc enabled auto-merge (squash) July 7, 2026 16:00
@myrrc myrrc force-pushed the myrrc/ffi-vxstr branch from 6bad344 to 5ab787d Compare July 7, 2026 16:02
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
@myrrc myrrc force-pushed the myrrc/ffi-vxstr branch from 5ab787d to deb614b Compare July 7, 2026 16:35
@myrrc myrrc merged commit 1e91494 into develop Jul 7, 2026
66 of 68 checks passed
@myrrc myrrc deleted the myrrc/ffi-vxstr branch July 7, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change lang/c Relates to the Vortex C (FFI) API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants