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

Add CPU template benchmarking for JSON ops #3618

Conversation

mattschlebusch
Copy link
Contributor

Changes

Add criterion benchmarks for the to/from string JSON operations for the CustomCpuTemplate type.

Reason

To keep a handle on the performance of serializing and deserializing CPU templates in JSON format.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
[CONTRIBUTING.md][3].

@mattschlebusch mattschlebusch self-assigned this Apr 18, 2023
@mattschlebusch mattschlebusch force-pushed the cpu-config-benchmark branch 8 times, most recently from e9ff78f to 8cf0824 Compare April 18, 2023 19:00
@mattschlebusch mattschlebusch marked this pull request as ready for review April 18, 2023 19:24
@mattschlebusch mattschlebusch added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Apr 18, 2023
@mattschlebusch mattschlebusch requested a review from a team April 18, 2023 19:51
src/vmm/src/guest_config/templates/test_utils/mod.rs Outdated Show resolved Hide resolved
src/vmm/benches/cpu_templates.rs Outdated Show resolved Hide resolved
src/vmm/Cargo.toml Outdated Show resolved Hide resolved
src/vmm/benches/cpu_templates.rs Outdated Show resolved Hide resolved
src/vmm/benches/cpu_templates.rs Outdated Show resolved Hide resolved
tests/integration_tests/build/test_coverage.py Outdated Show resolved Hide resolved
@mattschlebusch mattschlebusch force-pushed the cpu-config-benchmark branch 2 times, most recently from 1e31050 to 9ac20cd Compare April 19, 2023 11:37
Minimizes duplicating the creation of test data,
specifically for CustomCpuTemplate and CpuConfiguration.

Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
This change includes some minor refactoring to support different
criterion benchmarks.

Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
Copy link
Contributor

@bchalios bchalios left a comment

Choose a reason for hiding this comment

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

My main comment is do we really need these as part of the PR CI tests?

If I understand correctly, this essentially is benchmarking the serde implementation. I remember we had a discussion regarding a similar situation with versionize.

In any case, my main concern is whether we might add another good candidate for intermittent failures in the CI, blocking PRs, for testing something that is not Firecracker specific.

src/vmm/src/guest_config/templates/mod.rs Show resolved Hide resolved
Copy link
Contributor Author

@mattschlebusch mattschlebusch left a comment

Choose a reason for hiding this comment

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

If I understand correctly, this essentially is benchmarking the serde implementation. I remember we had a discussion regarding a similar situation with versionize.

Not quite. There is quite a bit of custom serialization and deserialization code for the fields in the CustomCpuTemplate type.

In any case, my main concern is whether we might add another good candidate for intermittent failures in the CI, blocking PRs, for testing something that is not Firecracker specific.

The format of the template and the custom processing we include for the fields in the template is not using default derivations unfortunately.

@mattschlebusch mattschlebusch merged commit e9d3080 into firecracker-microvm:feature/cpu-templates Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants