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

[Cache] Fix Memory leak#44002

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:4.4froma1812:fix_bug_43918
Nov 12, 2021
Merged

Conversation

@a1812
Copy link
Contributor

@a1812a1812 commentedNov 10, 2021
edited by nicolas-grekas
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#43918
LicenseMIT
Doc PR

Rough decision as an example of how to stop a leak

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@a1812a1812 marked this pull request as ready for reviewNovember 10, 2021 20:55
@carsonbotcarsonbot added this to the5.3 milestoneNov 10, 2021
@nicolas-grekas
Copy link
Member

@Jeroeny can you please try this patch a report back if it fixes your issue?

@Jeroeny
Copy link
Contributor

@Jeroeny can you please try this patch a report back if it fixes your issue?

It no longer seems to leak in the reproducer after having applied these changes. 👍

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 11, 2021
edited
Loading

This property has been added to save callingvalidateKey() and to save hashing long keys.

Since#40317, it's possible to skip callingvalidateKey() so I'm fine removing for that part.
But for hashing, this would still be useful.

This property has been added with the assumption that the number of different keys is upper bounded.
We could enforce this upper bound by limiting the size of theids array instead. Eg resetting it when it reaches 1000 items?

@a1812
Copy link
ContributorAuthor

But for hashing, this would still be useful.
This property has been added with the assumption that the number of different keys is upper bounded. We could enforce this upper bound by limiting the size of theids array instead. Eg resetting it when it reaches 1000 items?

i agree with the idea of 1000 keys, a@Jeroeny ?

@Jeroeny
Copy link
Contributor

But for hashing, this would still be useful.
This property has been added with the assumption that the number of different keys is upper bounded. We could enforce this upper bound by limiting the size of theids array instead. Eg resetting it when it reaches 1000 items?

i agree with the idea of 1000 keys, a@Jeroeny ?

Sounds good to me. Upon reaching the limit, do you want to purge the entire array or slice it so that it's maxed out at 1000 items (like the ArrayAdapter cache) ?

@nicolas-grekas
Copy link
Member

The logic in ArrayAdapter is too involving for this. I'd suggest halving the array instead:
array_splice($this->ids, 0, 500);

Jeroeny reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Thank you@a1812.

@nicolas-grekasnicolas-grekas merged commita8b4e7f intosymfony:4.4Nov 12, 2021
@nicolas-grekasnicolas-grekas modified the milestones:5.3,4.4Nov 12, 2021
@a1812a1812 deleted the fix_bug_43918 branchNovember 12, 2021 10:21
This was referencedNov 14, 2021
This was referencedNov 22, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@chalasrchalasrAwaiting requested review from chalasr

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@wouterjwouterjAwaiting requested review from wouterj

@xabbuhxabbuhAwaiting requested review from xabbuh

@ycerutoycerutoAwaiting requested review from yceruto

+1 more reviewer

@JeroenyJeroenyJeroeny left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@a1812@carsonbot@nicolas-grekas@Jeroeny

[8]ページ先頭

©2009-2025 Movatter.jp