Skip to content

feat: upgrade of deserialization of RVF.#3722

Merged
ShadowCurse merged 3 commits intofirecracker-microvm:mainfrom
ShadowCurse:serde_upgrade
May 24, 2023
Merged

feat: upgrade of deserialization of RVF.#3722
ShadowCurse merged 3 commits intofirecracker-microvm:mainfrom
ShadowCurse:serde_upgrade

Conversation

@ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented May 23, 2023

  • Rewrote deserialization method for RegisterValueFilter.
  • Improved performance by ~75%.
  • Added ability to use _ in string representation.

running: cargo bench --bench cpu_templates

deserialize_cpu_template
                        time:   [5.5389 µs 5.5402 µs 5.5419 µs]
                        change: [-77.543% -77.536% -77.530%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 200 measurements (5.50%)
  2 (1.00%) low mild
  4 (2.00%) high mild
  5 (2.50%) high severe

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.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet contribution quality standards.

  • This functionality cannot be added in rust-vmm.

@ShadowCurse ShadowCurse force-pushed the serde_upgrade branch 2 times, most recently from 24a043f to e96342d Compare May 23, 2023 13:26
@ShadowCurse ShadowCurse self-assigned this May 23, 2023
@ShadowCurse ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Enhancement Indicates new feature requests labels May 23, 2023
@zulinx86
Copy link
Contributor

zulinx86 commented May 23, 2023

Improved performance by ~65%.

Great news! Should we update baseline values as well?
https://github.com/firecracker-microvm/firecracker/blob/main/tests/integration_tests/performance/test_cpu_template_benchmark.py

@ShadowCurse
Copy link
Contributor Author

@zulinx86 Because perf tests pass I assume the delta is big enough for this improvement. I don't really think changing baselines here is necessary.

@zulinx86
Copy link
Contributor

@zulinx86 Because perf tests pass I assume the delta is big enough for this improvement. I don't really think changing baselines here is necessary.

Hmm, 65% looks huge enough to me.
But this issue seems to be caused by the fact that the benchmark measure it in milliseconds granularity.
https://github.com/firecracker-microvm/firecracker/blob/main/tests/integration_tests/performance/test_cpu_template_benchmark.py#L25-L38

For future reference, I'd appreciate if you could post the benchmark result before and after the change here.

Copy link
Contributor

@roypat roypat left a comment

Choose a reason for hiding this comment

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

Mh, is this on the hot path? I'm not very familiar with y'all's work the past few months, but I thought that the json deserialization wasn't supposed to be on the hot path and instead some optimized binary forward was used? :o

@zulinx86
Copy link
Contributor

zulinx86 commented May 23, 2023

Mh, is this on the hot path? I'm not very familiar with y'all's work the past few months, but I thought that the json deserialization wasn't supposed to be on the hot path and instead some optimized binary forward was used? :o

I don't have full context behind that, but the baseline values are ~45 us on x86 and ~7.5us on aarch64.
https://github.com/firecracker-microvm/firecracker/blob/main/tests/integration_tests/performance/test_cpu_template_benchmark.py#L25-L38

Test templates are here:

IIUC, we don't support binary compiled CPU template in firecracker v1.4. If users need more performance, we can support binary ones in successive versions.

@mattschlebusch Do you have more context about this?

@ShadowCurse
Copy link
Contributor Author

Mh, is this on the hot path? I'm not very familiar with y'all's work the past few months, but I thought that the json deserialization wasn't supposed to be on the hot path and instead some optimized binary forward was used? :o

So this whole PR was created, because I thought adding _ as a separator would be nice in our trinary format.
The refactoring just came naturally. Why wouldn't I optimize code that I touch?

@ShadowCurse ShadowCurse force-pushed the serde_upgrade branch 4 times, most recently from ab168ce to 496586d Compare May 24, 2023 11:56
@ShadowCurse ShadowCurse force-pushed the serde_upgrade branch 2 times, most recently from f96baa2 to 65242cc Compare May 24, 2023 13:26
Rewrote deserialization method for `RegisterValueFilter`.
Significantly improved performance (~75%).
Added ability to use `_` in string representation.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Minor change to use `Numeric` trait to constrain `V`
parameter in `RegisterValueFilter` instead of separate traits

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Performance tests were blown away by such
blazingly fast deserialization.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse ShadowCurse merged commit 1b83eec into firecracker-microvm:main May 24, 2023
@ShadowCurse ShadowCurse deleted the serde_upgrade branch May 24, 2023 15:37
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Jun 19, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR firecracker-microvm#3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Jun 19, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR firecracker-microvm#3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Jun 20, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR firecracker-microvm#3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Jun 20, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR firecracker-microvm#3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Jun 21, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR firecracker-microvm#3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Jun 26, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR firecracker-microvm#3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit that referenced this pull request Jun 26, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR #3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
ShadowCurse pushed a commit to ShadowCurse/firecracker that referenced this pull request Jul 26, 2023
Removes `ModifierMapvalue` since `Numeric` was introduced in PR firecracker-microvm#3722
and has similar functionalities.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
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 Type: Enhancement Indicates new feature requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments