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

Delete legacy blob component#50915

Merged
taras-yemets merged 36 commits into
mainfrom
taras-yemets/delete-legacy-blob
Apr 28, 2023
Merged

Delete legacy blob component#50915
taras-yemets merged 36 commits into
mainfrom
taras-yemets/delete-legacy-blob

Conversation

@taras-yemets

@taras-yemets taras-yemets commented Apr 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Removes legacy (table-based) blob component.
CodeMirror has been a default blob viewer since Sourcegraph 4.5.0. Table-based legacy blob viewer was left as an opt-out option in case we encounter major issues with the CodeMirror blob viewer. Luckily no significant issues were reported so we can remove the legacy (table-based) blob view.

Test plan

  • CI passes

@cla-bot cla-bot Bot added the cla-signed label Apr 20, 2023
@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Apr 20, 2023 •
edited by sourcegraph-buildkite
Loading

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
-0.75% (-20.67 kb) 🔽 -0.12% (-17.77 kb) 🔽 0.02% (+2.90 kb) -0.53% (-4) 🔽

Look at the Statoscope report for a full comparison between the commits 6a4bf80 and ff90add or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@taras-yemets taras-yemets force-pushed the taras-yemets/delete-legacy-blob branch from 96b1e20 to 6e5f46f Compare April 25, 2023 15:08
@taras-yemets taras-yemets marked this pull request as ready for review April 26, 2023 12:45
@taras-yemets taras-yemets requested a review from a team April 26, 2023 12:46
@sourcegraph-bot

sourcegraph-bot commented Apr 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff ff90add...6a4bf80.

Notify File(s)
@fkling client/web/src/search/results/StreamingSearchResults.tsx
@limitedmage client/web/src/search/results/StreamingSearchResults.tsx

@philipp-spiess

philipp-spiess commented Apr 26, 2023

Copy link
Copy Markdown
Contributor

Awesome! Does this mean we can also get rid of the fact that the first of the two Blob queries includes the legacy HTML rendered (IIRC this was necessary for backward compatibility to the old renderer).

For this first query we only need the content as raw text, the HTML highlight is just discarded IIRC.

Screenshot 2023-04-26 at 14 58 50

@taras-yemets taras-yemets force-pushed the taras-yemets/delete-legacy-blob branch from e31bd4d to a8efd30 Compare April 26, 2023 14:39
@taras-yemets

taras-yemets commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

Does this mean we can also get rid of the fact that the first of the two Blob queries includes the legacy HTML rendered (IIRC this was necessary for backward compatibility to the old renderer).
For this first query we only need the content as raw text, the HTML highlight is just discarded IIRC.

I guess we can.

Screenshot 2023-04-26 at 16 43 12

Setting the default value HTMLResponseFormat to JSON_SCIP(https://github.com/sourcegraph/sourcegraph/commit/a8efd30295e764064a30ce9842e5601e4ec6c84d) eliminates the first Blob query.
Now both fetchBlob calls use almost the same arguments and are memoized. When I say almost I mean that highlightedBlobInfoOrError may resolve even faster as it may have disableTimeout param set to true. In this case, do we even need formattedBlobInfoOrError (cc: @umpox)?

@taras-yemets

taras-yemets commented Apr 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

We can even remove the format field from FetchBlobOptions and pass HighlightResponseFormat.JSON_SCIP by default. WDYT, @philipp-spiess?

@umpox umpox closed this Apr 28, 2023
@umpox umpox reopened this Apr 28, 2023
@umpox

umpox commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

Oops sorry (missed the comment box...)

@umpox

umpox commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

I think we will still see code load noticeably faster with formattedBlobInfoOrError (and format: HighlightResponseFormat.HTML_PLAINTEXT set) mainly for medium/large/very large files.

I did a quick test loading this file:

  • Time to see code without plaintext: 2.1s
  • Time to see code with plaintext: 567ms

That files pretty big but I still see a 50ms-100ms improvement when loading medium sized files. For small files the difference is barely noticeable.

I think we should keep it. Btw, disableTimeout isn't actually used (in the backend) for the plaintext query IIRC, it's only for highlighted code.

umpox
umpox approved these changes Apr 28, 2023
View reviewed changes

@umpox umpox left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Exciting!

Comment thread client/web/src/repo/blob/backend.ts
Comment on lines -69 to -73
// We only want to include HTML data if explicitly requested. We always
// include LSIF because this is used for languages that are configured
// to be processed with tree sitter (and is used when explicitly
// requested via JSON_SCIP).
const html = [HighlightResponseFormat.HTML_PLAINTEXT, HighlightResponseFormat.HTML_HIGHLIGHT].includes(format)

@umpox umpox Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we'll get a perf boost from this PR. Looks like we were fetching html for plaintext responses when we didn't actually need to, nice!

@taras-yemets taras-yemets merged commit 2e909c0 into main Apr 28, 2023
@taras-yemets taras-yemets deleted the taras-yemets/delete-legacy-blob branch April 28, 2023 10:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@umpox umpox umpox approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@taras-yemets @sg-e2e-regression-test-bob @sourcegraph-bot @philipp-spiess @umpox @valerybugakov