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

perf(github): improve gh start time by skipping lookup table generation #961

Closed
wants to merge 1 commit into from

Conversation

vegerot
Copy link
Contributor

@vegerot vegerot commented Oct 4, 2024

Summary:
This change improves the start time of gh by skipping the generation of the
lookup table in tcell, a dependency of gh. This is done by setting the
TCELL_MINIMIZE environment variable when calling gh.

Here is a benchmark for a frequent command we run

$ hyperfine -w2 \
  'gh auth status --hostname github.com' \
  'TCELL_MINIMIZE=1 gh auth status --hostname github.com'

Benchmark 1: gh auth status --hostname github.com
  Time (mean ± σ):     406.5 ms ±  13.4 ms    [User: 63.9 ms, System: 25.6 ms]
  Range (min … max):   389.0 ms … 430.3 ms    10 runs

Benchmark 2: TCELL_MINIMIZE=1 gh auth status --hostname github.com
  Time (mean ± σ):     380.6 ms ±  10.9 ms    [User: 42.8 ms, System: 27.3 ms]
  Range (min … max):   364.1 ms … 404.2 ms    10 runs

Summary
  TCELL_MINIMIZE=1 gh auth status --hostname github.com ran
    1.07 ± 0.05 times faster than gh auth status --hostname github.com

I tried measuring the impact of this change on sl pr list but it's too noisy.
Are there any better examples?

$ hyperfine -w3 'CHGDISABLE=1 sl pr list' 'CHGDISABLE=1 ./sl pr list'
Benchmark 1: CHGDISABLE=1 sl pr list
  Time (mean ± σ):     772.0 ms ± 193.8 ms    [User: 227.0 ms, System: 55.2 ms]
  Range (min … max):   664.2 ms … 1313.6 ms    10 runs

Benchmark 2: CHGDISABLE=1 ./sl pr list
  Time (mean ± σ):     741.8 ms ±  29.5 ms    [User: 242.1 ms, System: 68.9 ms]
  Range (min … max):   689.8 ms … 778.1 ms    10 runs

Summary
  CHGDISABLE=1 ./sl pr list ran
    1.04 ± 0.26 times faster than CHGDISABLE=1 sl pr list

…tion

Summary:
This change improves the start time of gh by skipping the generation of the
lookup table in tcell, a dependency of gh. This is done by setting the
`TCELL_MINIMIZE` environment variable when calling `gh`.

Here is a benchmark for a frequent command we run

```sh
$ hyperfine -w2 \
  'gh auth status --hostname github.com' \
  'TCELL_MINIMIZE=1 gh auth status --hostname github.com'

Benchmark 1: gh auth status --hostname github.com
  Time (mean ± σ):     406.5 ms ±  13.4 ms    [User: 63.9 ms, System: 25.6 ms]
  Range (min … max):   389.0 ms … 430.3 ms    10 runs

Benchmark 2: TCELL_MINIMIZE=1 gh auth status --hostname github.com
  Time (mean ± σ):     380.6 ms ±  10.9 ms    [User: 42.8 ms, System: 27.3 ms]
  Range (min … max):   364.1 ms … 404.2 ms    10 runs

Summary
  TCELL_MINIMIZE=1 gh auth status --hostname github.com ran
    1.07 ± 0.05 times faster than gh auth status --hostname github.com
```

I tried measuring the impact of this change on `sl pr list` but it's too noisy.
Are there any better examples?

```sh
$ hyperfine -w3 'CHGDISABLE=1 sl pr list' 'CHGDISABLE=1 ./sl pr list'
Benchmark 1: CHGDISABLE=1 sl pr list
  Time (mean ± σ):     772.0 ms ± 193.8 ms    [User: 227.0 ms, System: 55.2 ms]
  Range (min … max):   664.2 ms … 1313.6 ms    10 runs

Benchmark 2: CHGDISABLE=1 ./sl pr list
  Time (mean ± σ):     741.8 ms ±  29.5 ms    [User: 242.1 ms, System: 68.9 ms]
  Range (min … max):   689.8 ms … 778.1 ms    10 runs

Summary
  CHGDISABLE=1 ./sl pr list ran
    1.04 ± 0.26 times faster than CHGDISABLE=1 sl pr list
```
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in c669d66.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants