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

Speed up listing extensions by lazy-loading extension information when needed #7493

Merged
merged 2 commits into from Jun 14, 2023

Conversation

mislav
Copy link
Contributor

@mislav mislav commented May 26, 2023

Fetching extension information such as its GitHub URL, current version, and pinned property isn't strictly necessary for just listing extensions, so this restructures extensions code to lazy-load this information on ext.URL(), ext.CurrentVersion(), ext.LatestVersion(), and ext.IsPinned(). This generally is a shift in responsibility for these items from the extension manager to the extension itself.

This will speed up gh invocation and shell completion, since extensions are automatically listed when constructing the gh command tree. Followup to #7457

@mislav mislav requested a review from a team as a code owner May 26, 2023 14:53
@mislav mislav requested review from williammartin and removed request for a team May 26, 2023 14:53
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI May 26, 2023
@samcoe
Copy link
Contributor

samcoe commented May 28, 2023

@mislav Thanks for getting started on this work and caring deeply about the performance of gh. My understanding of the performance issue is that when listing extensions we shell out to git to retrieve information for non-binary extensions, these git invocations are slow when compared to reading the manifest files for binary extensions. Is that accurate?

Assuming it is the git invocations that we are really trying to reduce here I have two alternative approaches that I think we could consider here.

  1. When mounting extensions on gh initialization we only rely on the extension name. Right now we use the name and the URL if it is available. Removing the URL would remove the git invocation. We would need to add in a new function to the extension manager to list just the extension names but that would be trivial. This is the simplest idea I could come up with, but slightly sacrifices the UX.

  2. We start writing manifest files for non-binary extensions. When installing/updating an extension we will create manifest files just like we do for binary extensions. Then when listing extensions we can read the manifest files instead of invoking git. I like this approach as it actually brings the two types of extensions more inline with each other. The code for writing manifest files is already in place so I don't see this is a huge lift. The only thing I would suggest is that when listing extensions we try to write a manifest file if one does not exist so that users won't need to do an install/update of an extension to see get the performance improvement.

Thoughts?

@mislav
Copy link
Contributor Author

mislav commented May 30, 2023

  1. Removing the URL would remove the git invocation. We would need to add in a new function to the extension manager to list just the extension names but that would be trivial.

I'd prefer this approach. However, I don't see why we should add yet another list functionality when we already have two (list w/ version metadata vs. list without version metadata). I'd rather reduce our current list function to only list extensions by name, and then when the caller requests more information per extension, those are looked up on request.

2. We start writing manifest files for non-binary extensions.

I appreciate this idea and I don't think it would be hard to do, but honestly I don't see why we should start writing manifest for git extensions when we can already look up information about them locally. We should just avoid looking up this information altogether unless the information is necessary.

The way I envision the lazy-loading approach, we wouldn't even load the binary extension manifest in plain list mode.

@samcoe
Copy link
Contributor

samcoe commented May 31, 2023

I'd prefer this approach. However, I don't see why we should add yet another list functionality when we already have two (list w/ version metadata vs. list without version metadata). I'd rather reduce our current list function to only list extensions by name, and then when the caller requests more information per extension, those are looked up on request.

That's a fair point, the listing functionality with the two functions is already a bit confusing. One option, that may be less work than making this lazy-loaded would be to have the list function take in a list of fields that the caller is requesting being loaded, similar to a GraphQL request. Just an idea.

I appreciate this idea and I don't think it would be hard to do, but honestly I don't see why we should start writing manifest for git extensions when we can already look up information about them locally. We should just avoid looking up this information altogether unless the information is necessary.

Also a good point. We had previously discussed allowing manifest files for extensions to open up some new features for them so I thought this approach would get us on the right track to move that direction. The lazy-loading fix you are creating here does not impede that potential future direction though, so no concerns from me there.

@mislav mislav assigned samcoe and unassigned mislav Jun 9, 2023
@mislav
Copy link
Contributor Author

mislav commented Jun 9, 2023

