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] boost perf by wrapping keys validity checks withassert()#40317

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:5.xfromnicolas-grekas:cache-assert
Mar 9, 2021

Conversation

@nicolas-grekas
Copy link
Member

QA
Branch?5.x
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

PSR-6 has one perf hog: checking the validity of keys.

But in practice, an invalid key should never happen in production: encoding/cleanup is a must-have, and it's a step that should be identifiedduring dev.

That's why I think we're safe wrapping these checks withassert().

On anArrayAdapter, this doubles the throughput of the pool when getting items.

I didn't useassert() in constructors when not on the hot path.

This PR also makes some callable properties static, as they should be from the beginning.

kaznovac reacted with thumbs up emojikaznovac reacted with rocket emoji
@nicolas-grekasnicolas-grekas merged commit1f65e78 intosymfony:5.xMar 9, 2021
@nicolas-grekasnicolas-grekas deleted the cache-assert branchMarch 9, 2021 12:07
@gggeek
Copy link

gggeek commentedMar 14, 2021
edited
Loading

While I applaud perf increases, I have seen my fair share real life cases of caching systems which gave errors when NULL cache keys were being generated. The error was iirc thrown by the memcached adapter, and quite a pain to troubleshoot, as it carried no info whatsoever as to the offending bit of code.
Of course, the data sets used in dev or for ci and testing never had any data that would trigger the same problem, making it hard to spot before going live / using real data.
In the end, I think we had to resort to modifying the bit of code which made the memcache call, adding in there an is_null check and logging the whole stack trace, just to get started on troubleshooting.
If I get it correctly, It seems that this commit is bringing back that possible problem.
Could we have best of both worlds?

@nicolas-grekas
Copy link
MemberAuthor

@gggeek I'd be fine throwing aTypeError when null is passed since that's the planned behavior for psr/cache v2 with PHP8 - aka what we'll do in Symfony 6.

Would you mind sending a PR doing so ?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@jderussejderussejderusse approved these changes

@derrabusderrabusderrabus approved these changes

+1 more reviewer

@TobionTobionTobion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

8 participants

@nicolas-grekas@gggeek@stof@jderusse@Tobion@derrabus@javiereguiluz@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp