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

Performance Regression Support (Manual / CI) #1102

Closed
Jma353 opened this issue Oct 28, 2019 · 4 comments
Closed

Performance Regression Support (Manual / CI) #1102

Jma353 opened this issue Oct 28, 2019 · 4 comments
Assignees
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases C: performance Black is too slow. Or too fast. T: enhancement New feature or request

Comments

@Jma353
Copy link
Contributor

Jma353 commented Oct 28, 2019

Background

black and blackd are very helpful in IDE scenarios for formatting when invoking a keyboard shortcut and formatting on save. However, this means performance of the tool is paramount. A slow formatter can make writing code feel sluggish, especially if you save often or tend to format often.

Performance Regression Testing

I recently was mulling over the idea of performance regression testing on PRs (PR compared to master). While we can think of various ways to improve performance (some ideas are already in #366), ensuring maintained performance is also important. For example, if I'm a contributor and I fix a bug by performing more thorough checks when formatting, and I degrade performance by 10%, I should probably think of another way to fix the bug. Additionally, a tool like this could also quantify impact of changes that actually improve performance.

Open to Suggestions

Wanted to make an issue to get folks' thoughts about this. In particular:

  • Do people like this idea? Should this be a signal contributors get when doing a PR?
  • What CI tools could we use for this?
  • Any other feedback you may have.

cc: @ambv, @zsol, @JelleZijlstra

@Jma353 Jma353 added the T: enhancement New feature or request label Oct 28, 2019
@JelleZijlstra
Copy link
Collaborator

I definitely agree that it's good to ensure we don't regress performance. I don't have great ideas though for how to do this in CI. Things like Travis generally use multi-tenant VMs so it's hard to get reliable performance data from them.

@zsol
Copy link
Collaborator

zsol commented Oct 29, 2019

I found myself wondering about how much I regress performance when merging some (sometimes my own) PRs, so I'm 100% onboard. I share Jelle's concern about doing this reliably as part of CI, but having some kind of easy-to-run test scenario to assess performance changes would be a nice first step.

We could then at least run this tool as part of the release process and figure out if we severely degrade performance before releasing into the wild.

@Jma353
Copy link
Contributor Author

Jma353 commented Oct 29, 2019

I'm all for a quick v0 of this.

When I get some free time this week, I'll RFC my plan in this issue.

@Jma353 Jma353 changed the title Performance Regression CI Support Performance Regression Support (Manual / CI) Oct 29, 2019
@ichard26 ichard26 added the C: performance Black is too slow. Or too fast. label Sep 16, 2020
@ichard26 ichard26 added the C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases label Apr 28, 2021
@ichard26 ichard26 self-assigned this Jun 13, 2021
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jul 1, 2023

This is a reasonable idea, but can be hard to do in CI (can only really reliably detect huge perf regressions). Closing since this issue is stale, but feel free to pick this kind of thing back up!

@hauntsaninja hauntsaninja closed this as not planned Won't fix, can't repro, duplicate, stale Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases C: performance Black is too slow. Or too fast. T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants