Cleanup of entries should not be skipped when using default settings #536
Conversation
By default (apc.smart = 0), apc_cache_default_expunge() should not skip cleanup of old entries. This may cause insertion of new entries to fail unnecessarily, which is unexpected behavior.
|
@LionsAd Just to be sure, since you mentioned The main purpose of this PR is to remove the skip that can occur, when I think this skip was intended as some kind of optimization, but when the expunge() hook is called, in the vast majority of cases this skip does not occur. So it optimizes almost nothing. But when it occurs, it causes unexpected behavior (PHP functions like apcu_store() are not able to store the new entry). That's why I think it makes sense to remove this skip. |
|
Yes, that is what I meant that with the old smart = 1 behavior, you have the same behavior. And I agree that it’s better to purge some items from time to time than to completely run out of memory. |
|
To check my understanding, this is only about the case where you're trying to store something that's larger than half the entire cache size? |
| apc_cache_wlocked_remove_entry(cache, entry); | ||
| continue; | ||
| } | ||
| if (cache->smart > 0L && apc_sma_get_avail_mem(cache->sma) >= (size_t) (cache->smart * size)) { |
There was a problem hiding this comment.
Does this even do anything useful? We're only going to call expunge if there is not enough space for size, right?
There was a problem hiding this comment.
Ah no, this is checking for total free size, not available single block. But then doesn't setting smart > 0 still result in expunge being skipped sometimes even though it may have been possible to free up consecutive space?
There was a problem hiding this comment.
Yes, it does and I'd like to change how smart works to address this issue as well. I made a suggestion how smart could be changed in #533. Then I opened this pr (#536) because i wasn't sure if it is a good idea to change the behavior of smart. But finally i think we should do it, because the current implementation seems to be not very useful and will cause unexpected behavior.
If you think it makes sense to change the behavior of smart, I could make the entire change (including smart) here or open a new PR for it. Which do you prefer?
Okay, I think I misunderstood, and what this is trying to fix is a high-fragmentation case where half the cache is empty, but not enough space for a consecutive allocation? |
|
While I am not the maintainer, my 2c is that this change is independently useful and the smart == 1 behavior could be tweaked in a follow-up. |
|
Friendly ping. |
|
I went ahead and merged this for now. I'm not particularly happy with the smart behavior here, but TBH I don't know what the correct behavior is even supposed to be there. I'm also somewhat concerned about fully expunging the cache in high fragmentation scenarios where there is still a lot of free space. I think that ideally, we'd make the persistence representation position-independent (via pointer unswizzling), which would then allow relocating the cache entries to reduce fragmentation. This would be a large change though. Another option to consider would be to change the allocator to reduce fragmentation (via size binning?) The current implementation is naive. |
By default (apc.smart = 0), apc_cache_default_expunge() should not skip cleanup of old entries. This may cause insertion of new entries to fail unnecessarily, which is unexpected behavior.