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

core/filemgr: use xxhash instead of sha512 for filenames #4697

Merged
merged 1 commit into from Nov 1, 2023

Conversation

calebdoxsey
Copy link
Contributor

@calebdoxsey calebdoxsey commented Nov 1, 2023

Summary

For envoy we use files as a DataSource whenever possible. When we have raw data we save it to a cache directory with a filename like tls-crt-xxx.pem. The hash of the filename is computed using the SHA512_256 of the bytes.

For this use case its not necessary to have a cryptographic hash. We can instead use xxhash which is much faster. Roughly 10x improvement:

// BenchmarkGetFileNameWithBytesHash-10    	   12331	     95284 ns/op	    1657 B/op	      63 allocs/op
// BenchmarkGetFileNameWithBytesHash-10    	  112304	      9472 ns/op	     112 B/op	       5 allocs/op

Related issues

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@coveralls
Copy link

Coverage Status

coverage: 63.99% (-0.03%) from 64.015% when pulling 9ac7f53 on cdoxsey/bytes-data-source-performance into 53573dc on main.

@wasaga
Copy link
Contributor

wasaga commented Nov 1, 2023

does xxhash require cgo?

The package is written with optimized pure Go and also contains even faster assembly implementations for amd64 and arm64. If desired, the purego build tag opts into using the Go code even on those architectures.

@calebdoxsey
Copy link
Contributor Author

Not as far as I know. I didn't add xxhash. We already use it. If we don't trust it I can replace it with something else everywhere we use it.

Copy link
Contributor

@wasaga wasaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice it's an existing dependency

@calebdoxsey calebdoxsey merged commit fd8cb18 into main Nov 1, 2023
10 checks passed
@calebdoxsey calebdoxsey deleted the cdoxsey/bytes-data-source-performance branch November 1, 2023 19:52
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.

None yet

3 participants