Skip to content

docs: adds RATIONALE.md with section on why TinyGo - #28

Merged
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
codefromthecrypt:initial-rationale
Jun 5, 2023
Merged

docs: adds RATIONALE.md with section on why TinyGo#28
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
codefromthecrypt:initial-rationale

Conversation

@codefromthecrypt

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This reverse documents a decision we made about TinyGo.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

documents the most important part of #6

Does this PR introduce a user-facing change?

NONE

What are the benchmark results of this change?

N/A

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jun 4, 2023
@k8s-ci-robot
k8s-ci-robot requested a review from sanposhiho June 4, 2023 02:17
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 4, 2023
@codefromthecrypt

Copy link
Copy Markdown
Contributor Author

ps you may wonder why have RATIONALE.md vs a separate website etc. For a fast moving project, it is more coherent to have changes self-contained. For example, a decision and its rationale in the same commit ideally. When things settle down, we can easily harvest these docs to a separate web page, if it is required to host them externally. Meanwhile, it encourages documentation of decisions otherwise left to PR comments because the follow-up overhead penalty adds friction and work.

@ffromani

ffromani commented Jun 4, 2023

Copy link
Copy Markdown

/cc

@k8s-ci-robot
k8s-ci-robot requested a review from ffromani June 4, 2023 06:22

@sanposhiho sanposhiho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great, they are very worthwhile to know.

/lgtm
/assign @kerthcet

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2023
@codefromthecrypt

Copy link
Copy Markdown
Contributor Author

once this is in, I'll rebase #25 and add the rationale for nottinygc as that is also not intuitive unless you had our experiences.

@kerthcet

kerthcet commented Jun 5, 2023

Copy link
Copy Markdown
Contributor

part of #14

Comment thread guest/RATIONALE.md

### Performance

As described above, the only way to support Go 1.21 would be via the subprocess

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't quite understand the difference of subprocess model with above the go runtime and memory needs to be recreated per request, can you explain more about this @codefromthecrypt thanks.

@codefromthecrypt codefromthecrypt Jun 5, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so the webassembly analogy for a subprocess model (fork a normal OS binary), is instantiating a new module per request.

First wazero needs to create an instance of the wasm binary and there is overhead for that including setting up functions and allocating a new slice of memory (minimally 128KB for tinygo).

Then, what tinygo compiled the go runtime as needs to happen (the part before main is called). Also anything after main returns needs to happen. So all that overhead around the actual function is what the subprocess model implies.

To illustrate this, I have a simple cat program that writes a small file to stdout. The part highlighted in grey box is the actual critical code, though some functions to the left could be argued as critical (lazy initialization happening of filesystem stuff). Everything else is setup/teardown overhead of a go executable compiled to wasm/wasi.

Screenshot 2023-06-05 at 15 18 57

Comment thread guest/RATIONALE.md Outdated
Co-authored-by: Kante Yin <kerthcet@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2023

@kerthcet kerthcet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm
/approve
/label tide/merge-method-squash
Thanks @codefromthecrypt

@k8s-ci-robot k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 5, 2023
@k8s-ci-robot

Copy link
Copy Markdown
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

Details 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 5, 2023
@k8s-ci-robot
k8s-ci-robot merged commit abaa2e0 into kubernetes-sigs:main Jun 5, 2023
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/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants