Adds initializers to support parsing JSON directly from a Span.#107
Adds initializers to support parsing JSON directly from a Span.#107tayloraswift merged 3 commits intorarestype:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the JSON parsing capabilities by introducing initializers that allow direct parsing from memory spans. This change is designed to optimize performance by eliminating the need to copy JSON buffer data, thereby reducing memory allocations and improving processing efficiency, particularly for large JSON payloads. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces useful initializers for parsing JSON directly from Span and RawSpan types, aiming to improve performance by avoiding unnecessary buffer copies. No specific security vulnerabilities were found in the changes. However, the implementation could benefit from improved code maintainability by reducing duplication. The newly added initializers in the modified files share a similar structure, and I've provided suggestions to refactor this repeated logic into private generic helper methods within each extension for conciseness and easier maintenance.
| /// Attempts to parse a JSON array from a span | ||
| public init(parsing span: Span<UInt8>) throws { | ||
| self.init( | ||
| try span.withUnsafeBufferPointer { buffer in | ||
| try JSON.NodeRule<Int>.Array.parse(buffer) | ||
| } | ||
| ) | ||
| } | ||
| /// Attempts to parse a JSON array from a raw span | ||
| public init(parsing span: RawSpan) throws { | ||
| self.init( | ||
| try span.withUnsafeBytes { buffer in | ||
| try JSON.NodeRule<Int>.Array.parse(buffer) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
The new initializers for Span<UInt8> and RawSpan contain duplicated logic. You can reduce this by extracting the parsing logic into a private, generic, static method. This helper can then be passed directly to withUnsafeBufferPointer and withUnsafeBytes, making the code more concise and maintainable.
/// Attempts to parse a JSON array from a span
public init(parsing span: Span<UInt8>) throws {
self.init(try span.withUnsafeBufferPointer(Self._parse(from:)))
}
/// Attempts to parse a JSON array from a raw span
public init(parsing span: RawSpan) throws {
self.init(try span.withUnsafeBytes(Self._parse(from:)))
}
private static func _parse<C>(from buffer: C) throws -> [JSON.Node]
where C: Collection, C.Element == UInt8, C.Index == Int
{
try JSON.NodeRule<Int>.Array.parse(buffer)
}| /// ``Object``) from a span. | ||
| public init(parsing span: Span<UInt8>) throws { | ||
| self = try span.withUnsafeBufferPointer { buffer in | ||
| try JSON.RootRule<Int>.parse(buffer) | ||
| } | ||
| } | ||
| /// Attempts to parse a complete JSON message (either an ``Array`` or an | ||
| /// ``Object``) from a raw span. | ||
| public init(parsing span: RawSpan) throws { | ||
| self = try span.withUnsafeBytes { buffer in | ||
| try JSON.RootRule<Int>.parse(buffer) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new initializers for Span<UInt8> and RawSpan contain duplicated logic. You can reduce this by extracting the parsing logic into a private, generic, static method. This helper can then be passed directly to withUnsafeBufferPointer and withUnsafeBytes, making the code more concise and maintainable.
| /// ``Object``) from a span. | |
| public init(parsing span: Span<UInt8>) throws { | |
| self = try span.withUnsafeBufferPointer { buffer in | |
| try JSON.RootRule<Int>.parse(buffer) | |
| } | |
| } | |
| /// Attempts to parse a complete JSON message (either an ``Array`` or an | |
| /// ``Object``) from a raw span. | |
| public init(parsing span: RawSpan) throws { | |
| self = try span.withUnsafeBytes { buffer in | |
| try JSON.RootRule<Int>.parse(buffer) | |
| } | |
| } | |
| /// ``Object``) from a span. | |
| public init(parsing span: Span<UInt8>) throws { | |
| self = try span.withUnsafeBufferPointer(Self._parse(from:)) | |
| } | |
| /// Attempts to parse a complete JSON message (either an ``Array`` or an | |
| /// ``Object``) from a raw span. | |
| public init(parsing span: RawSpan) throws { | |
| self = try span.withUnsafeBytes(Self._parse(from:)) | |
| } | |
| private static func _parse<C>(from buffer: C) throws -> JSON.Node | |
| where C: Collection, C.Element == UInt8, C.Index == Int | |
| { | |
| try JSON.RootRule<Int>.parse(buffer) | |
| } |
| /// Attempts to parse a JSON object from a span. | ||
| public init(parsing span: Span<UInt8>) throws { | ||
| self.init( | ||
| try span.withUnsafeBufferPointer { buffer in | ||
| try JSON.NodeRule<Int>.Object.parse(buffer) | ||
| } | ||
| ) | ||
| } | ||
| /// Attempts to parse a JSON object from a raw span. | ||
| public init(parsing span: RawSpan) throws { | ||
| self.init( | ||
| try span.withUnsafeBytes { buffer in | ||
| try JSON.NodeRule<Int>.Object.parse(buffer) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
The new initializers for Span<UInt8> and RawSpan contain duplicated logic. You can reduce this by extracting the parsing logic into a private, generic, static method. This helper can then be passed directly to withUnsafeBufferPointer and withUnsafeBytes, making the code more concise and maintainable.
/// Attempts to parse a JSON object from a span.
public init(parsing span: Span<UInt8>) throws {
self.init(try span.withUnsafeBufferPointer(Self._parse(from:)))
}
/// Attempts to parse a JSON object from a raw span.
public init(parsing span: RawSpan) throws {
self.init(try span.withUnsafeBytes(Self._parse(from:)))
}
private static func _parse<C>(from buffer: C) throws -> [(key: JSON.Key, value: JSON.Node)]
where C: Collection, C.Element == UInt8, C.Index == Int
{
try JSON.NodeRule<Int>.Object.parse(buffer)
}
|
should we limit this to just |
Yes, we should. Done! |
Adds initializers to support parsing JSON directly from a Span.
2 participants
Hi @tayloraswift, I found those additions to be quite useful, to avoid unnecessary copies of the JSON buffer to be parsed. Would you want to take them? Currently they need to use the unsafe APIs because the parser requires
Collectionwhich span does not provide.