Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Performance: Lazily syntax highlight blob views#39563

Merged
umpox merged 10 commits into
mainfrom
tr/blob-perf
Aug 3, 2022
Merged

Performance: Lazily syntax highlight blob views#39563
umpox merged 10 commits into
mainfrom
tr/blob-perf

Conversation

@umpox

@umpox umpox commented Jul 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note
For reviewer: This started pretty experimental, but I think it could be a useful performance UX win.
New to this part of the code base so please let me know if I should be doing something differently!

Description

This PR adds an experimental feature that:

  • Fetches and renders formatted but unhighlighted code blobs. We keep the formatting to ensure we don't have a large layout shift when the backend finally returns.
  • Disables the loading spinner on the blob view. I think we can now reasonably say that almost every file (?) should be returned almost immediately, so we shouldn't need this.

Example 1: Compared against the current blob view and syntax highlighting

Noticeable on most files, much less time waiting to view code.

LazyExample1.mp4

Example 2: Compared against CodeMirror and the newer syntax highlighting

Mainly noticeable on medium/large files, still valuable I think. New syntax highlighting is pretty awesome :)

LazyExample1-1.mp4

Test plan

Tested locally with the flag enabled and disabled

@cla-bot cla-bot Bot added the cla-signed label Jul 28, 2022
@github-actions github-actions Bot added the frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. label Jul 28, 2022
This was referenced Jul 28, 2022
Closed
Closed
@umpox umpox marked this pull request as ready for review August 1, 2022 14:11
@umpox umpox requested review from a team, eseliger, fkling, olafurpg, oleggromov and tjdevries August 1, 2022 14:18
eseliger
eseliger approved these changes Aug 1, 2022
View reviewed changes
Comment thread cmd/frontend/internal/highlight/highlight.go
Comment thread client/web/src/repo/blob/backend.ts Outdated
Comment thread client/web/src/repo/blob/backend.ts Outdated
Comment thread cmd/frontend/graphqlbackend/virtual_file.go Outdated
Comment thread cmd/frontend/graphqlbackend/schema.graphql Outdated
Comment thread cmd/frontend/graphqlbackend/schema.graphql Outdated
valerybugakov
valerybugakov reviewed Aug 2, 2022
View reviewed changes

@valerybugakov valerybugakov left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TIL about Note in the PR description

valerybugakov
valerybugakov approved these changes Aug 2, 2022
View reviewed changes

@valerybugakov valerybugakov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks fantastic on huge files (tested it locally) 🎉
And cudos for the full-stack solution!

Experimental improvements are great candidates for integration with the upcoming OpenTelemetry client observability. We can enable this feature on the dogfood instance, monitor time saved and potential unexpected side-effects, and decide if we want to make it a default behavior using data.

Comment thread client/web/src/repo/blob/BlobPage.tsx Outdated
@fkling

fkling commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

Possible that I'm misunderstanding this but atm this will fetch a lot of the information twice, correct? Should we enable this by default we can optimize to only fetch syntax highlighting information the second time.

@umpox

umpox commented Aug 3, 2022

Copy link
Copy Markdown
Contributor Author

@fkling Yep that's right, if we enable it fully I will optimize it - it's just so we can easily enable/disable this as an experimental feature

@umpox umpox merged commit f88e90a into main Aug 3, 2022
@umpox umpox deleted the tr/blob-perf branch August 3, 2022 13:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@oleggromov oleggromov Awaiting requested review from oleggromov

@olafurpg olafurpg Awaiting requested review from olafurpg

@tjdevries tjdevries Awaiting requested review from tjdevries

3 more reviewers

@fkling fkling fkling approved these changes

@valerybugakov valerybugakov valerybugakov approved these changes

@eseliger eseliger eseliger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@umpox @fkling @valerybugakov @tjdevries @eseliger