Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Clear IpUtils cache to prevent memory leaks#50064
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedApr 20, 2023
Can you run some bench comparing with/without LRU, also maybe without cache at all? |
danielburger1337 commentedApr 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just a a quick reminder why the cache was implemented: IPv6 checking isextremely slow (in comparison to IPv4). PHP 8.2.4 Average after 3 runs each and 1 million IP addresses / cache entries:
Other Tests:
An option to improve a tiny bit would be to only cache IPv6 results since IPv4 checking seems to be a little bit faster thanANY caching. <?phprequire'./vendor/autoload.php';useSymfony\Component\HttpFoundation\IpUtils;$start =microtime(true);for ($i =0;$i <1_000_000; ++$i) { IpUtils::checkIp4(random_int(1,254).random_int(0,254).random_int(0,254).random_int(0,255),'192.168.1.1');// IpUtils::checkIp6('::' . $i, 'fe80::/65');}$end =microtime(true);var_dump($end -$start,get_peak_memory_usage()); When only checking about 100 IP addresses the difference becomes basically indistinguishable:
|
nicolas-grekas commentedApr 20, 2023
let's do this yes! (with an inline comment reminding us about this) |
nicolas-grekas commentedApr 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The difference is not that big actually. |
nicolas-grekas commentedApr 20, 2023
The cache has been introduced in#23098 |
stof commentedApr 20, 2023
the benchmark being done seems to be testing the performance in case of cache misses though. the cache was added to improve performance for the common case of checking the trusted ips many times, which will do cache hits for most checks. |
nicolas-grekas left a comment
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.
@stof is right, let's keep the cache :)
nicolas-grekas commentedApr 20, 2023
Thank you@danielburger1337. |
Fixes#50032
IpUtils cache now uses LRU principal