-
Notifications
You must be signed in to change notification settings - Fork 6
perf: use simd json for json parse #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CodSpeed Performance ReportMerging #117 will improve performances by 32.29%Comparing Summary
Benchmarks breakdown
|
27c48c1 to
163f087
Compare
There was a problem hiding this 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 migrates the JSON parsing implementation from serde_json to simd-json for improved performance when parsing package.json files. The changes introduce strongly-typed enums for the type and sideEffects fields and update the file system API to work with Vec<u8> instead of String.
- Replaces
serde_jsonwithsimd-jsonfor JSON parsing with borrowed values - Introduces
ModuleTypeandSideEffectsenums for type-safe field representation - Updates file system reads to use bytes instead of strings
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Adds simd-json and self_cell dependencies |
Cargo.lock |
Updates dependency tree with new and updated packages |
deny.toml |
Adds Zlib license to allowed list |
src/package_json/mod.rs |
Adds ModuleType and SideEffects enum definitions |
src/package_json/simd.rs |
Migrates from serde_json to simd-json with borrowed values |
src/lib.rs |
Updates JSON type references and pattern matching |
src/cache.rs |
Changes file read from string to bytes |
napi/src/lib.rs |
Simplifies module type extraction using new enum |
tests/integration_test.rs |
Updates tests to use new typed fields |
tests/package.json |
Fixes invalid JSON value for sideEffects field |
src/tests/imports_field.rs |
Updates test helpers to use simd-json |
src/tests/exports_field.rs |
Updates test helpers to use simd-json |
Comments suppressed due to low confidence (2)
src/package_json/simd.rs:105
- The method chain is incorrect.
getreturnsOption<&JSONValue>, so.as_str()is being called on the Option instead of the JSONValue. This should be.and_then(|v| v.as_str())to first extract the value from the Option.
src/package_json/simd.rs:316 - Column positions should be counted by character positions, not byte positions. For multi-byte UTF-8 characters, this will produce incorrect column numbers. Change to
col += 1to count characters instead of bytes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9beb665 to
188366b
Compare
188366b to
99cb865
Compare
quininer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adapt simd_json for faster(20~30% improvement) JSON parse and use simd_json value for internal JSON representation.