Add incremental zstd_(un)compress_init() and zstd_(un)compress_add() #79
Conversation
|
Warning Rate limit exceeded@thekid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce incremental compression and decompression capabilities to the PHP Zstd extension. This is achieved by adding a new internal Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PHP_Extension
participant ZstdContext
User->>PHP_Extension: zstd_compress_init(level)
PHP_Extension->>ZstdContext: Create compression context
PHP_Extension-->>User: ZstdContext object
User->>PHP_Extension: zstd_compress_add(ZstdContext, data, end)
PHP_Extension->>ZstdContext: Feed data chunk
ZstdContext-->>PHP_Extension: Compressed chunk
PHP_Extension-->>User: Compressed data
User->>PHP_Extension: zstd_uncompress_init()
PHP_Extension->>ZstdContext: Create decompression context
PHP_Extension-->>User: ZstdContext object
User->>PHP_Extension: zstd_uncompress_add(ZstdContext, data)
PHP_Extension->>ZstdContext: Feed compressed chunk
ZstdContext-->>PHP_Extension: Decompressed chunk
PHP_Extension-->>User: Decompressed data
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
tests/inc.phpt (1)
1-28: LGTM! Good basic test for the incremental APIThis test effectively validates the core functionality of the incremental compression and decompression API. It correctly tests:
- Initializing contexts
- Adding data incrementally
- Finalizing compression
- Verifying data integrity through decompression
If the API provides a way to explicitly close or free the context resources, consider adding that to the test to demonstrate proper resource management.
tests/inc_decomp.phpt (2)
5-5: Consider adding a comment explaining the included dataThe test includes external data, but there's no indication of what data.inc contains.
-include(dirname(__FILE__) . '/data.inc'); +// Include test data - contains a sample string for compression testing +include(dirname(__FILE__) . '/data.inc');
9-23: LGTM! Thorough testing of incremental decompressionThis test effectively verifies that incremental decompression works correctly with various chunk sizes. Good job on testing with multiple chunk sizes and verifying data integrity.
Consider adding a test with a very small chunk size (e.g., 1 byte) to ensure the decompression context can handle extreme chunking.
tests/inc_comp.phpt (1)
5-5: Consider adding a comment explaining the included dataThe test includes external data, but there's no indication of what data.inc contains.
-include(dirname(__FILE__) . '/data.inc'); +// Include test data - contains a sample string for compression testing +include(dirname(__FILE__) . '/data.inc');README.md (3)
107-111: Fix typos in newly-listed API names
zstd_compress_init/zstd_uncompress_initaccidentally lost the second underscore before init.
This can be confusing for users who copy/paste the README examples and will also break Markdown autolinking.-* zstd\_compress_\init — Initialize an incremental compress context -* zstd\_uncompress_\init — Initialize an incremental uncompress context +* zstd\_compress\_init — Initialize an incremental compress context +* zstd\_uncompress\_init — Initialize an incremental uncompress context
229-235: Minor wording – missing prepositionThe sentence reads “Returns a zstd context instance success”.
Add the preposition on for correct grammar.-Returns a zstd context instance success, or FALSE on failure +Returns a zstd context instance on success, or FALSE on failure
315-318: Synchronise namespace-alias list with corrected namesThe namespace alias list still contains the single-underscore variants; adjust for consistency.
-`zstd_uncompress_init` and `zstd_uncompress_add` function alias. +`zstd_uncompress_init` and `zstd_uncompress_add` function aliases.(And ensure the function names contain both underscores.)
🧰 Tools
🪛 LanguageTool
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ... $context, $data ) ```zstd_compress, `zstd_uncompress`, `zstd_compress_dict`...(UNLIKELY_OPENING_PUNCTUATION)
zstd.stub.php (2)
13-20: Declare theZstdContextclass in stubs to keep static analysers happyTools like Psalm / PHPStan will raise “unknown type” warnings for the
ZstdContextparameter & return types.+/** @generated-class */ +final class ZstdContext {}Add this (preferably above the global‐namespace functions) so that IDEs recognise the symbol.
33-40: Uniform default-parameter spacingTiny consistency nit – other stubs use
int $level = 3(space on both sides of=).-function compress_init(int $level= 3): ZstdContext|false {} +function compress_init(int $level = 3): ZstdContext|false {}Apply to all four signatures for visual consistency.
zstd.c (2)
118-126: Zero-initialisation ofintern->ctxis currently implicit
zend_object_alloc()zeroes the memory, sointern->ctxstarts asNULL, but adding an explicit assignment improves readability and guards against future refactors that might switch allocation helpers.object_properties_init(&intern->zo, class_type); + intern->ctx = NULL;
548-563: Return value size can explode for very large inputs
smart_string_appendl()reallocates aggressively; for huge streams a singlezstd_compress_add()could allocate multi-GB buffers.Consider limiting chunk size or streaming directly to a zend_string with incremental reallocation, similar to the pattern in
php_zstd_output_handler_write(), to cap peak memory usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(3 hunks)tests/inc.phpt(1 hunks)tests/inc_comp.phpt(1 hunks)tests/inc_decomp.phpt(1 hunks)tests/inc_ns.phpt(1 hunks)zstd.c(7 hunks)zstd.stub.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
zstd.c (1)
zstd.stub.php (8)
zstd_compress_init(13-13)zstd_compress_add(15-15)zstd_uncompress_init(17-17)zstd_uncompress_add(19-19)compress_init(33-33)compress_add(35-35)uncompress_init(37-37)uncompress_add(39-39)
zstd.stub.php (1)
zstd.c (4)
zstd_compress_init(491-523)zstd_compress_add(525-566)zstd_uncompress_init(568-588)zstd_uncompress_add(590-634)
🪛 LanguageTool
README.md
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ... $context, $data ) ``` zstd_compress, `zstd_uncompress`, `zstd_compress_dict`...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (3)
tests/inc_ns.phpt (1)
1-28: LGTM! Good verification of namespace versionsThis test properly validates that the namespace versions of the incremental API functions work correctly.
tests/inc_comp.phpt (2)
7-23: LGTM! Well-structured incremental compression testThis test thoroughly validates the incremental compression functionality with different chunk sizes. Good job on verifying:
- Proper context initialization
- Chunk-by-chunk compression
- Compression finalization
- Compression efficiency (compressed size < original size)
- Data integrity through decompression
19-19: Good explicit finalization of the compression streamCorrectly demonstrates finalizing the compression by adding an empty chunk with the end flag set to true.
| ZSTD_inBuffer in = { in_buf, in_size, 0 }; | ||
| size_t res = 1; | ||
| const size_t grow = ZSTD_DStreamOutSize(); | ||
|
|
||
| while (in.pos < in.size && res > 0) { | ||
| if (ctx->output.pos == ctx->output.size) { | ||
| ctx->output.size += grow; | ||
| ctx->output.dst = erealloc(ctx->output.dst, ctx->output.size); | ||
| } | ||
|
|
||
| ctx->output.pos = 0; | ||
| res = ZSTD_decompressStream(ctx->dctx, &ctx->output, &in); | ||
| if (ZSTD_isError(res)) { | ||
| php_error_docref(NULL, E_WARNING, | ||
| "libzstd error %s\n", ZSTD_getErrorName(res)); | ||
| smart_string_free(&out); | ||
| RETURN_FALSE; | ||
| } | ||
|
|
||
| smart_string_appendl(&out, ctx->output.dst, ctx->output.pos); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential unbounded growth of ctx->output.dst
zstd_uncompress_add() keeps growing ctx->output.dst with erealloc() but never shrinks it.
A long-lived context processing a single large packet followed by many tiny packets will hold on to the peak allocation.
A simple mitigation is to cap the buffer at, say, twice the maximum chunk size or free & re-malloc when it exceeds a threshold.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(3 hunks)zstd.c(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zstd.c
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ... $context, $data ) ``` zstd_compress, `zstd_uncompress`, `zstd_compress_dict`...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (4)
README.md (4)
107-110: Functions list updated correctly.
The incremental API functions (zstd_compress_init,zstd_compress_add,zstd_uncompress_init,zstd_uncompress_add) are now listed alongside the existing one-shot functions as expected.
236-261: Docs forzstd_compress_addare clear.
The signature, description, parameters, and return values accurately reflect the implementation.
276-297: Docs forzstd_uncompress_addlook good.
Signature and descriptions match the implementation.
315-318: Alias summary is accurate.
The list of function aliases is correctly enumerated.🧰 Tools
🪛 LanguageTool
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ... $context, $data ) ```zstd_compress, `zstd_uncompress`, `zstd_compress_dict`...(UNLIKELY_OPENING_PUNCTUATION)
| function compress_init ( [ $level = 3 ] ) | ||
| function compress_add ( $context, $data [, $end = false ] ) | ||
| function uncompress_init () | ||
| function uncompress_add ( $context, $data ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align namespace alias default parameter.
The namespaced compress_init alias currently defaults to 3, but the core docs and implementation use ZSTD_COMPRESS_LEVEL_DEFAULT. For consistency, reference the constant here as well.
- function compress_init ( [ $level = 3 ] )
+ function compress_init ( [ $level = ZSTD_COMPRESS_LEVEL_DEFAULT ] )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function compress_init ( [ $level = 3 ] ) | |
| function compress_add ( $context, $data [, $end = false ] ) | |
| function uncompress_init () | |
| function uncompress_add ( $context, $data ) | |
| function compress_init ( [ $level = ZSTD_COMPRESS_LEVEL_DEFAULT ] ) | |
| function compress_add ( $context, $data [, $end = false ] ) | |
| function uncompress_init () | |
| function uncompress_add ( $context, $data ) |
| ### zstd\_compress\_init — Initialize an incremental compress context | ||
|
|
||
| #### Description | ||
|
|
||
| resource **zstd\_compress\_init** ( [ int _$level_ = ZSTD_COMPRESS_LEVEL_DEFAULT ] ) | ||
|
|
||
| Initialize an incremental compress context | ||
|
|
There was a problem hiding this comment.
Update return type and wording for zstd_compress_init.
The docs still say resource **zstd_compress_init**, but the implementation now returns a ZstdContext object—not a resource. Also consider using the noun “compression” instead of the verb “compress.”
Apply this diff:
- resource **zstd_compress_init** ( [ int _$level_ = ZSTD_COMPRESS_LEVEL_DEFAULT ] )
- Initialize an incremental compress context
+ ZstdContext **zstd_compress_init** ( [ int _$level_ = ZSTD_COMPRESS_LEVEL_DEFAULT ] )
+ Initialize an incremental compression context📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### zstd\_compress\_init — Initialize an incremental compress context | |
| #### Description | |
| resource **zstd\_compress\_init** ( [ int _$level_ = ZSTD_COMPRESS_LEVEL_DEFAULT ] ) | |
| Initialize an incremental compress context | |
| ### zstd\_compress\_init — Initialize an incremental compress context | |
| #### Description | |
| ZstdContext **zstd\_compress\_init** ( [ int _$level_ = ZSTD_COMPRESS_LEVEL_DEFAULT ] ) | |
| Initialize an incremental compression context |
| ### zstd\_uncompress\_init — Initialize an incremental uncompress context | ||
|
|
||
| #### Description | ||
|
|
||
| resource **zstd\_uncompress\_init** ( void ) | ||
|
|
||
| Initialize an incremental uncompress context | ||
|
|
||
| #### Return Values | ||
|
|
||
| Returns a zstd context instance on success, or FALSE on failure | ||
|
|
There was a problem hiding this comment.
Update return type for zstd_uncompress_init.
Similar to zstd_compress_init, this function returns a ZstdContext object. Update the docs accordingly:
- resource **zstd_uncompress_init** ( void )
- Initialize an incremental uncompress context
+ ZstdContext **zstd_uncompress_init** ( void )
+ Initialize an incremental uncompress context📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### zstd\_uncompress\_init — Initialize an incremental uncompress context | |
| #### Description | |
| resource **zstd\_uncompress\_init** ( void ) | |
| Initialize an incremental uncompress context | |
| #### Return Values | |
| Returns a zstd context instance on success, or FALSE on failure | |
| ### zstd\_uncompress\_init — Initialize an incremental uncompress context | |
| #### Description | |
| ZstdContext **zstd_uncompress_init** ( void ) | |
| Initialize an incremental uncompress context | |
| #### Return Values | |
| Returns a zstd context instance on success, or FALSE on failure |
Implements feature suggested in #64
ZstdContextclass rather than resources like in Add incremental zstd_(un)compress_init() and zstd_(un)compress_add() #77, see https://wiki.php.net/rfc/resource_to_object_conversion)Example
Summary by CodeRabbit
New Features
Documentation
Tests