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

ci: add github workflow for performance regression check #95

Merged
merged 9 commits into from
Oct 21, 2021

Conversation

SilverRainZ
Copy link
Member

@SilverRainZ SilverRainZ commented Oct 9, 2021

This PR add a github workflow for performance regression check (using benchdiff, a wrapper of benchstat).

Close #28.

Usage

The workflow will be triggered:

  • when a PR is created, or
  • anyone comments "/benchdiff" on PR

It takes a few minutes to run (the time is depends on the go packages modified by this PR), once it is finished, benchdiff result will be posted on this PR:

image

Non-full amount

This workflow won't do a full amount benchmarking, it only do bench on modified go packages (get by git diff <default_branch> <head_of_pr>, for details, refer to jobs.benchdiff.steps.head). In the aboved case, only packages collection/skipset and collection/skipmap are modified. If there is no go package modified, the following comment will be posted:

image

Github Checks

This workflow also creates a check for the HEAD commit of PR.

image

The details of check looks like this:

image

@luchsh
Copy link
Collaborator

luchsh commented Oct 11, 2021

/benchdiff

@SilverRainZ
Copy link
Member Author

/benchdiff

It takes effect after this PR is merged to default branch (develop).

@luchsh
Copy link
Collaborator

luchsh commented Oct 11, 2021

/benchdiff

