- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add compression support for PHP Sessions #2473
Add compression support for PHP Sessions #2473
Conversation
Previously, compression was available for standard data but not for session handling. This update enables the compression of PHP sessions, allowing for more efficient Redis memory usage.
The failed checks result from an igbinary issue on macOS, not related to this commit |
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.
This looks great.
I suppose you could also abstract the redis_compress
and redis_uncompress
logic that is each done exactly the same way twice into a helper that would return nonzero if the resulting value needs to be freed, but that's a minor nitpick
Thank you for your review and suggestions. Could you please check if everything is now fine or if further improvements are needed? |
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.
Sorry about the ZEND_STRL
Windows issue, I knew it confused Windows but had forgotten it in my old age. 😄
If you update the compress/uncompress helpers to always set the out args I'll get this merged.
I'm also happy to write a test for it. The session tests are a bit involved.
redis_session.c
Outdated
if (session_uncompress_data(redis_sock, resp, resp_len, &compressed_buf, &compressed_len)) { // Try uncompress | ||
*val = zend_string_init(compressed_buf, compressed_len, 0); | ||
efree(compressed_buf); // Free the buffer allocated by redis_uncompress | ||
} else { // Return raw data | ||
*val = zend_string_init(resp, resp_len, 0); | ||
} |
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.
If you change session_compress_data
to always set the out arguments and return whether you need to free the char * you can eliminate this if/else and just always use compressed_buf
and then conditionally free it based on the return value from session_compress_data
.
Thank you for the advice. It should be better now. |
fa44c7a
to
be8a7be
Compare
Merged, thanks! |
Previously, compression was available for standard data but not for session handling. This update enables the compression of PHP sessions, allowing for more efficient Redis memory usage.