Asynchronous loading of an issue's related merge requests
When viewing certain issues (e.g. https://gitlab.com/gitlab-com/operations/issues/42) you'll see a list of related merge requests. Loading this data on average takes around half a second (http://performance.gitlab.net/dashboard/db/issue-referenced_merge_requests). While we're working on improving this it may take a while for this to be resolved once and for all. Since this data is not really crucial when working with issues I propose that we load this data asynchronously, this way we can at least shave off half a second of loading time when viewing an issue.
UI wise we'll probably need some kind of spinner. We'll also need a way to load the related merge requests as JSON and use this to build the HTML (unless we start sending HTML fragments via XHR).
cc @jschatz1
Activity
@yorickpeterse YES!!! BIG YES!!! I think we can try and get this into 8.7 but most likely it will before 8.8.
Edited by Jacob SchatzI'm tempted to drop the idea of loading this data asynchronously. The data we currently displays uses quite a bunch of different helpers that we'd have to port over to JS in some shape or form. The Ruby code needed to generate the JSON for the referenced merge requests is also quite hairy. As an example my diff is as follows:
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 6d649e7..2bbfdce 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -3,7 +3,7 @@ class Projects::IssuesController < Projects::ApplicationController include IssuableActions before_action :module_enabled - before_action :issue, only: [:edit, :update, :show] + before_action :issue, only: [:edit, :update, :show, :referenced_merge_requests] # Allow read any issue before_action :authorize_read_issue!, only: [:show] @@ -127,6 +127,18 @@ class Projects::IssuesController < Projects::ApplicationController @closed_by_merge_requests ||= @issue.closed_by_merge_requests(current_user) end + def referenced_merge_requests + merge_requests = @issue.referenced_merge_requests(current_user).map do |mr| + ReferencedMergeRequest.new(mr) + end + + respond_to do |format| + format.json do + render json: merge_requests + end + end + end + protected def issue diff --git a/app/models/referenced_merge_request.rb b/app/models/referenced_merge_request.rb new file mode 100644 index 0000000..32d65d4 --- /dev/null +++ b/app/models/referenced_merge_request.rb @@ -0,0 +1,60 @@ +class ReferencedMergeRequest + include Gitlab::Routing.url_helpers + include GitlabRoutingHelper + + def initialize(merge_request) + @merge_request = merge_request + end + + def iid + @merge_request.iid + end + + def title + @merge_request.title + end + + def state + @merge_request.state + end + + def build_status + @merge_request.ci_commit.try(:status) + end + + def build_url + commit = @merge_request.ci_commit + + if commit + builds_namespace_project_commit_url(project.namespace, project, commit.sha) + else + nil + end + end + + def project_url + namespace_project_url(project.namespace, project) + end + + def merge_request_url + super(@merge_request) + end + + def as_json(*) + { + iid: iid, + title: title, + state: state, + build_status: build_status, + build_url: build_url, + project_url: project_url, + merge_request_url: merge_request_url + } + end + + private + + def project + @merge_request.project + end +end diff --git a/config/routes.rb b/config/routes.rb index 6bf22fb..8893a87 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -698,6 +698,7 @@ Rails.application.routes.draw do resources :issues, constraints: { id: /\d+/ } do member do post :toggle_subscription + get :referenced_merge_requests end collection do post :bulk_updateIn short, we'd have to:
- Expose merge request, CI build, and project URLs
- Re-implement rendering of CI status indicators in JavaScript in some shape or form
- Take care of also rendering the merge requests that close said issue
- Find a way so the UI doesn't jump around whenever the data finishes loading (due to there suddenly being more content)
Looking at the above diff in order to re-use as much as possible we have to start injecting URL helpers into the
ReferencedMergeRequestclass which is gross.ReferencedMergeRequestis basically just a decorator, it shouldn't also expose all kinds of URL/routing methods.This leaves us with 3 options:
- Render the HTML using Rails and just send it over the wire via an Ajax request, then inject it into the DOM.
- Render the HTML using JSON, proper templating, yadyadya, and deal with the complexity above.
- Just make the code fast enough so we can keep the current setup.
I'm leaning towards option 3 since we have to do that anyway.
@rspeicher @DouweM @dzaporozhets @jschatz1: any suggestions/comments?
-
@yorickpeterse Don't give up! This is also part of a bigger picture. We need a good way to return JSON so the JS can call it to directly render on the page. The more you do in the backend the less we have to do in the frontend. Six of one, half a dozen of the other. Your solution is way better than my currently solution found in issue_controller.rb where I just use
respond_towith many includes. We need ways to return JSON objects from Ruby so JS can render them so we can have a more performant frontend. @jschatz1 The issue is that we use a lot of Rails-side helpers that aren't always easily ported to JS, so rendering on the frontend using only JSON to go off of will not be easy.
@yorickpeterse I'm leaning towards 1 or 3. I don't mind 1 (injecting HTML directly), but @jschatz1 might :)
-
@DouweM @yorickpeterse Injecting HTML is exactly what I am trying to remove from the app. It makes things so much harder to maintain from a frontend perspective. I still think we need to find a good way to render JSON for the frontend.
@jschatz1 Do we have an issue to move everything to React? ;) I see where you're coming from, but right now the app is not ready to have a partial like this moved to JS/JSON
@DouweM don't we already use VueJS?
-
@DouweM @yorickpeterse The end total time will be the same though right? Although 1 win is to have the page in view immediately.
@DouweM JSON (Javascript object notation) is really easy to work with in Javascript. In general we will pass around less bytes using JSON. I hear you that it isn't easy, but it would be good to start thinking about that so we can start writing really stateful JS components. It will go very well with our new websockets.
@jschatz1 How far did you get with ActiveModelSerializers? It seems like a good solution for this kind of thing.
JSON (Javascript object notation) is really easy to work with in Javascript. In general we will pass around less bytes using JSON.
I guarantee you Douwe knows this already.
don't we already use VueJS?
Nope. It never went beyond proof of concept as far as I know.
Edited by Robert Speicher-
@connorshea @rspeicher For VueJS to be possible we'd need this exact thing.
I guarantee you Douwe knows this already.
Of course he does I'm just messing around. Sorry didn't mean to be overly sarcastic. I just really want JSON data returns if you can't tell.
I'll give the
ActiveModelSerializersa try again. I think I had a hard time with that if I remember, but I can give it another go. @DouweM or @rspeicher any view on JBuilder? It seems easier to use.Edited by Jacob Schatz -
I'll give it another whirl thanks @rspeicher and @DouweM
-
I've used React a few times before, and I have to say its documentation is pretty bad. I will fully admit that this may just be Shiny-New-JS-Framework-Syndrome™, but my first inclination is to prefer Vue based on its documentation and its smaller size. Having not used it, it may suffer from the same problems as React and not matter, so in the end I don't care too much either way.
Vue could be the easiest to learn and good starting point to build prototype from. You could pick up one jquery based component from app, rebuild with Vue and start using.
ROR is not silver bullet so keeping all things at backend (getting data, parse views, keep this fast) will be harder and harder and harder, even for ruby masters like you all.Milestone changed to 8.8
@jschatz1 I want JSON and more frontend rendering as well, but our views currently use a lot of Ruby-side helpers that would be non-trivial to reproduce on the client-side since they often use the DB or Git. We can of course decide on the backend what we need from the DB or the Git repo, and then send a JSON object representing exactly what we want rendered to the frontend, but then we'd still have a lot of branching on the backend based on what exactly it is we want to render. At that point, the value of rendering on the frontend is not as big anymore since we'd be duplicating the branching.
I agree this is the direction we should move in, I'm just a little more pessimistic in this case than you are
-
@DouweM I hear you. We did it for the right sidebar. It was pretty heavily dipped in the rails way of doing things (but much less than the MR page) and we turned that around to be more responsive. Look at the results. A much easier to use sidebar. We can do it again for sure. Maybe just a little more research needed. Thanks for your support. Glad we are in agreement.
-
Could you tell me which gems are only native? It could be good to seperate them in one group in gemfile.
I watched RubyC 2015 Bozhidar Batsov: "Ruby: The Bad Parts" and I am wonder could be gitlab faster on diffrent execution env? It seems that MRI is slow. -
This presentation RubyConf 2015 - Everything You Know About the GIL is Wrong by Jerry D'Antonio also shows that execution env could change performance characteristic.
@mariusz_jachimowicz Switching Rubies is not going to make much of a difference here as the code is slow due to what the code does, not due to the implementation.
@yorickpeterse why not send rendered HTML in JSON like we do for pagination, notes etc. https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/projects/issues_controller.rb#L43-48
-
@dzaporozhets @mariusz_jachimowicz That's ok for now but we are trying to lean towards JSON for the future.
-
@jschatz1 sure. But if we cant easily make it work with JSON why not use existing solution for performance improvement?
Milestone changed to 8.7
-
-
I agree that there might exist some helpers hard to port to js but JQuery based aproach is killing me softly. For instance my !3370 (closed) could be simpler.
@mariusz_jachimowicz I don't think there's any harm in playing around with Vue.js
Having said that, I think it's best to move discussions about Vue vs jQuery/whatever to a separate issue. The discussion in this issue is already getting a bit offtopic.
-
@yorickpeterse Yes of course. I am often offtopicable, I am sorry. I am very excited generally about GitLab, possible further improvements and all your work guys. I will try to be focused more on each particular topic to not mess up.
-
@mariusz_jachimowicz Feel free to make a vus.js POC. I have been working on one myself I just haven't had that much time.
-
@jschatz1 Is your prototype public available or only localy?
@mariusz_jachimowicz It's absolutely not a problem and tends to happen quite often with pretty much everybody. However, at some point we just have to move certain discussions to a separate issue
Edited by yorickpeterse-stagingmentioned in commit 31e28ebc
Status changed to closed by commit 31e28ebc
mentioned in merge request !3671 (merged)
Mentioned in commit pfjason/gitlab-ce@31e28ebc
assigned to @jschatz1
mentioned in commit test-nick/gitlab-ce@2529feb9
