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

makes guest API more similar to host #32

Closed codefromthecrypt closed 11 months ago

codefromthecrypt commented 11 months ago

What type of PR is this?

/kind cleanup /kind api-change

What this PR does / why we need it:

This makes the parameters to filter function more like those in the host by creating a proxy interface. Doing so only slows down the no-op case by some tens of nanoseconds, so it doesn't matter.

The interesting part is if users return nil for status instead of multiple values it is faster. So, this changes signature to return a pointer to status. (I checked against stack based value and it is still faster).

Finally, this optimizes the example, because our test data doesn't actually have podSpecNodeName field. This drop in execution time proves parameters not read are lazy.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE because the project isn't usable yet.

What are the benchmark results of this change?

First, changing to new guest signatures

goos: darwin
goarch: arm64
pkg: sigs.k8s.io/kube-scheduler-wasm-extension/internal/e2e
                                            │   old.txt   │              new.txt               │
                                            │   sec/op    │   sec/op     vs base               │
PluginFilter/noop/params:_small-12            147.2n ± 1%   187.8n ± 0%  +27.54% (p=0.002 n=6)
PluginFilter/noop/params:_real-12             148.4n ± 1%   186.4n ± 1%  +25.57% (p=0.002 n=6)
PluginFilter/filter-simple/params:_small-12   7.274µ ± 1%   6.518µ ± 6%  -10.39% (p=0.002 n=6)
PluginFilter/filter-simple/params:_real-12    113.9µ ± 1%   101.2µ ± 1%  -11.21% (p=0.002 n=6)
geomean                                       2.063µ        2.192µ        +6.25%

Next, optimizing the filter example

goos: darwin
goarch: arm64
pkg: sigs.k8s.io/kube-scheduler-wasm-extension/internal/e2e
                                            │   old.txt    │              new.txt               │
                                            │    sec/op    │   sec/op     vs base               │
goos: darwin
goarch: arm64
pkg: sigs.k8s.io/kube-scheduler-wasm-extension/internal/e2e
                                            │   old.txt    │              new.txt               │
                                            │    sec/op    │   sec/op     vs base               │
PluginFilter/noop/params:_small-12             147.2n ± 1%   187.3n ± 2%  +27.20% (p=0.002 n=6)
PluginFilter/noop/params:_real-12              148.4n ± 1%   187.1n ± 3%  +26.08% (p=0.002 n=6)
PluginFilter/filter-simple/params:_small-12    7.274µ ± 1%   6.564µ ± 3%   -9.75% (p=0.002 n=6)
PluginFilter/filter-simple/params:_real-12    113.95µ ± 1%   49.28µ ± 2%  -56.75% (p=0.002 n=6)
geomean                                        2.063µ        1.835µ       -11.05%
codefromthecrypt commented 11 months ago

ps I ran tests manually, and they pass. after https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/pull/30 we don't need manual verification

codefromthecrypt commented 11 months ago

@anuraaga your review is helpful thanks! many things I'm not committed to, and it is helpful to see someone looking!

codefromthecrypt commented 11 months ago

I should disclose the simple wasm filter grew 9KB over this, but it is 9/876 and probably unanimously worth it

codefromthecrypt commented 11 months ago

@anuraaga can you take another look and approve or let me know if any glitches left? since this has no formal approvals to lose yet, I snuck in some more godoc and polish.

k8s-ci-robot commented 11 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anuraaga, 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)~~ [sanposhiho] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment