Skip to content

[Cache] boost perf by wrapping keys validity checks with assert() #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 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
Mar 9, 2021

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
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 identified during dev.

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

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

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

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

@nicolas-grekas nicolas-grekas merged commit 1f65e78 into symfony:5.x Mar 9, 2021
@nicolas-grekas nicolas-grekas deleted the cache-assert branch March 9, 2021 12:07
@gggeek
Copy link

gggeek commented Mar 14, 2021

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
Member Author

@gggeek I'd be fine throwing a TypeError 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 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.

8 participants