Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 20, 2023
Merged

[HttpFoundation] Clear IpUtils cache to prevent memory leaks #50064

merged 1 commit into from
Apr 20, 2023

Conversation

danielburger1337
Copy link
Contributor

Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #50032
License MIT
Doc PR

Fixes #50032

IpUtils cache now uses LRU principal

@nicolas-grekas
Copy link
Member

Can you run some bench comparing with/without LRU, also maybe without cache at all?

@danielburger1337
Copy link
Contributor Author

danielburger1337 commented Apr 20, 2023

Can you run some bench comparing with/without LRU, also maybe without cache at all?

Just a a quick reminder why the cache was implemented: IPv6 checking is extremely slow (in comparison to IPv4).

PHP 8.2.4

Average after 3 runs each and 1 million IP addresses / cache entries:

  • With LRU: 3.778 seconds, 2.097152 mb
  • Max Items Cache: 3.615 seconds, 2.097152 mb
  • Current Cache: 2.833 seconds, 100.663296 mb

Other Tests:

  • Keeping count of the items stored and not using count: 3.593 seconds, 2.097152 mb

  • Keeping count of the items stored and not using count (LRU): 3.653 seconds, 2.097152 mb

  • Without any Cache (IPv4): 2.484 seconds, 2.097152 mb

  • Without any Cache (IPv6): 5.741 seconds, 2.097152 mb

An option to improve a tiny bit would be to only cache IPv6 results since IPv4 checking seems to be a little bit faster than ANY caching.


<?php

require './vendor/autoload.php';

use Symfony\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:

  • With LRU: 0.00061893463134766 seconds, 2.097152 mb
  • Max Items Cache: 0.00063395500183105 seconds, 2.097152 mb
  • Current Cache: 0.00051212310791016 seconds, 2.097152 mb
  • Without any Cache (IPv4): 0.00046300888061523 seconds, 2.097152 mb
  • Without any Cache (IPv6): 0.00075387954711914 seconds, 2.097152 mb

@nicolas-grekas
Copy link
Member

An option to improve a tiny bit would be to only cache IPv6 results since IPv4 checking seems to be a little bit faster than ANY caching.

let's do this yes! (with an inline comment reminding us about this)
let's drop the -v6 suffix while doing so

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 20, 2023

With LRU: 3.778 seconds, 2.097152 mb
Without any Cache (IPv6): 5.741 seconds, 2.097152 mb

The difference is not that big actually.

@nicolas-grekas
Copy link
Member

The cache has been introduced in #23098
What about dropping it altogether? The diff doesn't look much worth the added complexity to me.

@stof
Copy link
Member

stof commented Apr 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.

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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
Copy link
Member

Thank you @danielburger1337.

@nicolas-grekas nicolas-grekas merged commit 3761915 into symfony:6.3 Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HttpFoundation] IpUtils::$checkedIps is never cleared
4 participants