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

Post results from benchmark as a comment on PRs #481

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

vinistock
Copy link
Member

Motivation

Final part of #335

Post the results of running benchmarks as a comment on the PR.

Implementation

  1. Push the STDOUT results of running bin/benchmark into the GitHub actions output variable
  2. Use github-script to take that output and post it as a comment

@vinistock vinistock requested a review from a team as a code owner February 3, 2023 18:39
@vinistock vinistock self-assigned this Feb 3, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch 3 times, most recently from c794298 to 1c355c1 Compare February 3, 2023 18:53
@Shopify Shopify deleted a comment from github-actions bot Feb 3, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch from 1c355c1 to 4fe14eb Compare February 3, 2023 18:57
@Shopify Shopify deleted a comment from github-actions bot Feb 3, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch from 4fe14eb to 6a1a1f0 Compare February 3, 2023 19:03
@Shopify Shopify deleted a comment from github-actions bot Feb 3, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch from 6a1a1f0 to ada7f2e Compare February 3, 2023 19:12
@Shopify Shopify deleted a comment from github-actions bot Feb 3, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch 3 times, most recently from db9489c to 07c2d62 Compare February 3, 2023 19:34
@Shopify Shopify deleted a comment from github-actions bot Feb 3, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch 2 times, most recently from a99549f to bd98626 Compare February 3, 2023 19:50
@Shopify Shopify deleted a comment from github-actions bot Feb 3, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch 2 times, most recently from 8a078e3 to 0bcda2b Compare February 3, 2023 20:09
@bitwise-aiden
Copy link
Contributor

Looking at the comment this action posted above, I'm a little worried that we'll start caching the fastest runs, but that we unnecessarily fail later runs that run normally. The 15% might not be enough.

textDocument/hover faster by 58.27626493431508 %

@vinistock
Copy link
Member Author

Yes, I have the same concern. It seems that there's a lot of variation between each run. I'm not sure how we can achieve the best results, my only idea would be to switch the 15% for something like "faster or slower than 2 standard deviations".

What do you think? Any other ideas?

@andyw8
Copy link
Contributor

andyw8 commented Feb 6, 2023

I see in bin/benchmark we're using Benchmark.realtime, but wouldn't Benchmark.measure be more accurate since it provides the CPU time?

@vinistock
Copy link
Member Author

Measure returns it split by system and user CPU time. Would the right measure be the sum of both? Or just user?

@andyw8
Copy link
Contributor

andyw8 commented Feb 6, 2023

I think it should be both since changes could potentially affect either.

@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch 2 times, most recently from ef9bc00 to 9531aef Compare February 15, 2023 16:12
@Shopify Shopify deleted a comment from github-actions bot Feb 15, 2023
@vinistock
Copy link
Member Author

I've made some changes to try to make the benchmark more stable. It now

  • Considers any different smaller than a standard deviation as unchanged
  • Uses only user cpu time (time spent in Ruby)
  • Bumped the number of iterations
  • Did some preloading of things that could impact results
  • Changed the position and added some source:// comments, so that DocumentLink and Hover have actual results to process

@Shopify Shopify deleted a comment from github-actions bot Feb 15, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch from 9531aef to d99b018 Compare February 15, 2023 16:20
@Shopify Shopify deleted a comment from github-actions bot Feb 15, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch from d99b018 to 0572af7 Compare February 15, 2023 16:21
@Shopify Shopify deleted a comment from github-actions bot Feb 15, 2023
@Shopify Shopify deleted a comment from github-actions bot Feb 15, 2023
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch from 0572af7 to 67e6d37 Compare February 15, 2023 16:47
@vinistock vinistock force-pushed the vs/post_benchmark_results_as_comment branch from 67e6d37 to d75c92d Compare February 15, 2023 16:49
@github-actions
Copy link
Contributor

Benchmark results in seconds (slowest at top)

          textDocument/formatting average: 0.103217 std_dev: 0.027215
          textDocument/codeAction average: 0.038137 std_dev: 0.001258
          textDocument/diagnostic average: 0.037928 std_dev: 0.001712
      textDocument/selectionRange average: 0.003677 std_dev: 0.000796
        textDocument/foldingRange average: 0.001611 std_dev: 6.8e-05
   textDocument/documentHighlight average: 0.001526 std_dev: 0.000131
 textDocument/semanticTokens/full average: 0.000817 std_dev: 0.000176
textDocument/semanticTokens/range average: 0.000623 std_dev: 3.1e-05
               textDocument/hover average: 0.000598 std_dev: 9.0e-05
        textDocument/documentLink average: 0.00053 std_dev: 3.4e-05
           textDocument/inlayHint average: 0.000412 std_dev: 3.7e-05
      textDocument/documentSymbol average: 0.000255 std_dev: 5.4e-05
    textDocument/onTypeFormatting average: 0.000111 std_dev: 8.8e-05


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full faster by 60.47 %
textDocument/semanticTokens/range faster by 45.72 %
      textDocument/documentSymbol faster by 35.548 %
        textDocument/foldingRange faster by 48.846 %
          textDocument/formatting unchanged
          textDocument/diagnostic faster by 10.681 %
        textDocument/documentLink slower by 15.052 %
           textDocument/inlayHint faster by 11.703 %
      textDocument/selectionRange faster by 40.368 %
   textDocument/documentHighlight faster by 41.485 %
               textDocument/hover faster by 52.622 %
          textDocument/codeAction faster by 11.115 %
    textDocument/onTypeFormatting unchanged


At least one benchmark is slower than the main branch.

@vinistock
Copy link
Member Author

@dirceu gave a great suggestion to run both the main and the branch benchmark on the same CI run to avoid some variance. This also allows us to get rid of using cache.

Currently, I believe the benchmark will always fail because I added some extra things to the fixture, but once this is merged it should be considerably more stable.

@vinistock vinistock merged commit 3e2369e into main Feb 15, 2023
@vinistock vinistock deleted the vs/post_benchmark_results_as_comment branch February 15, 2023 19:30
@shopify-shipit shopify-shipit bot temporarily deployed to production February 15, 2023 20:50 Inactive
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.

6 participants