Skip to content

Conversation

Brooooooklyn
Copy link
Member

@Brooooooklyn Brooooooklyn commented Oct 11, 2025

https://github.com/cloudwego/sonic-rs is faster than our previous implementation, but we don't want to use it directly.

  1. Using sonic-rs only for string escaping would significantly increase the build time.
  2. Using sonic-rs only for string escaping introduces unnecessary abstraction overhead.

For small inputs:

sonic_rs::to_string(source).unwrap()

vs

json_escape_simd::escape(source)
Benchmarking short string escape simd: Collecting 100 samples in estimated 5
short string escape simd
                        time:   [91.420 ns 91.942 ns 92.492 ns]
                        change: [−4.1512% −3.5514% −2.9689%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

Benchmarking short string escape sonic: Collecting 100 samples in estimated 
short string escape sonic
                        time:   [200.26 ns 201.38 ns 202.58 ns]
                        change: [−3.9808% −3.4283% −2.8871%] (p = 0.00 < 0.05)
                        Performance has improved.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

codspeed-hq bot commented Oct 11, 2025

CodSpeed Performance Report

Merging #27 will improve performances by 25.75%

Comparing 10-11-bench_add_sonic-rs_benchmark (605d6da) with main (85a3cca)

Summary

⚡ 2 improvements
🆕 1 new
⏩ 8 skipped1

Benchmarks breakdown

Benchmark BASE HEAD Change
fixtures escape simd 40.4 ms 31.4 ms +28.89%
rxjs escape simd 1,179.7 µs 938.1 µs +25.75%
🆕 short string escape simd N/A 2.3 µs N/A

Footnotes

  1. 8 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.

@Brooooooklyn Brooooooklyn changed the title bench: add sonic-rs benchmark refactor: borrow the sonic-rs string escape implementation Oct 12, 2025
@Brooooooklyn Brooooooklyn requested a review from Copilot October 12, 2025 06:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the library to borrow the sonic-rs string escape implementation instead of using the original custom SIMD implementations. The change aims to improve performance, particularly for small inputs, while avoiding the overhead of using sonic-rs directly as a dependency.

  • Replaced custom x86 and aarch64 SIMD implementations with sonic-rs borrowed code
  • Removed platform-specific modules and feature flags
  • Updated benchmarks to include sonic-rs comparisons and short string tests

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/lib.rs Complete rewrite using sonic-simd crate with borrowed string escaping logic
src/x86.rs Removed entire x86-specific SIMD implementation
src/aarch64.rs Removed entire aarch64 NEON implementation
examples/escape.rs Updated to compare against sonic-rs and serde_json
benches/escape.rs Added sonic-rs benchmark and short string tests
benches/generic.rs Removed unused escape_into_generic function
Cargo.toml Added sonic-simd, sonic-rs dependencies and updated criterion
README.md Updated documentation to reflect sonic-rs implementation
.github/workflows/CI.yml Added miri testing workflow

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Brooooooklyn
Copy link
Member Author

@codex review

@Brooooooklyn
Copy link
Member Author

cursor review

Copy link

cursor bot commented Oct 12, 2025

Skipping Bugbot: Bugbot is disabled for this repository

@napi-rs napi-rs deleted a comment from cursor bot Oct 12, 2025
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

@Brooooooklyn
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

@Brooooooklyn Brooooooklyn marked this pull request as ready for review October 12, 2025 06:36
@Brooooooklyn Brooooooklyn merged commit d4d94e3 into main Oct 12, 2025
13 checks passed
@Brooooooklyn Brooooooklyn deleted the 10-11-bench_add_sonic-rs_benchmark branch October 12, 2025 06:47
This was referenced Oct 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant