configurable apcu compression level#96
Conversation
|
Warning Review limit reached
More reviews will be available in 17 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an APCu-facing module global and INI for zstd compression level; APCu serializer reads and validates the setting (falls back on default) and uses it for compression; tests compare APCu stored sizes at different levels. ChangesAPCu Compression Level Configuration
Sequence Diagram(s)sequenceDiagram
participant Test as tests/apcu_serializer.phpt
participant APCuSerializer as APCu_zstd_serializer
participant ModuleGlobals as PHP_ZSTD_G(apcu_compression_level)
participant ZSTD as ZSTD_compress
Test->>APCuSerializer: apcu_store(big_payload)
APCuSerializer->>ModuleGlobals: read apcu_compression_level
APCuSerializer->>APCuSerializer: validate level (zstd_check_compress_level), fallback if invalid/0
APCuSerializer->>ZSTD: ZSTD_compress(data, chosen_level)
ZSTD-->>APCuSerializer: compressed_data
APCuSerializer-->>Test: store result and update apcu_cache_info
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
I think the implementation is good, but I'd also like to see some test code. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/apcu_serializer.phpt`:
- Around line 72-79: The test reads mem_size from
apcu_cache_info()['cache_list'][0] and asserts $b < $a which is flaky; instead,
find the mem_size entry for the stored key "big_entry" from apcu_cache_info()
(search cache_list entries for ['info'] or ['key'] == 'big_entry') and compare
that mem_size before and after setting ini_set('zstd.apcu_compression_level',
19) so you measure the specific item's size, and change the assertion to allow
equality (use $b <= $a) to avoid nondeterministic failures; apply the same fix
to the second occurrence around the later assertion.
- Around line 67-68: Replace the raw file_get_contents(...) expression with a
guarded read: call file_get_contents(...) into a variable (e.g. $payload or
$zstd_source), check if the result === false, and if so print an explicit
failure message to STDERR and exit with non‑zero status (e.g. fwrite(STDERR,
"Failed to read fixture: ...\n"); exit(1);); then use that variable where the
original file_get_contents(...) result was used in the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d988a944-b5df-4c11-84c7-9d38c5431e55
📒 Files selected for processing (3)
php_zstd.htests/apcu_serializer.phptzstd.c
✅ Files skipped from review due to trivial changes (1)
- php_zstd.h
🚧 Files skipped from review as they are similar to previous changes (1)
- zstd.c
| file_get_contents(__DIR__ . '/../zstd.c') | ||
| ]; |
There was a problem hiding this comment.
Fail fast if payload fixture read fails.
If file_get_contents() returns false, this test may pass/fail for the wrong reason. Add a guard and explicit failure output/exit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/apcu_serializer.phpt` around lines 67 - 68, Replace the raw
file_get_contents(...) expression with a guarded read: call
file_get_contents(...) into a variable (e.g. $payload or $zstd_source), check if
the result === false, and if so print an explicit failure message to STDERR and
exit with non‑zero status (e.g. fwrite(STDERR, "Failed to read fixture: ...\n");
exit(1);); then use that variable where the original file_get_contents(...)
result was used in the test.
pavetheway91
force-pushed
the
apcu_level
branch
2 times, most recently
from
5b02610 to
3f1e32d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/apcu_serializer.phpt`:
- Around line 66-71: The test iterates apcu_cache_info()['cache_list'] without
guarding that apcu_cache_info() returned valid metadata, which can raise
warnings and break the test; update the code around the foreach (the call to
apcu_cache_info() and the loop over 'cache_list') to first fetch the result into
a variable, verify it's not false/null and that 'cache_list' exists and is an
array, and if not return null explicitly before attempting to iterate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05daba65-f658-41a1-b8af-dd9f5f45e23e
📒 Files selected for processing (3)
php_zstd.htests/apcu_serializer.phptzstd.c
🚧 Files skipped from review as they are similar to previous changes (2)
- php_zstd.h
- zstd.c
✅ Actions performedReview triggered.
|
✅ Actions performedReviews resumed. |
|
fixed php7 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/apcu_serializer.phpt (1)
87-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the size assertion non-flaky on Line 87.
Using strict
$b < $acan fail nondeterministically when both levels produce equal APCu entry size. Use<=to keep the intent while stabilizing CI.Suggested patch
-if ($a !== null && $b !== null && $b < $a) { +if ($a !== null && $b !== null && $b <= $a) { echo "SMALLER\n"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/apcu_serializer.phpt` around lines 87 - 89, The size comparison using strict inequality currently reads as a potential flaky check (the condition using $b < $a), so update the conditional in the test to use a non-strict comparison ($b <= $a) when evaluating $a and $b to stabilize the assertion that the second size is not greater than the first while leaving the echo "SMALLER\n" behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@php_zstd.h`:
- Around line 47-60: The zend_zstd_globals struct can be empty when
HAVE_APCU_SUPPORT is unset and PHP_VERSION_ID < 80000; ensure the globals struct
always has at least one member by adding a fallback dummy field (e.g., zend_long
dummy or char dummy) inside the ZEND_BEGIN_MODULE_GLOBALS(zstd) block under the
appropriate `#else` branch so zend_zstd_globals is never empty while keeping
ZEND_BEGIN_MODULE_GLOBALS(zstd) and ZEND_DECLARE_MODULE_GLOBALS(zstd) unguarded;
update any conditional compilation around the other members
(output_compression*, ob_handler, handler_registered, compression_coding,
apcu_compression_level) to retain their current guards.
---
Duplicate comments:
In `@tests/apcu_serializer.phpt`:
- Around line 87-89: The size comparison using strict inequality currently reads
as a potential flaky check (the condition using $b < $a), so update the
conditional in the test to use a non-strict comparison ($b <= $a) when
evaluating $a and $b to stabilize the assertion that the second size is not
greater than the first while leaving the echo "SMALLER\n" behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d626bf45-0ef0-44b8-88e6-f063b3ea1ca6
📒 Files selected for processing (3)
php_zstd.htests/apcu_serializer.phptzstd.c
🚧 Files skipped from review as they are similar to previous changes (1)
- zstd.c
2 participants
New ini setting "zstd.apcu_compression_level"
Summary by CodeRabbit