it does not work for this issue :(

@luchsh luchsh closed this Oct 11, 2021
@luchsh luchsh reopened this Oct 11, 2021
@SilverRainZ
Copy link
Member Author

it does not work for this issue :(

Yes, see my comment aboved. You can try this PR: SilverRainZ#3

@luchsh
Copy link
Collaborator

luchsh commented Oct 11, 2021

it does not work for this issue :(

Yes, see my comment aboved. You can try this PR: SilverRainZ#3

I'm trying to trigger the action multiple times by keep commenting '/benchdiff', let's see if the results are stable enough. (I think this is crucial for a performance-oriented action)

@SilverRainZ SilverRainZ marked this pull request as ready for review October 11, 2021 08:58
@SilverRainZ
Copy link
Member Author

@luchsh @zhangyunhao116 @PureWhiteWu This PR is ready for review, please take a look.

- name: Setup go
uses: actions/setup-go@v2
with:
go-version: 1.16
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could setup the os by os: [ubuntu-latest] ? I wonder if we can also use something like go-latest instead of 1.16 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that action/setup-go supports semver, ^1.x should the the thing we need.

This PR use 1.16 because it is used in all of other workflows. If we use ^1.x, it would be better to update all of them.

$ grep -r go-version
codeql-analysis.yml:          go-version: 1.16
push-check.yml:          go-version: 1.16
pr-benchdiff.yml:          go-version: 1.16
pr-check.yml:          go-version: 1.16

--packages="${{ steps.head.outputs.pkgs }}"
--count=10
--warmup-count=1
--benchtime=100000x
Copy link
Member

Choose a reason for hiding this comment

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

The benchtime may not be suitable for all packages, some packages will take a long time for each bench, it is configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may not be easy, --benchtime take effects for all --packages, and both them are action-level configuration, it is not easy to run a action for every single package.

If we can make sure that we changes at most one package in a PR, we can create a mapping from package name to --benchtime (even more bench configuration), thing will be easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any bench configuraion suitable for all packages? per-package configuration is not easy for now.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe no... but benchtime=1s is ok for ~70% packages

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, let's use benchtime=1s at least in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

luchsh
luchsh previously approved these changes Oct 11, 2021
luchsh
luchsh previously approved these changes Oct 21, 2021
zhangyunhao116
zhangyunhao116 previously approved these changes Oct 21, 2021
@SilverRainZ SilverRainZ enabled auto-merge (squash) October 21, 2021 08:06
@SilverRainZ SilverRainZ merged commit 9ad039f into develop Oct 21, 2021
@SilverRainZ SilverRainZ deleted the ci/benchdiff branch October 21, 2021 08:23
zhangyunhao116 added a commit that referenced this pull request Jan 18, 2022
* release: 20210913 (#83) (#88)

* feat: circuitbreaker.panel use skipmap (#78)

* fix(metainfo): fix misuse of append (#79)

* fix(lscq): add write barrier for LSCQ Pointer (#80)

* feat(metainfo): improve backward APIs (#81)

* release: 20210913 (#83) (#93)

* feat: circuitbreaker.panel use skipmap (#78)

* fix(metainfo): fix misuse of append (#79)

* fix(lscq): add write barrier for LSCQ Pointer (#80)

* feat(metainfo): improve backward APIs (#81)

* chore(skipmap,skipset): remove duplicated code generation declaration (#87)

* fix(lscq): cas2 use runtime.noescape (#94)

* feat: add fastrand.Read() (#90)

Co-authored-by: liyichao <liyichao@bytedance.com>

* doc: add badge to link to godoc (#99)

* ci: skip golang related workflows when there are only doc changes (#101)

* ci: add workflow for feishu/lark notification (#100)

* ci: add workflow for feishu/lark notification

* ci: fix typo

* ci: also send feishu notification on issue opened

Co-authored-by: Jonathan Lu <luchsh@users.noreply.github.com>

* feat(metainfo): define standard for backward prefix (#102)

* feat(fastrand): support Read,Shuffle,Perm (#103)

* feat(fastrand): Read remove temp buffer (#104)

* ci: add github workflow for performance regression check (#95)

* ci: add github workflow for performance regression check

Comment "/benchdiff" to trigger this check

* ci: add comments to pr-benchdiff.yml

* ci: report benchdiff result to PR_HEAD's check run

* ci: various changes

- support "pull_reqeust" event
- filter deleted go packages
- post a comment on job started
- post a comment on job failed

* ci: let xargs ignore empty line

* ci: skip benchdiff when there are only doc changes

* ci: set -x for debugging

* ci: make sure we are using github default branch as baseline

* ci: set benchtime=1s for benchdiff

* chore: add github issue forms (#105)

* test(lang/syncx): add benchmark for RWMutex (#89)

* fix(xxhash3): add fallback to fix panic occurs on non avx2, non sse2 machine (#108)

Co-authored-by: Ye Li <liye.e@bytedance.com>

* feat: add zset (#98)

* feat: add zset

* chore(zset): update comment

* docs(zset): add readme

* chore: some code style fixes

* chore(zset): rename Float64RangeOpt -> RangeOpt

* chore(zset): comment style fixes

* chore(zset): add a todo about maxLevel

* chore(zset): another comment fix

* chore(zset): move skiplist impl to another file

* chore(zset): add license header for opt.go

* chore: add myself as zset's CODEOWNER

* zset: don't use z.Range(0, z.Len()-1)

* doc: remove redundant section

* fix(zset): break when key is not exist

* chore(zset): simplify func name

* chore(zset): update cheatsheet

* chore(zset): meaningful const

* refactor(zset): optionalArray.init()

* docs: update zset readme

* docs(zset): also add @zhangyunhao116 as code owner

* docs(zset): some grammar fixes

* docs: fix docs typo (#109)

* feat: auto tuning gc (#112)

* feat: auto tuning gc

* doc: gctuner

* fix: gctuner tests data race (#113)

* fix: gctuner tests data race

* chore: add owner

* chore(ci): use self-hosted runner (#114)

* chore(ci): use self-hosted runner

* fix lint

Co-authored-by: Shengyu Zhang <reg@silverrainz.me>
Co-authored-by: ziposcar <499581494@qq.com>
Co-authored-by: liyichao <liyichao@bytedance.com>
Co-authored-by: Shengyu Zhang <zhangshengyu.0@bytedance.com>
Co-authored-by: Jonathan Lu <luchsh@users.noreply.github.com>
Co-authored-by: Pure White <wudi.daniel@bytedance.com>
Co-authored-by: lyeeeeee <awesomeyeli@gmail.com>
Co-authored-by: Ye Li <liye.e@bytedance.com>
Co-authored-by: chyroc <chyroc@qq.com>
Co-authored-by: Joway <joway.w@gmail.com>
Co-authored-by: Joway <wangzhuowei@bytedance.com>
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

Successfully merging this pull request may close these issues.

Benchmark step in .github/workflows/pr-check looks like nop
3 participants