-
Notifications
You must be signed in to change notification settings - Fork 153
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
[8.17](backport #6192) Use xxhash instead of sha256 for hashing AST nodes #6252
Conversation
Cherry-pick of 9c13110 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
579b24a
to
951de19
Compare
This pull request is now in conflicts. Could you fix it? 🙏
|
951de19
to
f50a88d
Compare
This pull request has not been merged yet. Could you please review and merge it @swiatekm? 🙏 |
f50a88d
to
f0c2637
Compare
# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) # Conflicts: # internal/pkg/agent/transpiler/ast.go # Conflicts: # internal/pkg/agent/transpiler/ast.go
f0c2637
to
eda16c8
Compare
Quality Gate failedFailed conditions |
) (elastic#6252)" This reverts commit 0b8bd08.
) (elastic#6252)" This reverts commit 0b8bd08.
What does this PR do?
We currently use sha256 for hashing AST nodes when generating configuration. This is used both for checking equality between AST instances and for avoiding recomputing inputs unnecessarily. This PR changes this to xxHash, which is much faster. It also changes the interface, letting the caller supply their own hasher instance, and avoiding needing to allocate one for each Node in the tree.
Of note is that I'm using the
xxhash.Digest
struct as an argument instead of the generichash.Hash64
interface from the standard library. The reason is that the former has an optimizedWriteString
method, which avoid needing to cast the string to a byte slice. I also don't expect to need to actually supply different hash implementations to this method.Note also that there's already fairly exhaustive tests for this, some unexpected behaviour included.
Why is it important?
It's a pretty significant performance improvement. See the benchstat results using the benchmark from #6180:
Checklist
./changelog/fragments
using the changelog toolRelated issues
This is an automatic backport of pull request #6192 done by [Mergify](https://mergify.com).