POC for Make diffs requests and rendering smarter (and faster) for commit page
What does this MR do?
Builds a Proof of Concept for rendering the commit page faster. The purpose of this MR is to gather an idea of the technical challenges https://gitlab.com/gitlab-org/gitlab-ce/issues/52499 would imply (not to get it merged before feature freeze).
Performance implications of the Commit page
Currently loading the information from Commit
object through diffs
and diff_files
is typically a fast operation:
- Measured with this script https://gitlab.com/snippets/1862118
Commit | Time to execute commit.diffs
|
Time to execute commit.diffs.diff_files
|
---|---|---|
gitlab-runner@b8b3287e | 0.000075 |
3.300508 |
https://gitlab.com/gitlab-org/gitlab-ce/commit/07c32a0df5306082fe800de1ae6e2c51325f46e | 0.000033 |
2.656836 |
https://gitlab.com/gitlab-org/gitlab-ce/commit/3e99123095b26988de67a94b0e7a5207c1ef5ae2 | 0.000031
|
3.556449 |
Meanwhile, loading the HTML it takes quite a bit more:
Page | Time to load |
---|---|
gitlab-runner@b8b3287e | Completed 200 OK in 12167ms (Views: 12120.1ms - ActiveRecord: 8.1ms) |
https://gitlab.com/gitlab-org/gitlab-ce/commit/07c32a0df5306082fe800de1ae6e2c51325f46e | Completed 200 OK in 10991ms (Views: 10936.1ms - ActiveRecord: 10.5ms) |
https://gitlab.com/gitlab-org/gitlab-ce/commit/3e99123095b26988de67a94b0e7a5207c1ef5ae2 | Completed 200 OK in 14829ms (Views: 14750.7ms - ActiveRecord: 12.2ms)
|
Also mentioned here https://gitlab.com/gitlab-org/gitlab-ce/issues/52396#note_159915498
Proposal for the 1st iteration:
- Limit the initial HTML load to
N
files. This MR uses200
files as the initial load. - If the commit has more than the limits, then request a new batch of files:
- A new batch of
N
files (25
files on this demo) is requested when the user scrolls to the bottom of the page.- This is just for experimentation purposes, we need UX guidance for this interaction.
- The new batch of files is appended at the bottom of the page
- Logic is the same until there are no more batches to load.
- A new batch of
- Hide this logic behind a Feature Flag, so we can easily revert the behavior if necessary.
Demo
It's using this commit gitlab-runner@b8b3287e
Notes
- This proposal is scoped to the commit show page. It does not include changes to Merge Requests diffs.
- On the demo, we used a large number (
200
) and a smaller number (25
) as the sizes for the first and the upcoming batches. We tried a different approach by using the same size for all the batches, but even though it speeds up the initial load, it slowed down the rendering of the upcoming batches. (See details on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/28614#note_177474558) - Files render in batches are being expanded by default. We're not doing this on the initial batch for performance reasons (as we're rendering 200 files and expanding all of them will be slow)
Metrics
Average time of loading the commit show page
Page | Time to load on master
|
Time to load on this branch |
---|---|---|
gitlab-runner@b8b3287e | 12_167ms |
6_598ms |
https://gitlab.com/gitlab-org/gitlab-ce/commit/07c32a0df5306082fe800de1ae6e2c51325f46e | 10_991ms |
8_088ms
|
https://gitlab.com/gitlab-org/gitlab-ce/commit/3e99123095b26988de67a94b0e7a5207c1ef5ae2 | 14_829msms
|
7_342ms
|
Average time of loading batch responses
Page | Avg time to render the batch |
---|---|
gitlab-runner@b8b3287e | Completed 200 OK in 2986ms (Views: 133.1ms - ActiveRecord: 7.9ms) |
https://gitlab.com/gitlab-org/gitlab-ce/commit/07c32a0df5306082fe800de1ae6e2c51325f46e | Completed 200 OK in 2170ms (Views: 8.0ms - ActiveRecord: 3.9ms |
https://gitlab.com/gitlab-org/gitlab-ce/commit/3e99123095b26988de67a94b0e7a5207c1ef5ae2 | Completed 200 OK in 3250ms (Views: 158.8ms - ActiveRecord: 6.1ms)
|
What we need for 1st iteration:
-
UX - Design the interaction of loading upcoming batches:
- Whether load a new batch when the user scrolls to the bottom or
- Loading sequentially in the background or,
- Any other interaction
- frontend To help with the requests
-
backend There are still some pending discussions:
- Make
diff_collection#decorate_batch!
to iterate only on the defined range instead of the whole collection - Only load the
discussions
related to the batch (instead of all the discussions) - Make the links with anchors (
#note_176667331
) compatible with the batches https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/28614#note_176717028
- Make
Future Iterations
- Move the batch logic to an API and replace HTML files with Vue components.
Edited by Mayra Cabrera