Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Use faster hashing algorithms when possible#52948
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedDec 9, 2023
@javiereguiluz Can you have a look at the tests as they seem to be broken by the changes made here. |
| privatefunctiongetVarName():string | ||
| { | ||
| returnsprintf('__internal_%s',hash('sha256',uniqid(mt_rand(),true),false)); | ||
| returnsprintf('__internal_%s',hash('xxh128',uniqid(mt_rand(),true))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we need a random var name here. A global sequence would be enough to ensure there is no conflict.
Similar to what I did inhttps://github.com/twigphp/Twig/pull/3601/files#diff-47bc8653b21716576958e25b6d7356ecb0f0070f17554d12d2bee985ac211b26R59
mahono commentedDec 10, 2023
Why not xxh3? (just asking for curiosity) |
GromNaN commentedDec 11, 2023
In fact, where the hash is substr, |
javiereguiluz commentedDec 11, 2023
I think it's better to use
|
fabpot commentedDec 17, 2023
Thank you@javiereguiluz. |
… namespace generation to the upgrade guide (mbabker)This PR was merged into the 7.1 branch.Discussion----------[Cache] Add a note about the change in the default cache namespace generation to the upgrade guide| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | no| New feature? | no| Deprecations? | no| Issues | N/A| License | MITAfter deploying the Symfony 7.1 upgrade for a client, we immediately noticed a high rate of cache misses for one of our heavily used controller actions. Digging through the changes for the relevant upstream packages, it looks like this was introduced by way of#52948 and changing the algorithm used for the default cache namespaces when a `namespace` attribute isn't configured on the `cache.pool` service tag.To my knowledge, there is no way to configure a namespace for cache pools using the `framework.cache` configuration, so through the framework configuration, there is no way to avoid hitting this issue without either manually building cache services or getting creative with a compiler pass that runs before the core `CachePoolPass`, so IMO it's best to call out the change in the upgrade guide.Commits-------f536f3c Add a note about the change in the default cache namespace generation to the upgrade guide
Uh oh!
There was an error while loading.Please reload this page.
xxh128is 60 times faster thansha256(source:https://php.watch/versions/8.1/xxHash) but it's not cryptographically secure.In previous versions we already used
xxh128in some places. In this PR I propose to use it in all places where we don't need a cryptographically secure hash.Comments:
I also remove someREVERTEDbase64encoding of binary hashes, etc. It looks convoluted and to me it looks unnecessary. But I can revert these changes if needed.xxh128but I'm not sure, so I didn't. These:#SymfonyHackday