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!: remove easyjson dependency #52

Merged

Conversation

fabriziosestito
Copy link
Contributor

@fabriziosestito fabriziosestito commented Jul 25, 2023

Description

This PR removes easyjson dependency from the mix by refactoring the example/template policy.
Also, it bumps the build container to tinygo:0.28.1.

Test

Existing tests should pass

Additional Information

Size comparison

before:

λ du -h policy.wasm
1.3M    policy.wasm

after:

λ du -h policy.wasm
480K    policy.wasm

Benchmarks

kwctl bench results

before:

validate_settings [75.8 thousand iterations in 5.07s with 100.0 samples]:
        elapsed [min mean max]: [50.58µs 66.77µs 78.08µs] (sample data: med = 66.64µs, var = 10.04ms², stdde
v = 3.17µs)
validate warming up for 3.00s
validate mean warm up execution time 68.90µs running 75.8 thousand iterations
validate [75.8 thousand iterations in 5.29s with 100.0 samples]:
        elapsed [min mean max]: [69.51µs 69.79µs 70.83µs] (sample data: med = 69.78µs, var = 34.73µs², stdde
v = 186.37ns)

after:

validate_settings [80.8 thousand iterations in 5.26s with 100.0 samples]:
        elapsed [min mean max]: [56.70µs 65.54µs 97.78µs] (sample data: med = 64.76µs, var = 17.64ms², stdde
v = 4.20µs)
validate warming up for 3.00s
validate mean warm up execution time 108.80µs running 50.5 thousand iterations
validate [50.5 thousand iterations in 5.60s with 100.0 samples]:
        elapsed [min mean max]: [94.64µs 111.06µs 166.29µs] (sample data: med = 110.76µs, var = 47.25ms², st
ddev = 6.87µs)

kwctl bench --warm-up-time 1 --measurement-time 1 --num-resamples 2 --num-samples 2 results

before:

validate_settings [15.2 thousand iterations in 1.02s with 100.0 samples]:
        elapsed [min mean max]: [22.47µs 69.78µs 416.87µs] (sample data: med = 65.86µs, var = 1.35s², stddev
 = 36.78µs)
validate warming up for 1.00s
validate mean warm up execution time 68.86µs running 15.2 thousand iterations
validate [15.2 thousand iterations in 1.04s with 100.0 samples]:
        elapsed [min mean max]: [55.74µs 68.49µs 77.68µs] (sample data: med = 68.55µs, var = 4.75ms², stddev
 = 2.18µs)

after:

validate_settings [20.2 thousand iterations in 1.31s with 100.0 samples]:
        elapsed [min mean max]: [23.45µs 66.31µs 275.55µs] (sample data: med = 64.56µs, var = 504.99ms², std
dev = 22.47µs)
validate warming up for 1.00s
validate mean warm up execution time 113.65µs running 10.1 thousand iterations
validate [10.1 thousand iterations in 1.14s with 100.0 samples]:
        elapsed [min mean max]: [95.51µs 112.59µs 197.84µs] (sample data: med = 112.33µs, var = 188.63ms², s
tddev = 13.73µs)

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Awesome! I'm looking forward to have this merged.

Could you also add a quick comparison about the following changes, before and after this PR (basically easyjson vs encoding/json):

  • size of the .wasm module
  • performance comparison (you can use kwctl bench for that)

README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you do the following changes:

  • update the date at the beginning of the file
  • update the section limitations explaining the limitations of tinygo (reflection is now supported)
  • mention we need tinygo 0.28.1 or later - otherwise the code compiles but will panic at runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flavio i've updated the README

Could you also add a quick comparison about the following changes, before and after this PR (basically easyjson vs encoding/json):

I've added this to the PR description.

go.mod Outdated Show resolved Hide resolved
settings.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
@fabriziosestito fabriziosestito changed the title Refactor/remove easyjson refactor!: remove easyjson dependency Jul 26, 2023
@fabriziosestito fabriziosestito force-pushed the refactor/remove-easyjson branch 2 times, most recently from 2f28788 to 924b925 Compare July 26, 2023 10:13
@fabriziosestito fabriziosestito self-assigned this Jul 26, 2023
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM, modulo de pending local replace of policy-sdk-go

@fabriziosestito fabriziosestito marked this pull request as ready for review July 27, 2023 14:38
@fabriziosestito fabriziosestito requested a review from a team as a code owner July 27, 2023 14:38
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

cool, this can be merged 👏

@fabriziosestito fabriziosestito merged commit 6fda6a4 into kubewarden:main Jul 27, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants