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

fix: compress data from API for the page load performance improvement #8720

Merged
merged 4 commits into from
Jan 23, 2024
Merged

fix: compress data from API for the page load performance improvement #8720

merged 4 commits into from
Jan 23, 2024

Conversation

keita-determined
Copy link
Contributor

@keita-determined keita-determined commented Jan 19, 2024 •
edited
Loading

Description

WEB-1870

The response data from (almost?) all determined AI API endpoints were not compressed at all, so that caused long response time with Fast 3G network (look at Content download time).

Especially, experiments-search API usually returns big data, so this PR improve the page loading time around 10x.

Before

Screenshot 2024-01-19 at 12 27 22 PM
Screenshot 2024-01-19 at 1 08 27 PM

After

Screenshot 2024-01-19 at 12 25 09 PM
Screenshot 2024-01-19 at 1 07 29 PM

Test Plan

  • Check if API data are compressed
  • Check if there's Content-Encoding: gzip in response header
  • Check if js/css/html files are still compressed

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

@cla-bot cla-bot bot added the cla-signed label Jan 19, 2024
@netlify Netlify
Copy link

netlify bot commented Jan 19, 2024 •
edited
Loading

✅ Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit a63b453
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65b033c0af1f73000866f3fa

@codecov Codecov
Copy link

codecov bot commented Jan 19, 2024 •
edited
Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (bd78ec1) 47.26% compared to head (a63b453) 47.26%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8720      +/-   ##
==========================================
- Coverage   47.26%   47.26%   -0.01%     
==========================================
  Files        1045     1045              
  Lines      166710   166713       +3     
  Branches     2246     2246              
==========================================
- Hits        78792    78791       -1     
- Misses      87760    87764       +4     
  Partials      158      158
Flag Coverage Δ
backend 40.94% <0.00%> (+<0.01%) ⬆️
harness 64.29% <ø> (-0.01%) ⬇️
web 42.57% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
master/internal/core.go 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

keita-determined
keita-determined commented Jan 19, 2024
View reviewed changes
master/internal/core.go
@@ -966,10 +966,15 @@ func (m *Master) Run(ctx context.Context, gRPCLogInitDone chan struct{}) error {
// Other static files should only be cached for a short period of time
cacheFileShortTerm := regexp.MustCompile(`.(antd.\S+(.css)|ico|png|jpe*g|gif|svg)$`)

// API endpoints
apiRegex := regexp.MustCompile(`^/api/.+$`)

gzipConfig := middleware.GzipConfig{
Skipper: func(c echo.Context) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

question: is there something we don't wanna compress? I followed the current code, but not sure if we want Skipper here

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm I'm not familiar with compression... @maxrussell would you have any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing I can think of. Seems like a good change.

Copy link
Contributor Author

@keita-determined keita-determined Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ok, i'll remove Skipper and relavant variable(s), then merge this PR. thanks for the review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some investigation, i conclude that Skipper is still required for gzip.

i found that test_gc_checkpoints_lfs test case should avoid being compressed on CI (look at CI error)

Also, according to @azhou-determined,

gzip didn’t play nicely with the tensorboard profiler plugin. Tensorboard wouldn’t load when we compressed the responses

@keita-determined keita-determined marked this pull request as ready for review January 19, 2024 20:41
@keita-determined keita-determined requested a review from a team as a code owner January 19, 2024 20:41
@keita-determined keita-determined requested review from AmanuelAaron, maxrussell and eecsliu and removed request for AmanuelAaron January 19, 2024 20:41
@keita-determined keita-determined changed the title perf: compress data from API to improve the performance fix: compress data from API to improve the performance Jan 19, 2024
@keita-determined keita-determined changed the title fix: compress data from API to improve the performance fix: compress data from API for the page load performance improvement Jan 19, 2024
@keita-determined keita-determined merged commit d661404 into main Jan 23, 2024
73 of 85 checks passed
@keita-determined keita-determined deleted the perf/compress-api-data branch January 23, 2024 23:47
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
@keita-determined @maxrussell
fix: compress data from API for the page load performance improvement (…
0309494 
…#8720)

* perf: compress data from API

* chore: empty commit

* chore: remove `Skipper` in `gzipConfig`

* chore: Revert "chore: remove `Skipper` in `gzipConfig`"

This reverts commit 2f51abb.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxrussell maxrussell maxrussell approved these changes

@eecsliu eecsliu Awaiting requested review from eecsliu

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@keita-determined @maxrussell @eecsliu