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

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

Merged
fabpot merged 1 commit intosymfony:7.1fromjaviereguiluz:faster_hash
Dec 17, 2023

Conversation

@javiereguiluz
Copy link
Member

@javiereguiluzjaviereguiluz commentedDec 8, 2023
edited
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
Issues-
LicenseMIT

xxh128 is 60 times faster thansha256 (source:https://php.watch/versions/8.1/xxHash) but it's not cryptographically secure.

In previous versions we already usedxxh128 in some places. In this PR I propose to use it in all places where we don't need a cryptographically secure hash.

Comments:

#SymfonyHackday

GromNaN reacted with rocket emoji
@fabpot
Copy link
Member

@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)));
Copy link
Member

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
Copy link

Why not xxh3? (just asking for curiosity)

ro0NL reacted with eyes emoji

@GromNaN
Copy link
Member

Why not xxh3? (just asking for curiosity)

In fact, where the hash is substr,xxh3 is sufficient andslightly faster.

@javiereguiluz
Copy link
MemberAuthor

I think it's better to usexxh128 instead ofxxh3 because:

  • We already usexxh128 in other parts of Symfony
  • xxh3 is faster, but only 3% faster
  • We're moving from 256 bits (sha256) to 128 bits (xxh128) ... using xxh3 would mean going to 64 bits ... maybe it's too low compared to the original
mahono reacted with thumbs up emoji

@fabpot
Copy link
Member

Thank you@javiereguiluz.

fabpot added a commit that referenced this pull requestJun 11, 2024
… 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
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

@jderussejderussejderusse left review comments

@fabpotfabpotfabpot approved these changes

@GromNaNGromNaNGromNaN approved these changes

@welcoMatticwelcoMatticAwaiting requested review from welcoMatticwelcoMattic is a code owner

@kbondkbondAwaiting requested review from kbond

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

7 participants

@javiereguiluz@fabpot@mahono@GromNaN@nicolas-grekas@jderusse@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp