Skip to content
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

[12.x] Replace md5 with much faster xxhash #52301

Merged
merged 4 commits into from
Aug 5, 2024

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Jul 28, 2024

In particular, this may make a noticeable difference for the etag content hashing. md5 doesn't really have a purpose in 2024: xxh128 is much faster and produces the same size hash as md5.

@hafezdivandari
Copy link
Contributor

What about File::hash()?

hash_file instead of md5_file?

@GrahamCampbell
Copy link
Member Author

No, that is intentionally omitted, and should not be changed.

@timacdonald
Copy link
Member

timacdonald commented Jul 28, 2024

We would need to note in the upgrade guide that on the first deploy of an upgraded Laravel 12 app it will reset active rate limiters.

Could be important for applications. It could allow unexpected behaviour within the application if you are using rate limiters to restict business logic, e.g., you can only create 10 invoices per month.

@GrahamCampbell
Copy link
Member Author

Alternatively, I could also revert that portion of this PR, if we don't want this upgrading step.

@timacdonald
Copy link
Member

FWIW we already use xxh128 in a few places within the framework related to view hashing.

return $this->cachePath.'/'.hash('xxh128', 'v2'.Str::after($path, $this->basePath)).'.'.$this->compiledExtension;

if (! is_file($viewFile = $directory.'/'.hash('xxh128', $contents).'.blade.php')) {

static::$parentPlaceholder[$section] = '##parent-placeholder-'.hash('xxh128', $salt.$section).'##';

static::$componentHashStack[] = $hash = hash('xxh128', $component);

@taylorotwell
Copy link
Member

Agree with @timacdonald we probably only want to do this where it isn't "breaking" in the sense of resetting rate limiters, etc.

@taylorotwell taylorotwell marked this pull request as draft July 29, 2024 07:31
@valorin
Copy link
Contributor

valorin commented Jul 31, 2024

I like the idea of swapping to a more efficient hash. Also gets rid of more uses of MD5! 😁

One idea to avoid the breaking change issue is to define the hashing algo in a config file.

Both algos can be used through hash($algo, ...), so it would be trivial to swap between the two by changing the config value without making the code any more complicated. The next major release could default to xxh128, with an upgrade guide step to change back to md5 if your application relies on cache, rate limiters, etc, across upgrades.

@imanghafoori1
Copy link
Contributor

The next major release could default to xxh128, with an upgrade guide step to change back to md5 if your application relies on cache, rate limiters, etc, across upgrades.

Maybe, It is better to default to md5 in source code like this config('app.hash_algo', md5); and add HASH_ALGO=xxh128 to .env.example in laravel/laravel, So upgraders from v11 do not break without noticing anything and installers v12 from scratch also get the performance benefit without noticing anything.

@GrahamCampbell
Copy link
Member Author

GrahamCampbell commented Aug 4, 2024

There is negligible performance benefit for the choice of hashing algorithm for the cache key for throttling. The cost of reading the config and checking it will be more than the performance gain of changing the algorithm for hashing like 10 bytes of data.

@GrahamCampbell GrahamCampbell marked this pull request as ready for review August 4, 2024 19:40
@GrahamCampbell
Copy link
Member Author

OK, I've rolled back the rate limiter change. I think the etag change is low impact and is the place in this PR where we will see the main performance benefit. I think we should note this change in the upgrading guide.

freekmurze pushed a commit to spatie/laravel-responsecache that referenced this pull request Aug 5, 2024
* Switch DefaultHasher to xxh128 for a faster alternative to MD5.

- See https://xxhash.com/
- Laravel is considering replacing MD5 with xxh128: laravel/framework#52301

* Update ResponseHasherTest to reflect xxh128 algorithm changes
@taylorotwell taylorotwell merged commit 87f9dd5 into laravel:master Aug 5, 2024
29 checks passed
@GrahamCampbell GrahamCampbell deleted the xxhash branch August 5, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants