Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
nicolas-grekas merged 1 commit intosymfony:6.3fromdanielburger1337:clear-iputils-cache
Apr 20, 2023
Merged

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

nicolas-grekas merged 1 commit intosymfony:6.3fromdanielburger1337:clear-iputils-cache
Apr 20, 2023

Conversation

@danielburger1337
Copy link
Contributor

QA
Branch?6.3
Bug fix?no
New feature?no
Deprecations?no
TicketsFix#50032
LicenseMIT
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
ContributorAuthor

danielburger1337 commentedApr 20, 2023
edited
Loading

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 isextremely 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 usingcount: 3.593 seconds, 2.097152 mb

  • Keeping count of the items stored and not usingcount (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 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:

  • 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 commentedApr 20, 2023
edited
Loading

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

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-grekasnicolas-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.

danielburger1337 reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas merged commit3761915 intosymfony:6.3Apr 20, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

[HttpFoundation] IpUtils::$checkedIps is never cleared

4 participants

@danielburger1337@nicolas-grekas@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp