Use xxhash instead of sha256 for hashing AST nodes #6192

swiatekm · 2024-12-03T15:09:57Z

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 generic hash.Hash64 interface from the standard library. The reason is that the former has an optimized WriteString 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:

goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent/internal/pkg/agent/application/coordinator
cpu: 13th Gen Intel(R) Core(TM) i7-13700H
                                      │ bench_main.txt │           bench_hash.txt            │
                                      │     sec/op     │    sec/op     vs base               │
Coordinator_generateComponentModel-20     37.99m ± 24%   34.29m ± 14%  -9.73% (p=0.009 n=10)

                                      │ bench_main.txt │           bench_hash.txt            │
                                      │      B/op      │     B/op      vs base               │
Coordinator_generateComponentModel-20     34.22Mi ± 0%   31.53Mi ± 0%  -7.88% (p=0.000 n=10)

                                      │ bench_main.txt │           bench_hash.txt            │
                                      │   allocs/op    │  allocs/op   vs base                │
Coordinator_generateComponentModel-20      810.7k ± 0%   713.0k ± 0%  -12.05% (p=0.000 n=10)

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool

Related issues

elasticmachine · 2024-12-03T15:10:00Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

mergify · 2024-12-03T15:11:06Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b chore/config-gen-optimize-hashing upstream/chore/config-gen-optimize-hashing
git merge upstream/main
git push upstream chore/config-gen-optimize-hashing

# Conflicts: # internal/pkg/agent/transpiler/ast.go

elastic-sonarqube · 2024-12-03T17:29:28Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
86.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

blakerouse · 2024-12-09T16:06:13Z

internal/pkg/agent/transpiler/ast.go

@@ -58,6 +59,9 @@ type Node interface {
 	// Hash compute a sha256 hash of the current node and recursively call any children.
 	Hash() []byte

+	// Hash64With recursively computes the given hash for the Node and its children
+	Hash64With(h *xxhash.Digest) error


Why not just replace the usage of Hash()? Why do you have both? Do we need both?

I don't think we need both. If there were a bunch of callers of Hash, then I'd be in favor of keeping it and implementing it using Hash64With, but there aren't, so we can get rid of it. I was planning to do so in a follow up.

blakerouse

Okay, removal in follow-up is good.

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110)

# 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 (cherry picked from commit 9c13110) # Conflicts: # go.mod # internal/pkg/agent/transpiler/ast.go

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) Co-authored-by: Mikołaj Świątek <[email protected]>

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) # Conflicts: # go.mod # internal/pkg/agent/transpiler/ast.go # Conflicts: # internal/pkg/agent/transpiler/ast.go

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) # Conflicts: # go.mod # internal/pkg/agent/transpiler/ast.go # Conflicts: # internal/pkg/agent/transpiler/ast.go Co-authored-by: Mikołaj Świątek <[email protected]>

# 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 (cherry picked from commit 9c13110) # Conflicts: # internal/pkg/agent/transpiler/ast.go # Conflicts: # internal/pkg/agent/transpiler/ast.go