@samcoe Could you see to finishing the lazy-loading aspect? Basically, the parts that I've commented out in this branch should run on request. Or feel free to go with your own approach if that's simpler. Thanks!

}
// For single extensions manually retrieve latest version since we forgo doing it during list.
if latestVersion := f.LatestVersion(); latestVersion == "" {
return fmt.Errorf("unable to retrieve latest version for extension %q", name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a slight change in behavior, where we will now output a more user friendly error message but with less insight into what might have failed while retrieving the latest version.

Copy link
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

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

Tested it locally and it worked well. I think this is a good step forward in general.

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Jun 13, 2023
@samcoe samcoe merged commit beb6234 into trunk Jun 14, 2023
10 checks passed
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Jun 14, 2023
@samcoe samcoe deleted the ext-lazy-loading branch June 14, 2023 00:33
renovate bot added a commit to scottames/dots that referenced this pull request Jun 23, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [aquaproj/aqua-registry](https://togithub.com/aquaproj/aqua-registry)
| minor | `v4.20.0` -> `v4.21.1` |
| [cli/cli](https://togithub.com/cli/cli) | minor | `v2.30.0` ->
`v2.31.0` |
| [weaveworks/eksctl](https://togithub.com/weaveworks/eksctl) | minor |
`v0.145.0` -> `v0.146.0` |
| [zellij-org/zellij](https://togithub.com/zellij-org/zellij) | patch |
`v0.37.1` -> `v0.37.2` |

---

### Release Notes

<details>
<summary>aquaproj/aqua-registry</summary>

###
[`v4.21.1`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v4.21.1)

[Compare
Source](https://togithub.com/aquaproj/aqua-registry/compare/v4.21.0...v4.21.1)


[Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av4.21.1)
| [Pull
Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av4.21.1)
| aquaproj/aqua-registry@v4.21.0...v4.21.1

#### Fixes


[#&#8203;13199](https://togithub.com/aquaproj/aqua-registry/issues/13199)
kubernetes-sigs/kustomize: Follow up changes of kustomize v5.1.0

-
[kubernetes-sigs/kustomize#5220

###
[`v4.21.0`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v4.21.0)

[Compare
Source](https://togithub.com/aquaproj/aqua-registry/compare/v4.20.0...v4.21.0)


[Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av4.21.0)
| [Pull
Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av4.21.0)
| aquaproj/aqua-registry@v4.20.0...v4.21.0

#### 🎉 New Packages


[#&#8203;13173](https://togithub.com/aquaproj/aqua-registry/issues/13173)
[assetnote/surf](https://togithub.com/assetnote/surf): Escalate your
SSRF vulnerabilities on Modern Cloud Environments. `surf` allows you to
filter a list of hosts, returning a list of viable SSRF candidates

[#&#8203;13174](https://togithub.com/aquaproj/aqua-registry/issues/13174)
[iyear/tdl](https://togithub.com/iyear/tdl): A Telegram downloader
written in Golang

[#&#8203;13172](https://togithub.com/aquaproj/aqua-registry/issues/13172)
[nikolaydubina/go-cover-treemap](https://togithub.com/nikolaydubina/go-cover-treemap):
Go code coverage to SVG treemap
[@&#8203;iwata](https://togithub.com/iwata)

</details>

<details>
<summary>cli/cli</summary>

### [`v2.31.0`](https://togithub.com/cli/cli/releases/tag/v2.31.0):
GitHub CLI 2.31.0

[Compare Source](https://togithub.com/cli/cli/compare/v2.30.0...v2.31.0)

#### What's New

- New suite of `project` commands for interacting with and manipulating
projects. Huge shoutout 🥳 for the time and effort put into this work by
[@&#8203;mntlty](https://togithub.com/mntlty) in
[cli/cli#7375
[cli/cli#7578
- New `search code` command by
[@&#8203;joshkraft](https://togithub.com/joshkraft) in
[cli/cli#7376
- New `cs view` command by
[@&#8203;dmgardiner25](https://togithub.com/dmgardiner25) in
[cli/cli#7496
[cli/cli#7539

#### What's Changed

- `api`: output a single JSON array in REST pagination mode by
[@&#8203;mislav](https://togithub.com/mislav) in
[cli/cli#7190
- `api`: support array params in GET queries by
[@&#8203;mislav](https://togithub.com/mislav) in
[cli/cli#7513
- `api`: force method to uppercase by
[@&#8203;ffalor](https://togithub.com/ffalor) in
[cli/cli#7514
- `alias`: Allow aliases to recognize extended commands by
[@&#8203;srz-zumix](https://togithub.com/srz-zumix) in
[cli/cli#7523
- `alias import`: Fix `--clobber` flag by
[@&#8203;samcoe](https://togithub.com/samcoe) in
[cli/cli#7569
- `run rerun`: Improve docs around `--job` flag by
[@&#8203;williammartin](https://togithub.com/williammartin) in
[cli/cli#7527
- `run view`: Support viewing logs for jobs with composite actions by
[@&#8203;williammartin](https://togithub.com/williammartin) in
[cli/cli#7526
- `gist edit`: Add selector option to `gist edit` command by
[@&#8203;kousikmitra](https://togithub.com/kousikmitra) in
[cli/cli#7537
- `repo clone`: Set upstream remote to track all branches after initial
fetch by [@&#8203;samcoe](https://togithub.com/samcoe) in
[cli/cli#7542
- `extension`: Speed up listing extensions by lazy-loading extension
information when needed by [@&#8203;mislav](https://togithub.com/mislav)
in
[cli/cli#7493
- `auth`: Add timeouts to keyring operations by
[@&#8203;samcoe](https://togithub.com/samcoe) in
[cli/cli#7580
- `auth status`: write to stdout on success by
[@&#8203;rajhawaldar](https://togithub.com/rajhawaldar) in
[cli/cli#7540
- `completion`: Fix bash completions for extensions and aliases by
[@&#8203;mislav](https://togithub.com/mislav) in
[cli/cli#7525
- `issue/pr view`: alphabetically sort labels for `gh pr/issue view` by
[@&#8203;ffalor](https://togithub.com/ffalor) in
[cli/cli#7587
- Fix error handling for extension and shell alias commands by
[@&#8203;samcoe](https://togithub.com/samcoe) in
[cli/cli#7567
- Fix pkg imported more than once by
[@&#8203;testwill](https://togithub.com/testwill) in
[cli/cli#7591
- Refactor a nested if statement by
[@&#8203;yanskun](https://togithub.com/yanskun) in
[cli/cli#7596
- Fix a typo by
[@&#8203;lerocknrolla](https://togithub.com/lerocknrolla) in
[cli/cli#7557
- Fix flaky test by [@&#8203;samcoe](https://togithub.com/samcoe) in
[cli/cli#7515
- Credential rotations, renames and decouplings from Mislav by
[@&#8203;williammartin](https://togithub.com/williammartin) in
[cli/cli#7544
- build(deps): bump github.com/cli/go-gh/v2 from 2.0.0 to 2.0.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[cli/cli#7546
- build(deps): bump github.com/AlecAivazis/survey/v2 from 2.3.6 to 2.3.7
by [@&#8203;dependabot](https://togithub.com/dependabot) in
[cli/cli#7576

#### New Contributors

- [@&#8203;srz-zumix](https://togithub.com/srz-zumix) made their first
contribution in
[cli/cli#7523
- [@&#8203;lerocknrolla](https://togithub.com/lerocknrolla) made their
first contribution in
[cli/cli#7557
- [@&#8203;testwill](https://togithub.com/testwill) made their first
contribution in
[cli/cli#7591
- [@&#8203;rajhawaldar](https://togithub.com/rajhawaldar) made their
first contribution in
[cli/cli#7540

**Full Changelog**: cli/cli@v2.30.0...v2.31.0

</details>

<details>
<summary>weaveworks/eksctl</summary>

###
[`v0.146.0`](https://togithub.com/weaveworks/eksctl/releases/tag/v0.146.0):
eksctl 0.146.0 (permalink)

[Compare
Source](https://togithub.com/weaveworks/eksctl/compare/0.145.0...0.146.0)

### Release v0.146.0

#### 🎯 Improvements

- Update vpc-cni to 1.12.6
([#&#8203;6692](https://togithub.com/weaveworks/eksctl/issues/6692))

#### 🧰 Maintenance

- Bump dependencies
([#&#8203;6690](https://togithub.com/weaveworks/eksctl/issues/6690))

#### 📝 Documentation

- New eksctl channel
([#&#8203;6697](https://togithub.com/weaveworks/eksctl/issues/6697))

#### Acknowledgments

Weaveworks would like to sincerely thank:
[@&#8203;wind0r](https://togithub.com/wind0r)

</details>

<details>
<summary>zellij-org/zellij</summary>

###
[`v0.37.2`](https://togithub.com/zellij-org/zellij/releases/tag/v0.37.2)

[Compare
Source](https://togithub.com/zellij-org/zellij/compare/v0.37.1...v0.37.2)

This is a patch release to fix some minor issues in the previous
release.

#### What's Changed

- hotfix: include theme files into binary by
[@&#8203;jaeheonji](https://togithub.com/jaeheonji) in
[zellij-org/zellij#2566
- fix(plugins): make hide_self api idempotent by
[@&#8203;imsnif](https://togithub.com/imsnif) in
[zellij-org/zellij#2568

**Full Changelog**:
zellij-org/zellij@v0.37.1...v0.37.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/scottames/dots).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMzEuMCIsInVwZGF0ZWRJblZlciI6IjM1LjEzMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Pending Release 🥚
Development

Successfully merging this pull request may close these issues.

None yet

3 participants