Optimise pipelines.json
What does this MR do?
Following and looking at this MR this is another take on that: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10327.
This effectively (currently the biggest problem is stages
) reduces number of queries from ~200 to ~50 on my test setup. It reduces the request processing time by 4-5x.
This is a proof of concept.
Are there points in the code the reviewer needs to double check?
Why was this MR needed?
Screenshots (if relevant)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Merge request reports
Activity
@godfat You did very good job with other MR, which is great. I wonder if we can get rid of
loaded_builds
with that MR as this approach is more Rails-way.- Resolved by Kamil Trzcińśki
- Resolved by Kamil Trzcińśki
@ayufan I like this approach because it's not doubling the codes. This should also greatly speed it up but I am not very sure which approach would be better. Do you think we should implement both and measure?
This doesn't really tell the performance difference because both are working in different progress:
- master: 24 seconds
- this MR: 22 seconds
- !10327 (closed): 6 seconds
With this code on my local:
user = User.last project = Project.last now = Time.now p = PipelineSerializer.new(project: project, user: user).represent(PipelinesFinder.new(project).execute).as_json; Time.now - now
This is walking through 78 pipelines.
added 7 commits
Toggle commit list- Resolved by username-removed-423915
101 before do 102 Ci::Pipeline::AVAILABLE_STATUSES.each do |status| 103 create_pipeline(status) 104 end 105 106 RequestStore.begin! 107 end 108 109 after do 110 RequestStore.end! 111 RequestStore.clear! 112 end 113 114 it "verifies number of queries" do 115 recorded = ActiveRecord::QueryRecorder.new { subject } 116 expect(recorded.count).to be_within(1).of(50) Why are we checking
count
manually, instead of using acontrol_count
andexceed_query_limit
? See https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/query_recorder.md#L1 and other uses like http://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/features/milestones/show_spec.rb#L21.@DouweM We don't really know what is the baseline. This is why it is explicit check.
@ayufan But you can set a baseline by doing 1 request to get the control count, then adding 10 more pipelines, doing another request, and making sure it doesn't increase the first number, right?
This test came up in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13356/diffs#note_37088371 and when preparing
9.4.5
in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13506
- Resolved by username-removed-423915
@ayufan Thank you for helping me on this. Looks good to me and let's merge this as the first step.
Two failures are relevant, one is that we need to ignore those relations when exporting/importing, and the other would be removing tests checking
has_command?
. Do you want me to fix them?@ayufan ok, and I am done with the other merge request, so please review it, thanks!
@jameslopez I just realized that https://gitlab.com/gitlab-org/gitlab-ce/commit/553c784bb29711ce03518ff2ceb9e990a5d8a317 doesn't exclude associations in https://gitlab.com/gitlab-org/gitlab-ce/builds/13817031 How could we ignore associations then?