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

[Merged by Bors] - Only run benchmarks on PRs when a label is set #2114

Closed
wants to merge 1 commit into from

Conversation

raskad
Copy link
Member

@raskad raskad commented Jun 11, 2022

This changes our ci benchmarks to only run when the label run-benchmark is set on the PR.

The motivation is to reduce the time waiting on benchmarks to run while working on PRs. Also this saves some ci minutes which is always good.
When we spot changes that we suspect impact performance, we can add the run-benchmark label to the PR and the benchmarks will run.

@raskad raskad added github_actions Pull requests that update Github_actions code Internal Category for changelog labels Jun 11, 2022
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Nice! This will definitely speedup our CI process by a lot :)

@github-actions
Copy link

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 90,499 90,499 0
Passed 56,446 56,446 0
Ignored 23,580 23,580 0
Failed 10,473 10,473 0
Panics 0 0 0
Conformance 62.37% 62.37% 0.00%

@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #2114 (5e1a1ae) into main (5002b9b) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2114      +/-   ##
==========================================
- Coverage   43.66%   43.62%   -0.04%     
==========================================
  Files         217      217              
  Lines       19644    19673      +29     
==========================================
+ Hits         8578     8583       +5     
- Misses      11066    11090      +24     
Impacted Files Coverage Δ
boa_engine/src/syntax/ast/node/spread/mod.rs 57.14% <0.00%> (-14.29%) ⬇️
boa_engine/src/builtins/object/mod.rs 59.91% <0.00%> (-6.75%) ⬇️
...x/ast/node/declaration/async_generator_decl/mod.rs 6.66% <0.00%> (-6.67%) ⬇️
...ntax/parser/expression/left_hand_side/arguments.rs 35.00% <0.00%> (-2.50%) ⬇️
boa_engine/src/vm/code_block.rs 45.81% <0.00%> (ø)
boa_engine/src/syntax/ast/node/mod.rs 48.85% <0.00%> (ø)
boa_engine/src/syntax/ast/node/new/mod.rs 37.50% <0.00%> (ø)
boa_engine/src/syntax/ast/node/call/mod.rs 69.23% <0.00%> (ø)
boa_engine/src/syntax/ast/node/array/mod.rs 33.33% <0.00%> (ø)
boa_engine/src/syntax/ast/node/block/mod.rs 50.00% <0.00%> (ø)
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5002b9b...5e1a1ae. Read the comment docs.

@jedel1043
Copy link
Member

I saw the test conformance comment and it occured to me that it would be good to conditionally run parts of the CI only when there has been changes on boa_engine, boa_gc, boa_interner, etc

@raskad
Copy link
Member Author

raskad commented Jun 11, 2022

I saw the test conformance comment and it occured to me that it would be good to conditionally run parts of the CI only when there has been changes on boa_engine, boa_gc, boa_interner, etc

True. I think there is some discussion needed on this, as some changes outside of the Rust crates probably also have to be tested. For example if we update the 262 submodule, we should probably run ci normally.

@Razican
Copy link
Member

Razican commented Jun 11, 2022

The thing is that a change to, for example, the Unicode dependency, could make some random stuff to fail, and performance to change in any place (almost). So it's difficult to know when it makes sense or not.

@raskad
Copy link
Member Author

raskad commented Jun 11, 2022

The thing is that a change to, for example, the Unicode dependency, could make some random stuff to fail, and performance to change in any place (almost). So it's difficult to know when it makes sense or not.

That is true, but I would argue that those cases should not be the norm and we still have the full benchmarks on main. If any change unexpectedly changes performance, we can still track it down to the commit. IMO the benefits of getting faster feedback on PRs outweigh the negatives.

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

I’m assuming this works at the point when you add the label?

@raskad
Copy link
Member Author

raskad commented Jun 11, 2022

I’m assuming this works at the point when you add the label?

Yes, when you add the label it gets triggered for the first time. Any push after that also triggers them like in the past.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@Razican Razican added this to the v0.16.0 milestone Jun 12, 2022
@Razican
Copy link
Member

Razican commented Jun 12, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jun 12, 2022
This changes our ci benchmarks to only run when the label `run-benchmark` is set on the PR.

The motivation is to reduce the time waiting on benchmarks to run while working on PRs. Also this saves some ci minutes which is always good.
When we spot changes that we suspect impact performance, we can add the `run-benchmark` label to the PR and the benchmarks will run.
@bors
Copy link

bors bot commented Jun 12, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Only run benchmarks on PRs when a label is set [Merged by Bors] - Only run benchmarks on PRs when a label is set Jun 12, 2022
@bors bors bot closed this Jun 12, 2022
@bors bors bot deleted the run-benchmarks-labeled branch June 12, 2022 12:03
bors bot pushed a commit that referenced this pull request Jun 13, 2022
This changes the trigger type for PR benchmarks back to the default (`opened`, `synchronize`, `reopened`).  As part of #2114 I added the `labeled` trigger type. This causes the benchmarks to run when the `run-benchmark` label is present and another label is added.
For example in #2116 I added the `run-benchmark` label while creating the PR. The benchmarks then where triggered six times; one for the PR creation (`opened`) and five times for each label that I initially added to the PR.

The only drawback is that the benchmarks are not triggered, when we just add the label, but unfortunately I don't have a clever idea on how to achieve that right now. We will have to add the label and then trigger the run via a `synchronize` (push).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code Internal Category for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants