Repository navigation
perf: parse WebSocket ingress with sonic-rs - #55
Conversation
Praying
left a comment
There was a problem hiding this comment.
I reviewed the ingress parser change against the PR description and exercised the workspace, bin, bench, and targeted WebSocket test targets. The core byte-based parsing path is sound, but the issues below should be addressed before merging, especially the payload disclosure and production dependency boundary.
| let count = counter.fetch_add(1, Ordering::Relaxed) + 1; | ||
| if count % 100 == 1 { | ||
| warn!( | ||
| error = %parse_error, |
There was a problem hiding this comment.
[P1] Keep parser diagnostics payload-free
sonic_rs::Error is not safe to format here: its Display implementation includes a fragment of the input around the error position (roughly 16 bytes). As a result, malformed private/account WebSocket messages can leak payload content into logs, contrary to the payload-free diagnostics guarantee in the PR. Please log only sanitized metadata such as the error category/line/column, frame type, payload size, and counter.
| sonic-rs = { workspace = true } | ||
|
|
||
| # Terminal benchmark UI | ||
| ratatui = { workspace = true } |
There was a problem hiding this comment.
[P1] Keep benchmark-only dependencies out of normal builds
ratatui, crossterm, futures-util, tokio-tungstenite, and the root sonic-rs dependency are used by the benchmark binary, but they are added as unconditional root dependencies. Because src/bin/binance_ws_bench.rs is auto-discovered, ordinary ccxt-rust builds and downstream consumers now resolve and compile the terminal UI stack. Please isolate this target in a benchmark/example crate, or make the dependencies optional and gate the binary with required-features.
| self.last = value; | ||
| self.total += u128::from(value); | ||
| self.count += 1; | ||
| if self.values.len() == SAMPLE_CAPACITY { |
There was a problem hiding this comment.
[P2] Use one sampling window for the displayed statistics
When the deque reaches 4096 samples, the oldest value is removed from values, but it is never subtracted from total and count remains cumulative. Therefore mean covers the full process lifetime while p50/p95/p99 cover only the rolling 4096 samples, despite the UI labeling the table as rolling. Please either maintain a rolling sum/count or label the mean as cumulative.
| .collect() | ||
| } | ||
|
|
||
| // Byte-for-byte equivalent to WsClient::validate_ingress_json_depth. |
There was a problem hiding this comment.
[P2] Benchmark the production depth guard instead of a copy
This helper duplicates WsClient::validate_ingress_json_depth byte-for-byte. A later production fix can leave the benchmark measuring stale behavior without any failure. Please expose the production helper to the benchmark through an appropriate internal/test API, or add a shared helper so the measured code cannot drift.
Summary
sonic-rsas a workspace dependency forccxt-coresonic_rs::from_sliceWsClient's publicserde_json::Valuereceive typeingress_parse_errors()and emit rate-limited, payload-free warningsWhy
WebSocket ingress is a high-frequency JSON parsing path. Using sonic-rs moves decoding to its SIMD-aware parser, and binary frames avoid the previous intermediate UTF-8
Stringallocation.Hardening found that sufficiently deep input could exhaust the runtime thread stack before sonic-rs returned its recursion error. The preflight nesting guard preserves the effective limit of the previous serde_json parser and rejects that input before recursive deserialization.
Developer impact
The receive type and downstream exchange parsers remain unchanged. Operators gain a resettable parse-error counter and bounded diagnostics that do not log exchange payload contents.
GitNexus MCP assessment
GitNexus indexed commit
e5288aaon this branch and mapped the parser throughstart_message_loopto connect/reconnect coordination. Its pre-change impact report rated the parser change LOW risk (2 direct dependents, 5 symbols total, one module). Change detection rated the hardening diff LOW risk with no affected indexed execution process.Validation
cargo fmt --all -- --checkgit diff --checkcargo test -p ccxt-core --lib ws_client::tests --all-features -- --test-threads=1(14 passed)cargo test -p ccxt-core --test ws_property_tests --all-features(10 passed)cargo check --workspace --all-featurescargo clippy -p ccxt-core --lib --all-features(changed file clean; 9 pre-existing warnings elsewhere)The existing reconnect integration suite has 7 passing tests and one unrelated baseline mismatch:
WsConfig::default().pong_timeoutis 30 seconds while the test expects 90 seconds. The full core suite also contains the pre-existingcircuit_breaker::tests::test_circuit_breaker_with_eventsindefinite wait noted earlier.