configurable apcu compression level#96
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a guarded module global and INI for APCu compression level; APCu serializer reads and validates the configured level (falls back to default) and uses it for compression; module/global wiring and tests updated to exercise 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 Possibly related PRsPoem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)zstd.czstd.c:28:10: fatal error: 'php.h' file not found ... [truncated 635 characters] ... ystem" 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
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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
php_zstd.h (1)
60-60: ⚡ Quick winClarify the comment per maintainer feedback.
The maintainer previously suggested using a clearer comment like
"dummy to avoid empty structures"to better convey the intent of this field.📝 Suggested comment improvement
- int unused_dummy; /* no empty struct */ + int unused_dummy; /* dummy to avoid empty structures */🤖 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 `@php_zstd.h` at line 60, Update the comment for the struct field unused_dummy to clearly state its purpose: replace the vague "no empty struct" with a concise explanatory comment such as "dummy to avoid empty structures" so readers understand this field exists solely to prevent an empty struct; locate the field named unused_dummy in php_zstd.h and change only the comment text accordingly.
🤖 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.
Nitpick comments:
In `@php_zstd.h`:
- Line 60: Update the comment for the struct field unused_dummy to clearly state
its purpose: replace the vague "no empty struct" with a concise explanatory
comment such as "dummy to avoid empty structures" so readers understand this
field exists solely to prevent an empty struct; locate the field named
unused_dummy in php_zstd.h and change only the comment text accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec40df9a-994c-49cc-aa78-b8c5eb1aaf77
📒 Files selected for processing (3)
php_zstd.htests/apcu_serializer.phptzstd.c
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/apcu_serializer.phpt
- zstd.c
|
Although this has not been verified in CI, when using ZTS with PHP 8.0 or earlier and APCu enabled, the |
|
Would this fix it? |
|
We also need to check the following |
|
Sure, I'll do several ZTS builds of PHP 7 and 8 and test with those too. |
|
Yep, this coredumps at least with 7.4-zts. I believe, I've got a fix for it already, but I'll do some additional tests first. |
|
ran some additional tests with: php74 |
|
Thanks. |
2 participants
New ini setting "zstd.apcu_compression_level"
Summary by CodeRabbit
New Features
Tests