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

Create a script + github action for rebaselining tests #23146

Open
sbc100 opened this issue Dec 13, 2024 · 4 comments
Open

Create a script + github action for rebaselining tests #23146

sbc100 opened this issue Dec 13, 2024 · 4 comments
Assignees

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Dec 13, 2024

We have a bunch of codesize tests that need to be rebaselined. This need can occur under 2 different circumstances.

  1. A llvm or binaryen commit rolled in that changes the generated code.
  2. An emscripten PR causes changes to the generated code.

I think it would be good if we can avoid mixing (1) and (2). These means that the main branch should always be updated to include changes from llvm/binaryen before the changes for specific PR are landed (as part of that PR).

For (1) procedure I've been using looks something like this:

$ git checkout -b main upstream/main
$ ./emcc --clear-cache
$ test/runner other.*code_size* other.*codesize* --rebase
$ git add -u .
$ git commit -m "Rebaseline codesize expectations. NFC"
$ git show  # check everything looks reasonable
$ git push upsteam # direct push to upstream, no PR

There are several problems with this (as pointed out in #23126 (comment)).

  1. Its a lot of separate steps
  2. It involves pushing directly to repo, without any review
  3. Its hard to see (using git diff) exactly what effect the changes have.

I propose adding a new script that performs most of the step above that will have the following advantages:

  1. It can analyze the results and present a human readable report (e.g. test xxx regressed by 10 bytes (0.1%)).
  2. It can automatically create a PR with the report in the description.
  3. It can be turned into a github action so it can be run on a remote machine (not a users machine where llvm and binaries version might not be exactly at tot).
@kripken
Copy link
Member

kripken commented Dec 13, 2024

Sounds great!

@brendandahl
Copy link
Collaborator

Yeah, sounds good. It's such a pain having the size tests randomly break and then having to figure out if it's my changes or some llvm/binaryen roll.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 13, 2024

We could even have a github action generate the code size changes for you and add to your PR? That way you wouldn't need to make sure you have the exact tot version locally.

sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 14, 2024
This is just the first iteration.

See emscripten-core#23146 for more plans.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 14, 2024
This is just the first iteration.

See emscripten-core#23146 for more plans.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 14, 2024
This is just the first iteration.

See emscripten-core#23146 for more plans.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 14, 2024
This is just the first iteration.

See emscripten-core#23146 for more plans.
sbc100 added a commit to sbc100/emscripten that referenced this issue Dec 17, 2024
This is just the first iteration.

See emscripten-core#23146 for more plans.
sbc100 added a commit that referenced this issue Dec 17, 2024
This is just the first iteration.

See #23146 for more plans.
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 17, 2024

For usage of the initial version of this script: #23188

hedwigz pushed a commit to hedwigz/emscripten that referenced this issue Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants