Skip to content

fix: compression edge case #255

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

Merged

Conversation

dictcp
Copy link
Contributor

@dictcp dictcp commented Jun 10, 2016

the condition to do compression is incorrent since previous refactoring. according to docs

https://secure.php.net/manual/en/memcached.configuration.php#ini.memcached.compression-factor

Store compressed if: plain_len > comp_len * factor

https://github.com/php-memcached-dev/php-memcached/blob/master/memcached.ini#L93

store compressed if: plain_len > comp_len * factor

hence, the condition (to early exit, without compression) should be
if (ZSTR_LEN(payload) < (compressed_size * compression_factor))


PR includes

  • fix to the compression_factor formula (whether to compress or not),
  • correctly unset MEMC_VAL_COMPRESSION_{FASTLZ,ZLIB} flags flags properly when plain-text is sent

@dick9gag
Copy link

dick9gag commented Jul 9, 2016

@krakjoe @mkoppanen

could you help to review the PR and accept it if it looks good to you?

it fixes some problem due to incorrect compression flag set

@sodabrew
Copy link
Contributor

@dictcp Could I ask you to rebase this against master, as the php7 branch has been merged?

@dictcp dictcp force-pushed the fix-compress-condition-flags branch from 2eaee8b to 2b860f2 Compare January 24, 2017 02:58
@dictcp dictcp changed the base branch from php7 to master January 24, 2017 02:59
@dictcp dictcp force-pushed the fix-compress-condition-flags branch from 2b860f2 to 1b8d4f7 Compare January 24, 2017 03:58
@dictcp dictcp force-pushed the fix-compress-condition-flags branch from 4a79ba6 to 3a2e9a1 Compare January 24, 2017 05:42
@dictcp
Copy link
Contributor Author

dictcp commented Jan 24, 2017

@sodabrew rebased and target to merge to master branch.

@sodabrew
Copy link
Contributor

Thanks for the rebase! The doc link says

store uncompressed if plain_len > comp_len * factor

But the code is:
if (ZSTR_LEN(payload) < (compressed_size * MEMC_G(compression_factor)))

One of those two expressions has the comparison operator flipped!?

@dictcp
Copy link
Contributor Author

dictcp commented Jan 24, 2017

it should be

Store compressed if: plain_len > comp_len * factor

https://secure.php.net/manual/en/memcached.configuration.php#ini.memcached.compression-factor

store compressed if: plain_len > comp_len * factor

https://github.com/php-memcached-dev/php-memcached/blob/master/memcached.ini#L93

hence, the condition (to early exit, without compression) should be
if (ZSTR_LEN(payload) < (compressed_size * compression_factor))

@sodabrew
Copy link
Contributor

Nit: should your change be payload <= compressed * factor?

Could I bug you to update the docs in this PR as you've described?

@sodabrew sodabrew modified the milestone: 3.0.1 Jan 24, 2017
@dictcp
Copy link
Contributor Author

dictcp commented Jan 24, 2017

@sodabrew push commit and update the description of the PR. sounds good to you?
(no sure what you means by "update the docs", do you mean I should update the ChangeLog, or leaving a good description on on PR sounds good)

@sodabrew sodabrew modified the milestones: 3.0.0, 3.0.1 Jan 24, 2017
@sodabrew sodabrew merged commit 8924e3d into php-memcached-dev:master Jan 24, 2017
@sodabrew
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants