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

refactor: use gojson to marshal config for SHA #4222

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Jun 26, 2023

What this PR does / why we need it:

Use gojson to marshal config to JSON to get config SHA.

This speeds up the marshalling of config to JSON by about 35%, using roughly the same amount of memory and the same amount of memory allocations.

Results (run locally):

$ benchstat /tmp/old /tmp/new
goos: darwin
goarch: arm64
pkg: github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/deckgen
                      │  /tmp/old   │              /tmp/new               │
                      │   sec/op    │   sec/op     vs base                │
DeckgenGenerateSHA-10   31.32µ ± 0%   20.25µ ± 1%  -35.36% (p=0.000 n=10)

                      │   /tmp/old   │              /tmp/new               │
                      │     B/op     │     B/op      vs base               │
DeckgenGenerateSHA-10   10.95Ki ± 0%   10.87Ki ± 0%  -0.70% (p=0.000 n=10)

                      │  /tmp/old  │            /tmp/new            │
                      │ allocs/op  │ allocs/op   vs base            │
DeckgenGenerateSHA-10   12.00 ± 0%   12.00 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

Results can be observed at https://kong.github.io/kubernetes-ingress-controller/dev/bench/ for 2501474 or at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/5377294841

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@pmalek pmalek self-assigned this Jun 26, 2023
pmalek referenced this pull request Jun 26, 2023
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Patch coverage: 100.0% and project coverage change: -0.1 ⚠️

Comparison is base (9ec5633) 62.6% compared to head (6d0ea66) 62.6%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4222     +/-   ##
=======================================
- Coverage   62.6%   62.6%   -0.1%     
=======================================
  Files        153     153             
  Lines      17006   17006             
=======================================
- Hits       10656   10655      -1     
+ Misses      5685    5683      -2     
- Partials     665     668      +3     
Impacted Files Coverage Δ
internal/dataplane/deckgen/deckgen.go 47.7% <100.0%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmalek pmalek marked this pull request as ready for review June 26, 2023 14:16
@pmalek pmalek requested a review from a team as a code owner June 26, 2023 14:16
@pmalek pmalek added the area/perf Performance Related Issues label Jun 26, 2023
@czeslavo
Copy link
Contributor

czeslavo commented Jun 26, 2023

Do you think it would be worth noting in the changelog? IMHO we could mention it in the "Under the hood" section, WDYT? It's a performance improvement and it might be worth making such changes visible to the users.

@pmalek
Copy link
Member Author

pmalek commented Jun 26, 2023

Do you think it would be worth noting in the changelog? IMHO we could mention it in the "Under the hood" section, WDYT? It's a performance improvement and it might be worth making such changes visible to the users.

I believe so, yes. Just wanted to gather feedback from the team beforehand.

@czeslavo
Copy link
Contributor

Are there any objections to using gojson that we're aware of? 🤔

rainest
rainest previously approved these changes Jun 26, 2023
@rainest
Copy link
Contributor

rainest commented Jun 26, 2023

I'm ambivalent on a changelog entry since the SHA computation isn't something you usually think too much about. Can't think of objections--they say they're compatible with the stdlib version, so as long as they don't break that I wouldn't expect issues.

Should we not also use this elsewhere we're currently using json.Marshal and json.Unmarshal? The internal/dataplane/sendconfig/inmemory.go in particular is operating on a similarly large struct.

@pmalek pmalek force-pushed the deckgen-generate-sha-with-gojson branch from 2501474 to 6d0ea66 Compare June 26, 2023 17:48
@pmalek pmalek added this to the KIC v2.11.0 milestone Jun 26, 2023
@pmalek pmalek requested a review from rainest June 26, 2023 17:48
@pmalek
Copy link
Member Author

pmalek commented Jun 26, 2023

I'm ambivalent on a changelog entry since the SHA computation isn't something you usually think too much about. Can't think of objections--they say they're compatible with the stdlib version, so as long as they don't break that I wouldn't expect issues.

I've added the changelog entry as that's something that people may refer to in case we'll observe performance gains.

Should we not also use this elsewhere we're currently using json.Marshal and json.Unmarshal? The internal/dataplane/sendconfig/inmemory.go in particular is operating on a similarly large struct.

We should definitely consider this but first: measure! Add a benchmark, gather some data points (so that it's visible at: https://kong.github.io/kubernetes-ingress-controller/dev/bench/) and then propose a patch so that we can observe the change (and claim the bragging rights).

@pmalek pmalek merged commit fb3435d into main Jun 27, 2023
36 checks passed
@pmalek pmalek deleted the deckgen-generate-sha-with-gojson branch June 27, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/perf Performance Related Issues refactor size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants