-
-
Notifications
You must be signed in to change notification settings - Fork 14
feat: added option for user-provided buffer, improved type generation #281
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
base: main
Are you sure you want to change the base?
Conversation
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 adds support for user-provided output buffers in decompression functions to improve performance when processing multiple packages sequentially. It also enhances TypeScript type definitions with detailed overloads for better type safety.
- Added
output
field toDecOptions
for user-providedUint8Array
buffers - Updated return types to support returning decompressed length when using provided buffers
- Enhanced TypeScript type definitions with comprehensive function overloads
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/lib.rs | Core implementation of user-provided buffer support and improved type definitions |
benchmark/bench.ts | Added benchmarks to test performance improvements with user-provided buffers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if let Some(ref mut opts) = self.options { | ||
if let Some(ref mut output_buffer) = opts.output { | ||
let decompressed_len = self | ||
.inner |
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.
The use of unsafe
to get a mutable reference to the output buffer is potentially dangerous. Consider using safe alternatives or adding proper safety documentation explaining why this unsafe block is necessary and what invariants must be maintained.
Copilot uses AI. Check for mistakes.
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.
@Brooooooklyn a question from me - I see no other way than using the unsafe in this small part. Would adding:
// SAFETY: We know the buffer is valid for the lifetime of this function
// and we're not extending beyond its bounds
comment be sufficient enough? I might be missing something, as Im pretty new to Rust.
@Brooooooklyn would you mind chiming on the |
Reference: #279
Use case for user-provider Uint8Array is mostly when decompressing a lot of packages one after the other. I also a bit over-used napi-rs type overwrite, to create better return type.
Benchmark table:
The biggest improvement we see from uncompress-sync to sync-alloc-uncompress