Fix apc_cache_default_expunge() to always remove expired entries

[madmajestro]

When apc_cache_default_expunge() is called, it now always cleans up expired entries and doesn't skip cleanup anymore, as it is only called when there is not enough memory available to store a cache entry and cleanup is really needed. The skip can happen because the function apc_sma_get_avail_mem() doesn't take into account that a block of contiguous memory is needed. In situations with huge cache fragmentation, the old behavior can cause apcu to not accept new entries anymore which don't fit into a fragment of available memory. This causes php-functions like apcu_store() to not being able to insert bigger entries, until the cache has filled up enough with small entries to trigger the cleanup. This commit fixes the issue and mostly implements a behavior that should have already been implemented previously according to the header file. This also applies to the smart configuration setting that is now used to tune the probability of a full cache wipe.

[madmajestro]

@nikic:
With #531, #532 and #533 in place, the cache expiration / cache wipe mechanism is much more reliable than before and it now finally works as expected.

Is it therefore possible to make a new release with this fixes during the next days? It would help me a lot if it gets packaged by Ondřej Surý as an official Debian package and by the way it would be really nice to get this fixes in the next Debian release too.

The improvements are:

• Cache entries with TTL now expire correctly when apc.ttl = 0 (default). In use cases that primarily use entries with short TTL times, this significantly reduces the probability of full cache wipes. Try to cleanup cache entries with explicit ttl when apc.ttl is 0 #531

• If removing expired entries did not free enough contiguous memory, a cache wipe was needed but it was incorrectly not performed. This now works reliable. Fix apc_sma_get_avail_size() to look for a contiguous block of memory to make expunge more reliable #532

• Cache cleanup is no longer skipped when needed and the smart configuration setting can now be used to tune the probability of a full cache wipe. Fix apc_cache_default_expunge() to always remove expired entries #533

Thank you very much for the reviews and your effort

Arndt

[madmajestro]

... and can you give me a hint / link on how I can publish documentation for the "smart" configuration parameter (e.g. on php.net)?

[madmajestro]

I just saw that i didn't explain the exact reason why the faulty skip can happen: The skip can happen because the function apc_sma_get_avail_mem() doesn't take into account that a block of contiguous memory is needed. But it doesn't make sense to fix apc_sma_get_avail_mem(), because it is used in other code paths and the needed check is already done in the sma layer in a correct way (which leads to the call of the expunge function). So it is a buggy double-check, which is removed by this pr. The commit text has been slightly adjusted to reflect this.

[madmajestro]

I added two tests that demonstrate the unexpected behavior on the master branch (which is solved by this pr). Any chance to get a review for this? Please let me know if I'm wrong or if I can do better.

[mszabo-wikia]

@madmajestro Thanks for this! Regarding your question about updating docs, they live at https://github.com/php/doc-en/tree/master/reference/apcu.

[madmajestro]

@mszabo-wikia Thanks for your reply! I will take a look at it.

[madmajestro]

I just pushed #536 which is less verbose and solves the same problem when using the default settings. It also preserves the current behavior of the "smart" setting, which could be changed in another PR. Maybe that's a better approach. If #536 is accepted, I'll close this PR.

[madmajestro]

superseded by #536 and #537