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

Measure input latency and text render time #163184

Merged
merged 18 commits into from
Nov 14, 2022
Merged

Measure input latency and text render time #163184

merged 18 commits into from
Nov 14, 2022

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Oct 10, 2022

WIP - This may not get merged

Part of #161622

@Tyriar Tyriar added this to the October 2022 milestone Oct 10, 2022
@Tyriar Tyriar self-assigned this Oct 10, 2022
@Tyriar Tyriar requested a review from alexdima October 24, 2022 12:43
@Tyriar
Copy link
Member Author

Tyriar commented Oct 24, 2022

@alexdima thoughts on the approach? Probably won't merge this for October as time is a bit tight and I'd want to see if anything goes wrong in Insiders first

@Tyriar Tyriar modified the milestones: October 2022, November 2022 Oct 24, 2022
@alexdima
Copy link
Member

@Tyriar Let's look at this next debt week.

@alexdima alexdima removed their request for review October 24, 2022 19:42
@Tyriar
Copy link
Member Author

Tyriar commented Oct 24, 2022

@alexdima I'll be out all next week fyi, but can meet or react to comments the first week of dev

@alexdima alexdima self-assigned this Oct 24, 2022
@Tyriar Tyriar marked this pull request as ready for review November 11, 2022 00:41
@Tyriar Tyriar requested review from alexdima and lramos15 November 11, 2022 00:41
@Tyriar
Copy link
Member Author

Tyriar commented Nov 11, 2022

@alexdima this is ready for a look

@lramos15 can you review the telemetry event?

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

I like the idea. Some high level comments:

  • IMHO we should move away from using the browser's performance and do the math on our won.
  • We don't need a buffer, we can compute a rolling average by adding things up at each moment and dividing at the end (i.e. each data structure needs to keep min, max, avgSum and avgCount). Then there is no need to allocate memory or loop over items or worry about overflows.

src/vs/editor/browser/controller/textAreaInput.ts Outdated Show resolved Hide resolved
src/vs/base/browser/performance.ts Show resolved Hide resolved
src/vs/base/browser/performance.ts Outdated Show resolved Hide resolved
src/vs/base/browser/performance.ts Outdated Show resolved Hide resolved
src/vs/base/browser/performance.ts Outdated Show resolved Hide resolved
src/vs/base/browser/performance.ts Outdated Show resolved Hide resolved
src/vs/base/browser/performance.ts Outdated Show resolved Hide resolved
@alexdima alexdima requested a review from jrieken November 11, 2022 16:05
Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

Telemetry side of things looks good to me 👍. Do we plan to collect this indefinitely?

@Tyriar
Copy link
Member Author

Tyriar commented Nov 14, 2022

@lramos15 yes

Copy link
Member Author

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @alexdima 👍

@Tyriar Tyriar enabled auto-merge November 14, 2022 20:43
@Tyriar Tyriar merged commit f952317 into main Nov 14, 2022
@Tyriar Tyriar deleted the tyriar/measure_latency branch November 14, 2022 20:43
Tyriar added a commit that referenced this pull request Nov 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants