Skip to content
Snippets Groups Projects
New issue look: Off

Rewrite MR comments in Vue JS For Maximum Performance

    • Open Issue created by Jacob Schatz @jschatz1

    This issue is to track the work that will be done to rewrite the discussions on the MR page in VueJS This is a huge rewrite but also very important to begin the process of improving performance for our MR discussions.

    cc @victorwu @fatihacet

    Designs

      Child items ...

      2. Activity

      • Jacob Schatz
        Jacob Schatz @jschatz1 ·
        Author Maintainer

        @victorwu Let's schedule this soon.

      • Victor Wu changed milestone to %Next 3-6 months

        changed milestone to %Next 3-6 months

      • Victor Wu added Discussion label

        added Discussion label

      • Jacob Schatz changed milestone to %10.2

        changed milestone to %10.2

      • Victor Wu changed milestone to %10.3

        changed milestone to %10.3

      • Victor Wu
        Victor Wu @victorwu ·
        Reporter

        @fatihacet : Anyway to chop this down so that it is even smaller in scope?

      • Victor Wu mentioned in issue #36258

        mentioned in issue #36258

      • username-removed-502136
        username-removed-502136 @fatihacet ·
        Contributor

        @victorwu I don't think there is a way to do so. It's because we don't want to mix Vue and existing code together. In the past, we experienced many problems of this mixin. So unfortunately, this should be all in or nothing like we did for the Issue discussions and MR widget.

        Also, we should keep in my that the feature set of the MR page is much more bigger than the Issue page, especially diff rendering and diff commenting, resolvability and new coming feature which is commenting on image diffs. So a potential rewrite of the MR discussions is another very big and multi-month refactor and looks like the biggest one ever. On the other hand I already created the basic commenting components for the issue discussions so we can reuse them but again other features I mentioned above makes the scope huge. I would say this maybe a 3 release thing to do, at best 2 release. Sorry but that's what I see. /cc @jschatz1

        Edited by username-removed-502136
      • Victor Wu
        Victor Wu @victorwu ·
        Reporter

        Thanks @fatihacet. Let's take 3 months as the working assumption. Suppose we find a 3 month window and proceed. What would be required? What would be the recommended approach? How can we reduce risk and increase our chances of success? Something like this?

        • One dedicated FE person working on this full time as their highest priority. (In order not to delay the work.) Is that enough? That's important for us to plan so as to understand capacity for other features and dependencies during the 3 month window.
        • I know that GitLab engineers have lots of responsibilities with doing code review, fixing regressions, being release manager, participating in technical/architectural discussions, and generally supporting the product as a team member. Would it be reasonable/possible to drop some of these responsibilities during that time (for that dedicated FE person) to shave off at least one month from your estimate? (Thus, 2 months.)
        • Would the person be constantly rebasing the branch to master?
        • How would I be able to help? Would there be a way for me to continually test the branch and find bugs and offer support? I can definitely help continually doing regression testing if it will help. I can mindlessly just manually test a list of features over and over again during the period.
        • What areas of the product should we not work on during this 3 month window? These are some merge request related areas I want to work on soon. Which of these and others should we freeze development on if we were to proceed?
      • Victor Wu
        Victor Wu @victorwu ·
        Reporter

        @fatihacet : Can you enumerate the benefits of doing this Vue rewrite? Whenever we can do this, I'd like to be able to articulate the benefits to our stakeholders to convince them it is worth the effort.

        • It would make loading the merge request page x times faster. What is x?
        • It would reduce bugs/regressions because of these reasons. Would you be able to explain those reasons? Any way to estimate how much more stable/bug-free this would be?
        • It would speed up our development of merge request features going forward. Any way to quantify that? E.g. make us twice as efficient. Write less code. Write less tests. Anything to explain to external stakeholders.
      • username-removed-502136
        username-removed-502136 @fatihacet ·
        Contributor

        @victorwu

        One dedicated FE person working on this full time as their highest priority. (In order not to delay the work.) Is that enough?

        I agree with that. One person should be dedicated with this refactor and it should be their only focus, like no other deliverables. When the person start working on the refactor and come to a certain point we can grab someone else to help fixing existing tests or writing tests for the new code. Testing is whole another part of the refactors. Better we handle testing, better chance to reduce the time. Failing tests were the reason of MR widget refactor miss a release.

        I know that GitLab engineers have lots of responsibilities with doing code review, fixing regressions, being release manager, participating in technical/architectural discussions, and generally supporting the product as a team member. Would it be reasonable/possible to drop some of these responsibilities during that time (for that dedicated FE person) to shave off at least one month from your estimate? (Thus, 2 months.)

        I don't think it would make a month long difference. As far as I can tell from my two big refactor experience, my main focus was the refactor and I was spending at least 85% of my time on the refactor but they took so much time. It's because the context of the refactors were huge and there were tons of logic to re-implement.

        Would the person be constantly rebasing the branch to master?

        That's right. WIP branch should always be synced with master. About branching and development, we previously did this in two different ways.

        • Multiple small reviewable MRs like we did in MR widget refactor

          • Easier for reviewers. They do more reviews but with less code so they can track overall progress and be part of the feature development. They can build knowledge about how the feature is building and interrupt for change requests sooner than later.
          • Hard for developer. I need to create different MRs and need to make sure that I am not doing something wrong while creating branches and/or force pushing etc.

        • One giant MR like we did in issue discussions refactor

          • Easier for developer. I just need to push the same branch and keep track of only one branch.
          • Hard for reviewers. They will see one huge MR with 1000+ lines of changes so review won't be efficient. It's really easy to miss important things in such MRs.
          • Hard for people who wants to open that MR. Because it had 400+ comments, 250+ commits and 100+ pipelines. It was taking way too much time to open load the page. Plus it was a pain to type and submit a comment in that MR.

        So we should go with the multiple small chunk MRs option.

        How would I be able to help? Would there be a way for me to continually test the branch and find bugs and offer support? I can definitely help continually doing regression testing if it will help. I can mindlessly just manually test a list of features over and over again during the period.

        That would be GREAT. I can't express how this would be helpful. I really love this idea :thumbsup: We never do that before but if we have done this, that would save a lot of time. We do the testing round when the MR is ready. Reviewer test the new code in their local and they come up with 10-15 maybe even more bugs, missing things, unimplemented parts, design flaws etc. Some of the bugs they found, may require significant changes, like an architectural change so the cost of those findings can be huge and can be the reason to miss the next release. At this point, it also brings a huge disappointment and frustration to developer.

        What areas of the product should we not work on during this 3 month window?

        The main idea would be, freezing new feature development for the MR page. 2-3 months is a big window, we of course need to do bug fixes and that's totally ok but we shouldn't build new features. It's because we can't easily plug the new feature into the refactored end code. To avoid double work we should freeze feature development.

        It would make loading the merge request page x times faster. What is x?

        It's really hard to tell that x. But I believe we will see more screenshots and comments like this one.

        Basically our servers won't need to render HTML of let's say 200+ discussions. We will only need the JSON representation to render the views in client side. That's the whole idea of the refactor and it's also the main reason of that significant drop in the graph above.

        Also refactored code gives us a huge room for performance improvements. Like we can fetch and render discussions as small chunks. We can render the first 5 comment instantly then fetch rest of them and render when they arrive. No need to render all of them at once. This would drop first meaningful paint time significantly. Or another idea, when you scroll the page, we can remove invisible comments from DOM and avoid some performance bottlenecks. So much chances for further improvements.

        It would reduce bugs/regressions because of these reasons.

        This is a long time investment. Right after the refactor there could be some bugs, and maybe important bugs but they will be much more easier to fix because we will have well written, higher quality, modularised, good reviewed and documented code. After the initial bugs are fixed, we will have a solid feature which we can quickly develop new features on top of it.

        Currently notes.js which is the JavaScript file we use in the MR discussions page is a nightmare. It's 1500+ lines and it does too much DOM stuff. It's like a mine field. Whereever you touch may be the reason of another regression. We can avoid all of this.

        It would speed up our development of merge request features going forward. Any way to quantify that?

        Right. It would speed up our development in terms of building new features and fixing further regressions. You remember that before we start the issue discussions refactor, the original issue was "Start a thread from a non-discussion comment". I was assigned with that feature and started working on that using Vue in a non Vue, legacy code and it didn't work. I spend almost a week to make it happen or trying to find workarounds to make it happen. We just lost that time, then me and @jschatz1, we decided that this not gonna work and that's how we start refactoring. With the current issue discussions code, it's much more easier to develop that feature in a less time.

        Another benefit is, by nature of VueJS we write more simple code. Simple code means less learning curve so others can quickly get involved. Also Vue allows us to do complex things with less code and logic. This is another good advantage.

      • username-removed-443319
        username-removed-443319 @smcgivern ·
        Contributor

        @victorwu I think this would also affect https://gitlab.com/gitlab-org/gitlab-ce/issues/31847.

        It would be really nice to do https://gitlab.com/gitlab-org/gitlab-ce/issues/29939 on the backend, in conjunction with / alongside this.

        For an MR like https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14061, the initial page load currently spends about 70% of the time getting the comments. Now this won't be that dramatic, because things like https://gitlab.com/gitlab-org/gitlab-ce/issues/35475 will improve that in the mean time, but it's still a lot :slight_smile:

      Please register or sign in to reply