# 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 Co-authored-by: Mikołaj Świątek <[email protected]>

) (elastic#6252)" This reverts commit 0b8bd08.

swiatekm added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify backport-8.17 Automated backport with mergify labels Dec 3, 2024

swiatekm requested a review from a team as a code owner December 3, 2024 15:09

swiatekm requested review from kaanyalti and andrzej-stencel December 3, 2024 15:09

mergify bot assigned swiatekm Dec 3, 2024

swiatekm force-pushed the chore/config-gen-optimize-hashing branch from 69dde06 to d8326e3 Compare December 3, 2024 15:14

swiatekm changed the title ~~Chore/config gen optimize hashing~~ Use xxhash instead of sha256 for hashing AST nodes Dec 3, 2024

swiatekm force-pushed the chore/config-gen-optimize-hashing branch from d8326e3 to d7220b4 Compare December 3, 2024 15:30

Use xxhash instead of sha256 for hashing AST nodes

5b699bf

# Conflicts: # internal/pkg/agent/transpiler/ast.go

swiatekm force-pushed the chore/config-gen-optimize-hashing branch from d7220b4 to 5b699bf Compare December 3, 2024 16:36

andrzej-stencel approved these changes Dec 5, 2024

View reviewed changes

blakerouse reviewed Dec 9, 2024

View reviewed changes

blakerouse approved these changes Dec 9, 2024

View reviewed changes

swiatekm merged commit 9c13110 into main Dec 9, 2024
14 checks passed

swiatekm deleted the chore/config-gen-optimize-hashing branch December 9, 2024 17:04

mergify bot pushed a commit that referenced this pull request Dec 9, 2024

Use xxhash instead of sha256 for hashing AST nodes (#6192)

dca1306

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110)

mergify bot pushed a commit that referenced this pull request Dec 9, 2024

Use xxhash instead of sha256 for hashing AST nodes (#6192)

579b24a

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) # Conflicts: # internal/pkg/agent/transpiler/ast.go

mergify bot pushed a commit that referenced this pull request Dec 9, 2024

Use xxhash instead of sha256 for hashing AST nodes (#6192)

e9bccff

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) # Conflicts: # go.mod # internal/pkg/agent/transpiler/ast.go

This was referenced Dec 9, 2024

[8.16](backport #6192) Use xxhash instead of sha256 for hashing AST nodes #6250

Merged

[8.x](backport #6192) Use xxhash instead of sha256 for hashing AST nodes #6251

Merged

[8.17](backport #6192) Use xxhash instead of sha256 for hashing AST nodes #6252

Merged

swiatekm added a commit that referenced this pull request Dec 9, 2024

Use xxhash instead of sha256 for hashing AST nodes (#6192)

441f560

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) # Conflicts: # go.mod # internal/pkg/agent/transpiler/ast.go

swiatekm added a commit that referenced this pull request Dec 9, 2024

Use xxhash instead of sha256 for hashing AST nodes (#6192) (#6251)

7b4374b

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) Co-authored-by: Mikołaj Świątek <[email protected]>

swiatekm added a commit that referenced this pull request Dec 13, 2024

Use xxhash instead of sha256 for hashing AST nodes (#6192)

951de19

# Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 9c13110) # Conflicts: # internal/pkg/agent/transpiler/ast.go

orestisfl added a commit to orestisfl/elastic-agent that referenced this pull request Jan 24, 2025

Revert "Use xxhash instead of sha256 for hashing AST nodes (elastic#6192

d76f381

) (elastic#6252)" This reverts commit 0b8bd08.

orestisfl added a commit to orestisfl/elastic-agent that referenced this pull request Jan 24, 2025

Revert "Use xxhash instead of sha256 for hashing AST nodes (elastic#6192

50f9943

) (elastic#6252)" This reverts commit 0b8bd08.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use xxhash instead of sha256 for hashing AST nodes #6192

Use xxhash instead of sha256 for hashing AST nodes #6192

swiatekm commented Dec 3, 2024 •

edited

Loading

elasticmachine commented Dec 3, 2024

mergify bot commented Dec 3, 2024

elastic-sonarqube bot commented Dec 3, 2024

blakerouse Dec 9, 2024

swiatekm Dec 9, 2024

blakerouse left a comment

Use xxhash instead of sha256 for hashing AST nodes #6192

Use xxhash instead of sha256 for hashing AST nodes #6192

Conversation

swiatekm commented Dec 3, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Dec 3, 2024

mergify bot commented Dec 3, 2024

elastic-sonarqube bot commented Dec 3, 2024

Quality Gate passed

blakerouse Dec 9, 2024

Choose a reason for hiding this comment

swiatekm Dec 9, 2024

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

swiatekm commented Dec 3, 2024 •

edited

Loading