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

Implements prefilter and configures TinyGo unit tests #43

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Jun 19, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This implements the prefilter function and configures TinyGo unit tests via make test-guest. This runs the same unit tests without TinyGo on make test.

Which issue(s) this PR fixes:

Special notes for your reviewer:

  • This uses the name "prefilter" not "preFilter" because it matches the package name and helps clarify it is an operation name. We still use PreFilter when referring to framework.
  • This implements prefilter node name results with a host callback for the same reason as the status reason: using a host callback allows the guest to control memory allocation. This uses a C-String approach to avoid complexity in marshalling. The same approach is used in http-wasm for header values.
  • This intentionally does not do anything other design changes, such as caching state. We can do that in another PR.
  • tinygo test uses a wasmtime by default, so the Makefile has been updated to switch to wazero.
  • To avoid segfaults in TinyGo tests, this removes nottinygc as a strict dependency, and instead conventionally adds it when compiling plugins to wasm.
  • This intentionally does not backfill more TinyGo unit tests because the change is already large.

Does this PR introduce a user-facing change?

NONE

What are the benchmark results of this change?

goos: darwin
goarch: arm64
pkg: sigs.k8s.io/kube-scheduler-wasm-extension/internal/e2e
                                                        │  before.txt  │              after.txt               │
                                                        │    sec/op    │    sec/op     vs base                │
PluginFilter/noop-wat/params:_small-12                    262.3n ±  2%   265.2n ±  1%       ~ (p=0.121 n=6)
PluginFilter/noop-wat/params:_real-12                     269.6n ±  2%   268.3n ±  1%       ~ (p=0.619 n=6)
PluginFilter/noop/params:_small-12                        333.1n ±  1%   328.0n ±  1%  -1.52% (p=0.002 n=6)
PluginFilter/noop/params:_real-12                         338.0n ±  2%   332.1n ±  3%  -1.75% (p=0.004 n=6)
PluginFilter/test/params:_small-12                        6.431µ ±  1%   6.438µ ±  0%       ~ (p=0.669 n=6)
PluginFilter/test/params:_real-12                         115.5µ ±  2%   116.8µ ±  0%  +1.14% (p=0.004 n=6)
PluginScore/noop-wat/params:_small-12                     260.6n ±  0%   259.7n ±  3%       ~ (p=0.056 n=6)
PluginScore/noop-wat/params:_real-12                      265.4n ±  0%   263.9n ±  0%  -0.55% (p=0.004 n=6)
PluginScore/noop/params:_small-12                         382.4n ±  1%   378.1n ±  0%  -1.15% (p=0.002 n=6)
PluginScore/noop/params:_real-12                          354.4n ±  0%   348.1n ±  0%  -1.76% (p=0.002 n=6)
PluginScore/test/params:_small-12                         3.668µ ±  1%   3.639µ ±  1%  -0.79% (p=0.002 n=6)
PluginScore/test/params:_real-12                          39.85µ ±  2%   39.16µ ± 12%       ~ (p=0.065 n=6)
PluginFilterAndScore/noop-wat/params:_small-12            379.8n ±  0%
PluginFilterAndScore/noop-wat/params:_real-12             390.6n ± 16%
PluginFilterAndScore/noop/params:_small-12                586.5n ±  5%
PluginFilterAndScore/noop/params:_real-12                 554.6n ±  6%
PluginFilterAndScore/test/params:_small-12                10.17µ ±  1%
PluginFilterAndScore/test/params:_real-12                 169.5µ ±  4%
PluginPreFilter/noop-wat/params:_small-12                                281.1n ±  3%
PluginPreFilter/noop-wat/params:_real-12                                 289.1n ±  1%
PluginPreFilter/noop/params:_small-12                                    347.2n ±  1%
PluginPreFilter/noop/params:_real-12                                     350.6n ±  0%
PluginPreFilter/test/params:_small-12                                    3.533µ ±  2%
PluginPreFilter/test/params:_real-12                                     40.56µ ±  9%
PluginPrefilterFilterAndScore/noop-wat/params:_small-12                  517.5n ±  1%
PluginPrefilterFilterAndScore/noop-wat/params:_real-12                   524.3n ±  3%
PluginPrefilterFilterAndScore/noop/params:_small-12                      788.3n ±  0%
PluginPrefilterFilterAndScore/noop/params:_real-12                       753.4n ±  0%
PluginPrefilterFilterAndScore/test/params:_small-12                      14.10µ ±  1%
PluginPrefilterFilterAndScore/test/params:_real-12                       218.8µ ±  1%
geomean                                                   1.436µ         1.429µ        -0.65%               ¹

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 19, 2023
@codefromthecrypt
Copy link
Contributor Author

just noticed the prefilter example was almost 1MB of wasm. trying to figure out why, and reduce it.

@codefromthecrypt
Copy link
Contributor Author

I forgot that our baseline size is pretty big (first nottinygc and then tinygo 0.28 is also larger).

Anyway, I was able to make it the same size as score.

$ du -k  examples/*/main.wasm
996	examples/filter-simple/main.wasm
960	examples/prefilter-simple/main.wasm
960	examples/score-simple/main.wasm

@codefromthecrypt codefromthecrypt force-pushed the prefilter branch 2 times, most recently from 02d3243 to 03c1d70 Compare June 20, 2023 01:51
@codefromthecrypt
Copy link
Contributor Author

fixed bench, as the prior commit didn't actually call prefilter. The below is higher because we don't yet cache the pod being scheduled. The next PR should show some dramatic difference

PluginFilterAndScore/test/params:_real-12                     169.5µ ±  4%
PluginPrefilterFilterAndScore/test/params:_real-12            218.8µ ±  1%

@sanposhiho sanposhiho requested a review from kerthcet June 20, 2023 07:20
Copy link
Contributor

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

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

/approve

guest/prefilter/cstring.go Show resolved Hide resolved
guest/prefilter/cstring_test.go Show resolved Hide resolved
scheduler/plugin/host.go Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2023
This implements the prefilter function and configures TinyGo unit tests
via `make test-guest`. This runs the same unit tests without TinyGo on
`make test`.

To avoid segfaults in TinyGo tests, this removes nottinygc as a strict
dependency, and instead conventionally adds it when compiling plugins to wasm.

Notes:
* `tinygo test` uses a wasmtime by default, so the Makefile has been
  updated to switch to wazero.
* This implements prefilter node name results with a host callback for
  the same reason as the status reason: using a host callback allows the
  guest to control memory allocation. This uses a C-String approach to
  avoid complexity in marshalling. The same approach is used in
  http-wasm for header values.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@sanposhiho
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2023
@k8s-ci-robot k8s-ci-robot merged commit 75d80da into kubernetes-sigs:main Jun 22, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants