Avoid unnecessary copies during config generation #6184

swiatekm · 2024-12-02T16:14:31Z

What does this PR do?

Remove some unnecessary copies when generating component configuration.

The first copy avoided is done because we want to replace the inputs key with the rendered inputs. A shallow copy is sufficient for this. I did find and fix a bug in Dict shallow copies while at it.

The rest of the copies have to do with applying vars to each AST node. We currently make a copy of each Node we do this for, but that's not necessary, as Apply creates a new Node with all the data recursively copied anyway. I've added a test to verify that Apply actually works this way.

Benchstat result vs main, 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_noclone.txt          │
                                      │     sec/op     │   sec/op     vs base                │
Coordinator_generateComponentModel-20     37.99m ± 24%   32.36m ± 4%  -14.82% (p=0.000 n=10)

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

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

Why is it important?

More performance is good! More broadly, this is an attempt to incrementally improve the problem from #5991.

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

mergify · 2024-12-02T16:15:10Z

This pull request does not have a backport label. Could you fix it @swiatekm? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-12-02T16:15:10Z

backport-v8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

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

elasticmachine · 2024-12-02T17:14:54Z

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

elastic-sonarqube · 2024-12-02T17:46:47Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

kaanyalti

Code changes look good to me, but I don't have much context here. It would be better for someone else to look at it as well.

blakerouse

Nice improvement! Looks good.

blakerouse · 2024-12-09T15:46:42Z

internal/pkg/agent/transpiler/utils.go

 			if !ok {
 				continue
 			}
 			hadStreams := false
 			if streams := getStreams(dict); streams != nil {
 				hadStreams = true
 			}
+			// Apply creates a new Node with a deep copy of all the values


Thanks for the comment here, helps with understanding that Apply is going to return a copy.

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322)

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322) # Conflicts: # internal/pkg/agent/transpiler/ast.go

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322) # Conflicts: # internal/pkg/agent/transpiler/ast.go # internal/pkg/agent/transpiler/ast_test.go

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322) # Conflicts: # internal/pkg/agent/transpiler/ast.go

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322) Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.com>

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322) # Conflicts: # internal/pkg/agent/transpiler/ast.go Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.com>

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322) # Conflicts: # internal/pkg/agent/transpiler/ast.go # internal/pkg/agent/transpiler/ast_test.go # Conflicts: # internal/pkg/agent/transpiler/ast.go # internal/pkg/agent/transpiler/ast_test.go

* Fix a bug in AST dictionary shallow cloning * Avoid unnecessary deepcopy during config generation * Don't unnecessarily copy AST when rendering inputs # Conflicts: # internal/pkg/agent/transpiler/ast.go (cherry picked from commit 398f322) # Conflicts: # internal/pkg/agent/transpiler/ast.go # internal/pkg/agent/transpiler/ast_test.go # Conflicts: # internal/pkg/agent/transpiler/ast.go # internal/pkg/agent/transpiler/ast_test.go Co-authored-by: Mikołaj Świątek <mail@mikolajswiatek.com>

swiatekm added 2 commits December 2, 2024 17:11

Fix a bug in AST dictionary shallow cloning

9bbea06

Avoid unnecessary deepcopy during config generation

928f1e1

mergify bot assigned swiatekm Dec 2, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Dec 2, 2024

swiatekm added backport-8.16 Automated backport with mergify backport-8.17 Automated backport with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Dec 2, 2024

swiatekm force-pushed the feat/config-gen-remove-copies branch from 3e175a0 to 969aac9 Compare December 2, 2024 16:50

Don't unnecessarily copy AST when rendering inputs

f0bf34d

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

swiatekm force-pushed the feat/config-gen-remove-copies branch from 969aac9 to f0bf34d Compare December 2, 2024 17:00

swiatekm added the enhancement New feature or request label Dec 2, 2024

swiatekm marked this pull request as ready for review December 2, 2024 17:14

swiatekm requested a review from a team as a code owner December 2, 2024 17:14

swiatekm requested review from blakerouse and kaanyalti December 2, 2024 17:14

swiatekm mentioned this pull request Dec 5, 2024

Cache conditions when applying variables to config #6229

Merged

4 tasks

kaanyalti approved these changes Dec 5, 2024

View reviewed changes

blakerouse approved these changes Dec 9, 2024

View reviewed changes

swiatekm merged commit 398f322 into main Dec 9, 2024
15 checks passed

swiatekm deleted the feat/config-gen-remove-copies branch December 9, 2024 16:06

This was referenced Dec 9, 2024

[8.16](backport #6184) Avoid unnecessary copies during config generation #6247

Merged

[8.x](backport #6184) Avoid unnecessary copies during config generation #6248

Merged

[8.17](backport #6184) Avoid unnecessary copies during config generation #6249

Merged

This was referenced Dec 11, 2024

[8.16](backport #6229) Cache conditions when applying variables to config #6279

Merged

[8.x](backport #6229) Cache conditions when applying variables to config #6280

Merged

[8.17](backport #6229) Cache conditions when applying variables to config #6281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessary copies during config generation #6184

Avoid unnecessary copies during config generation #6184

swiatekm commented Dec 2, 2024 •

edited

Loading

mergify bot commented Dec 2, 2024

mergify bot commented Dec 2, 2024

elasticmachine commented Dec 2, 2024

elastic-sonarqube bot commented Dec 2, 2024

kaanyalti left a comment

blakerouse left a comment

blakerouse Dec 9, 2024

Avoid unnecessary copies during config generation #6184

Avoid unnecessary copies during config generation #6184

Conversation

swiatekm commented Dec 2, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

mergify bot commented Dec 2, 2024

mergify bot commented Dec 2, 2024

elasticmachine commented Dec 2, 2024

elastic-sonarqube bot commented Dec 2, 2024

Quality Gate passed

kaanyalti left a comment

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

blakerouse Dec 9, 2024

Choose a reason for hiding this comment

swiatekm commented Dec 2, 2024 •

edited

Loading