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.

Sign up for GitHub

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

Jump to bottom

Debounce calls to git branch #3887

Merged
merged 1 commit into from
May 16, 2023
Merged

Debounce calls to git branch #3887

merged 1 commit into from
May 16, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented May 15, 2023 •
edited
Loading

Whilst testing with the output channel open I noticed this:

[version: 0.8.11, 2023-05-15T04:03:47.737Z, pid: 99240] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.738Z, pid: 99245] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.739Z, pid: 99250] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.741Z, pid: 99255] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.742Z, pid: 99260] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.744Z, pid: 99265] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.745Z, pid: 99270] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.749Z, pid: 99275] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.752Z, pid: 99280] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.754Z, pid: 99285] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.756Z, pid: 99290] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.758Z, pid: 99295] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.761Z, pid: 99300] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.764Z, pid: 99305] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.766Z, pid: 99310] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.767Z, pid: 99315] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.768Z, pid: 99320] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.770Z, pid: 99325] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.772Z, pid: 99330] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.773Z, pid: 99335] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.776Z, pid: 99340] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.778Z, pid: 99345] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.780Z, pid: 99350] > git branch --no-merge - INITIALIZED
[version: 0.8.11, 2023-05-15T04:03:47.800Z, pid: 99234] > git branch --no-merge - COMPLETED (68ms)
[version: 0.8.11, 2023-05-15T04:03:47.801Z, pid: 99240] > git branch --no-merge - COMPLETED (66ms)
[version: 0.8.11, 2023-05-15T04:03:47.801Z, pid: 99245] > git branch --no-merge - COMPLETED (64ms)
[version: 0.8.11, 2023-05-15T04:03:47.801Z, pid: 99250] > git branch --no-merge - COMPLETED (63ms)
[version: 0.8.11, 2023-05-15T04:03:47.801Z, pid: 99255] > git branch --no-merge - COMPLETED (61ms)
[version: 0.8.11, 2023-05-15T04:03:47.801Z, pid: 99260] > git branch --no-merge - COMPLETED (60ms)
[version: 0.8.11, 2023-05-15T04:03:47.802Z, pid: 99265] > git branch --no-merge - COMPLETED (59ms)
[version: 0.8.11, 2023-05-15T04:03:47.802Z, pid: 99270] > git branch --no-merge - COMPLETED (58ms)
[version: 0.8.11, 2023-05-15T04:03:47.802Z, pid: 99275] > git branch --no-merge - COMPLETED (55ms)
[version: 0.8.11, 2023-05-15T04:03:47.802Z, pid: 99280] > git branch --no-merge - COMPLETED (53ms)
[version: 0.8.11, 2023-05-15T04:03:47.802Z, pid: 99285] > git branch --no-merge - COMPLETED (50ms)
[version: 0.8.11, 2023-05-15T04:03:47.803Z, pid: 99290] > git branch --no-merge - COMPLETED (49ms)
[version: 0.8.11, 2023-05-15T04:03:47.803Z, pid: 99295] > git branch --no-merge - COMPLETED (46ms)
[version: 0.8.11, 2023-05-15T04:03:47.803Z, pid: 99300] > git branch --no-merge - COMPLETED (44ms)
[version: 0.8.11, 2023-05-15T04:03:47.804Z, pid: 99305] > git branch --no-merge - COMPLETED (42ms)
[version: 0.8.11, 2023-05-15T04:03:47.804Z, pid: 99310] > git branch --no-merge - COMPLETED (40ms)
[version: 0.8.11, 2023-05-15T04:03:47.804Z, pid: 99315] > git branch --no-merge - COMPLETED (38ms)
[version: 0.8.11, 2023-05-15T04:03:47.807Z, pid: 99320] > git branch --no-merge - COMPLETED (40ms)
[version: 0.8.11, 2023-05-15T04:03:47.808Z, pid: 99325] > git branch --no-merge - COMPLETED (39ms)
[version: 0.8.11, 2023-05-15T04:03:47.808Z, pid: 99330] > git branch --no-merge - COMPLETED (37ms)
[version: 0.8.11, 2023-05-15T04:03:47.808Z, pid: 99335] > git branch --no-merge - COMPLETED (36ms)
[version: 0.8.11, 2023-05-15T04:03:47.808Z, pid: 99340] > git branch --no-merge - COMPLETED (33ms)
[version: 0.8.11, 2023-05-15T04:03:47.809Z, pid: 99345] > git branch --no-merge - COMPLETED (32ms)
[version: 0.8.11, 2023-05-15T04:03:47.809Z, pid: 99350] > git branch --no-merge - COMPLETED (30ms)

This PR denounces calls to git branch --no-merge by moving them into managedUpdate in the ExperimentsData class. If you can foresee any problems with this approach then PLMK.

@mattseddon mattseddon added the bug Something isn't working label May 15, 2023
@mattseddon mattseddon self-assigned this May 15, 2023
mattseddon
mattseddon commented May 15, 2023
View reviewed changes
extension/src/experiments/data/index.ts
@@ -41,6 +41,7 @@ export class ExperimentsData extends BaseData<ExpShowOutput> {
}

public async update(): Promise<void> {
void this.updateAvailableBranchesToSelect()
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] FWIW moving this in here means that

vscode-dvc/extension/src/process/manager.ts

Lines 53 to 58 in d92aa52

private shouldDebounce(lastStarted: number | undefined) {
return (
lastStarted &&
getCurrentEpoch() - lastStarted < DEFAULT_DEBOUNCE_WINDOW_MS
)
}
takes effect

Copy link
Contributor

Choose a reason for hiding this comment

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

I did the same thing for what I'm currently working on (cleaning up the table state when changing branches). It makes more sense as well.

@mattseddon mattseddon marked this pull request as ready for review May 15, 2023 06:38
@mattseddon mattseddon requested review from sroy3 and julieg18 as code owners May 15, 2023 06:38
@mattseddon mattseddon force-pushed the debounce-update-available-branches branch from d92aa52 to 7a87ce2 Compare May 15, 2023 06:38
sroy3
sroy3 approved these changes May 15, 2023
View reviewed changes
extension/src/experiments/data/index.ts
@@ -41,6 +41,7 @@ export class ExperimentsData extends BaseData<ExpShowOutput> {
}

public async update(): Promise<void> {
void this.updateAvailableBranchesToSelect()
Copy link
Contributor

Choose a reason for hiding this comment

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

I did the same thing for what I'm currently working on (cleaning up the table state when changing branches). It makes more sense as well.

@mattseddon mattseddon force-pushed the debounce-update-available-branches branch from 7a87ce2 to 1d01044 Compare May 15, 2023 21:34
@mattseddon mattseddon enabled auto-merge (squash) May 15, 2023 21:34
@mattseddon mattseddon force-pushed the debounce-update-available-branches branch from 1d01044 to 40368b5 Compare May 15, 2023 23:39
@codeclimate
Copy link

codeclimate bot commented May 15, 2023

Code Climate has analyzed commit 40368b5 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.9% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 89140c5 into main May 16, 2023
@mattseddon mattseddon deleted the debounce-update-available-branches branch May 16, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julieg18 julieg18 julieg18 approved these changes

@sroy3 sroy3 sroy3 approved these changes

Assignees

@mattseddon mattseddon

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattseddon @sroy3 @julieg18