Per https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546?lineprofiler=true, which is a large MR, we spend a lot of timing highlighting and truncating diff lines for use in discussions:
A lot of time is also in position comparisons, figuring out where we are, etc.:
We could speed this up quite a bit by not rendering the diffs at all on page load for a collapsed discussion, and then getting them async when we expand it (user clicks / anchor auto-expands discussion).
For the backend, we'd need to:
Have a way of getting just that diff (should be possible, based on the discussion ID).
@sarrahvesselov I'm considering this for 9.6, as it would be a nice performance gain.
The UX help I think we need is that in a collapsed diff discussion on an MR, we'll need an empty / loading state for the diff, as this won't be filled in on page load any more.
So in this screenshot:
The comments would be visible, but not the diff section, which we'd get async. (I'm not 100% sure about the filename, but I think we could render that on load.)
If it helps, we could possibly show the diff without highlighting, and fill the highlighting later - but I wonder if that would be more confusing than the skeleton.
@smcgivern If I understand this correctly, the majority of the waiting is on the highlighting to get processed? And we currently just let the page wait before the processing is finished, to show it all?
If so.. even though I think code without the highlighting makes it quite illegible :), I think its better than skeleton loading, but only if:
It really is as fast as skeleton loading
Doesn't make the content jump (vs skeleton loading which will prob have this as an effect ;) )
If I understand this correctly, the majority of the waiting is on the highlighting to get processed? And we currently just let the page wait before the processing is finished, to show it all?
Correct. It's not a majority in all cases, but it's bad on large MRs with lots of resolved comments.
I have no preference on un-highlighted vs skeleton - if skeleton is simpler, let's use that.
@smcgivern taking a look at this again I think its best to give this the skeleton loading principle.
The jumping of content would be great to avoid (as not every diff in the discussion stream is exactly equally large, although they should be close correct?), by being able to calculate how many lines the diff is, resulting in the height it will take up after loading.
@vsizov this would be great to have verified to be possible
(I'm not 100% sure about the filename, but I think we could render that on load.)
@vsizov this would be great to have verified to be possible.
As for the design:
Here are for example 2 different sizes, calculated on how big the the eventual diff would become.
The makes use of the following skeleton line variations (with which you can alternate):
@smcgivern Just to be clear, this issue only tackles code diff discussions in the discussion tab, correct?
As I think the loading of the diff tab (when opening the diff tab, from for example the discussion tab, could use some skeletons loading as well (Currently just displays a spinner). But is perhaps better off in a separate issue.. unless the same thing can be applied there.
This is a good chance to remind about my issue https://gitlab.com/gitlab-org/gitlab-ce/issues/30800 (I'm talking about sending a raw diff to a browser and highlighting it there). But I think this particular issue is just a step forward (calculations on a client side). Thanks for creating it.
I created the WIP MR that adds an endpoint which allows to load the diff using AJAX call. The payload of the response is a JSON document { discussion_html: ... } where discussion_html key contains the rendered template discussions/_diff_with_notes. Let me know if you need some other payload.
@smcgivern I could also return discussion.truncated_diff_lines, it's 2 minutes change but in my opinion, the more calculations we do async the better it is for the main page, isn't it?
@vsizov absolutely, but I'd also like to change as little as possible for the first iteration. In this case, we're using profiler output to identify a particular hotspot and fix it.
@dimitrieh here is example I have with skeleton loader. It is fixed size currently. From my (limited) understanding we don't know number of lines until we get truncated_diff_lines -
There is a bit of a content jump. Can potentially animate to limit it, but I don't think it's a big deal. Example where code is smaller than skeleton:
I agree. Static number of lines. @psimyn will you look at how I did the repo editor ghost lines. I want to make sure we are all doing this one way and make it reusable?
Would be great... this lessens the effect of content jump if any.. and is also what I propose in other places with skeleton loading.. will put this in the paradigm as well
Dimitrie Hoekstrachanged title from Projects::MergeRequestsController#show is slow to Projects::MergeRequestsController#show is slow (implement skeleton loading)
changed title from Projects::MergeRequestsController#show is slow to Projects::MergeRequestsController#show is slow (implement skeleton loading)
On the front-end I think you can get some serious performance wins by using windowing for rendering the layout for large merge requests. You should only render the file that the user can currently see on the screen, and only render the next file when scrolled to.
I'm not sure which FE framework you guys are using but there are several open source libs available that do this. Here are two examples:
@smcgivern - @vsizov's original commit returning discussions/_diff_with_notes makes frontend simpler - otherwise need to duplicate most of that template in frontend. Ok to go back to that to keep the change simpler?
@psimyn : Looks like this was started by not completed in 10.2. Pushing it to 10.3 for now. If you don't plan on finishing it in 10.3, please put it in the Backlog. Thanks!
Is anyone even working on this? 😞 It's a shame, it would have been a pretty straightforward improvement, but I guess it got overtaken by https://gitlab.com/gitlab-org/gitlab-ce/issues/38178, which is a much bigger project.
I think this should stay open, as this isn't covered by vue refactor yet (it still loads collapsed diffs). I'll try updating previous MR for 10.5 as well