Skip to content

Shave ~8ms off generator runtime (SC-945)#1387

Merged
TheRealFalcon merged 4 commits into
canonical:mainfrom
holmanb:holmanb/generator-speed-ups
Apr 25, 2022
Merged

Shave ~8ms off generator runtime (SC-945)#1387
TheRealFalcon merged 4 commits into
canonical:mainfrom
holmanb:holmanb/generator-speed-ups

Conversation

@holmanb
Copy link
Copy Markdown
Member

@holmanb holmanb commented Apr 16, 2022

Shave ~8ms off generator runtime

- remove redundant file reads and subcommands[1]
- add make target that renders templates[2]
- add helper script for measuring script runtime
- add make target that benchmarks generator[3]
- add make target that benchmarks ds-identify[4]

[1] cuts time in lxd from 17-20ms down to 8-13ms in lxc on my local machine
[2] example: make render-template FILE=./systemd/cloud-init-generator.tmpl
[3] usage: NUM_ITER=42 make benchmark-generator
[4] usage: NUM_ITER=42 make benchmark-ds-identify

Additional Context

  • read_proc_cmdline() gets run twice currently. I don't believe this is necessary. This is where the time savings come from.

@holmanb holmanb force-pushed the holmanb/generator-speed-ups branch 2 times, most recently from da5c616 to 0e17509 Compare April 16, 2022 06:25
- remove redundant file reads and subcommands[1]
- add make target that renders templates[2]
- add make target that benchmarks generator[3]

[1] cuts time in lxd from 17-20ms down to 8-13ms in lxc on my local machine
[2] example: make render-template FILE=./systemd/cloud-init-generator.tmpl
[3] usage: NUM_RUNS=42 make benchmark-generator
@holmanb holmanb force-pushed the holmanb/generator-speed-ups branch from 0e17509 to 77a4ccd Compare April 16, 2022 06:31
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

The generator changes LGTM, and I think it makes sense to have a render-template target in the Makefile. I'm not sure I agree with putting performance benchmarking in the makefile. It's unrelated to actually building anything, and is more or less shoehorning a shell script into Makefile syntax. I think it'd make sense as a separate script in the tools directory. Additionally, I think there should be some indication that the benchmarking shouldn't be run on a system not being used to test cloud-init.

@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Apr 18, 2022

It's unrelated to actually building anything, and is more or less shoehorning a shell script into Makefile syntax. I think it'd make sense as a separate script in the tools directory.

I'm fine with moving the "benchmark" implementation to a shell script in tools/, but I would prefer if we keep a target in the makefile for easy discovery and invocation.

At a glance I see several other targets that are small shell scripts in the Makefile (some of which I wrote, admittedly). For consistency, would you prefer moving other tool implementations out of the Makefile and into tools/?

Additionally, I think there should be some indication that the benchmarking shouldn't be run on a system not being used to test cloud-init.

+1

@holmanb holmanb marked this pull request as draft April 19, 2022 15:45
@holmanb holmanb force-pushed the holmanb/generator-speed-ups branch from 1c5bcc6 to 4ed19e9 Compare April 20, 2022 13:22
@holmanb holmanb marked this pull request as ready for review April 20, 2022 13:25
@holmanb holmanb changed the title Shave ~8ms off generator runtime Shave ~8ms off generator runtime (SC-945) Apr 20, 2022
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

I dub thee the official Makefile maintainer.

@TheRealFalcon TheRealFalcon merged commit d6b4301 into canonical:main Apr 25, 2022
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.

2 participants