kubernetes-sigs / kube-scheduler-wasm-extension

All the things to make the scheduler extendable with wasm.
Apache License 2.0
86 stars 17 forks source link

Adds cyclestate.Pod to share unmarshalled result to all plugins #45

Closed codefromthecrypt closed 11 months ago

codefromthecrypt commented 11 months ago

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Before, each plugin would unmarshal the pod being scheduled. For example, PreFilter, Filter, Score would unmarshal the same pod three times. This exposes an internal field cyclestate.Pod, reset on PreFilter, which is shared for all plugins. One side-effect to this is that all TinyGo plugins will always implement PreFilterPlugin implicitly. In other words, they will export the "prefilter" function even if they have not called prefilter.SetPlugin.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

What are the benchmark results of this change?

The baseline of benchmarks changed because now all of them call PreFilter first. While there is an increase in latency of the "small" tests, this is nothing for concern because the data isn't of realistic size. The increased time is due in part to the second callback. What's important is when multiple plugins are executed, the latency is significantly down. This is the case in PluginPrefilterFilterAndScore.

goos: darwin
goarch: arm64
pkg: sigs.k8s.io/kube-scheduler-wasm-extension/internal/e2e
                                                        │  before.txt   │             after.txt              │
                                                        │    sec/op     │   sec/op     vs base               │
PluginPreFilter/noop-wat/params:_small-12                  279.9n ±  1%   284.8n ± 1%   +1.75% (p=0.004 n=6)
PluginPreFilter/noop-wat/params:_real-12                   287.4n ±  2%   289.1n ± 1%        ~ (p=0.589 n=6)
PluginPreFilter/noop/params:_small-12                      344.6n ±  2%   308.6n ± 1%  -10.45% (p=0.002 n=6)
PluginPreFilter/noop/params:_real-12                       348.4n ±  2%   313.1n ± 1%  -10.16% (p=0.002 n=6)
PluginPreFilter/test/params:_small-12                      3.406µ ±  0%   3.993µ ± 0%  +17.22% (p=0.002 n=6)
PluginPreFilter/test/params:_real-12                       45.72µ ± 14%   45.41µ ± 1%        ~ (p=0.699 n=6)
PluginFilter/noop-wat/params:_small-12                     263.2n ±  0%   402.1n ± 0%  +52.80% (p=0.002 n=6)
PluginFilter/noop-wat/params:_real-12                      267.9n ±  2%   408.5n ± 0%  +52.45% (p=0.002 n=6)
PluginFilter/noop/params:_small-12                         326.8n ±  0%   457.9n ± 2%  +40.12% (p=0.002 n=6)
PluginFilter/noop/params:_real-12                          330.6n ±  0%   470.8n ± 2%  +42.42% (p=0.002 n=6)
PluginFilter/test/params:_small-12                         6.479µ ±  1%   7.507µ ± 1%  +15.88% (p=0.002 n=6)
PluginFilter/test/params:_real-12                          115.7µ ±  2%   113.2µ ± 2%        ~ (p=0.065 n=6)
PluginScore/noop-wat/params:_small-12                      258.7n ±  0%   399.8n ± 1%  +54.52% (p=0.002 n=6)
PluginScore/noop-wat/params:_real-12                       263.3n ±  1%   402.9n ± 1%  +53.01% (p=0.002 n=6)
PluginScore/noop/params:_small-12                          379.1n ±  0%   503.5n ± 0%  +32.80% (p=0.002 n=6)
PluginScore/noop/params:_real-12                           346.7n ±  0%   473.5n ± 0%  +36.59% (p=0.002 n=6)
PluginScore/test/params:_small-12                          3.601µ ±  1%   4.263µ ± 1%  +18.37% (p=0.002 n=6)
PluginScore/test/params:_real-12                           39.82µ ± 17%   43.87µ ± 8%        ~ (p=0.132 n=6)
PluginPrefilterFilterAndScore/noop-wat/params:_small-12    517.2n ±  0%   524.3n ± 0%   +1.37% (p=0.002 n=6)
PluginPrefilterFilterAndScore/noop-wat/params:_real-12     521.2n ±  2%   527.8n ± 0%        ~ (p=0.065 n=6)
PluginPrefilterFilterAndScore/noop/params:_small-12        779.5n ±  0%   683.4n ± 0%  -12.32% (p=0.002 n=6)
PluginPrefilterFilterAndScore/noop/params:_real-12         746.1n ±  0%   731.3n ± 0%   -1.99% (p=0.002 n=6)
PluginPrefilterFilterAndScore/test/params:_small-12       14.088µ ±  1%   7.773µ ± 0%  -44.82% (p=0.002 n=6)
PluginPrefilterFilterAndScore/test/params:_real-12         216.5µ ±  2%   111.8µ ± 2%  -48.39% (p=0.002 n=6)
geomean                                                    1.429µ         1.551µ        +8.51%
codefromthecrypt commented 11 months ago

I think there's a bug in the test score-simple plugin setup as it is too fast. I'll look into it.

PluginScore/test/params:_real-12                          39818.0n ± 17%   350.1n ±  1%  -99.12% (p=0.002 n=6)
codefromthecrypt commented 11 months ago

ah right I didn't change the benchmarks to begin with PreFilter, so they were caching the result. fixing now

codefromthecrypt commented 11 months ago

ok I fixed the benchmarks and updated the description about what's going on. If not clear I can elaborate more.

k8s-ci-robot commented 11 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codefromthecrypt, sanposhiho

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/blob/main/OWNERS)~~ [codefromthecrypt,sanposhiho] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
sanposhiho commented 11 months ago

Ah no, you cannot use the button yet. I will give lgtm again later.

sanposhiho commented 11 months ago

/lgtm