Skip to content
Snippets Groups Projects

Optimise pipelines.json

Merged Kamil Trzcińśki requested to merge optimise-pipelines into master

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?

What are the relevant issue numbers?

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • @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:

    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.

  • Ideally I would like to try out both, but I am not sure if it could be done before 7th.

  • added 1 commit

    • 04e42e75 - Optimise pipelines.json [ci skip]

    Compare with previous version

  • added 7 commits

    • 062806e4 - Define baseline for test for pipelines serializer
    • 3015e768 - Revert "Reverse pipelines controller changes"
    • 8a1c12aa - Introduce endpoint optimisations
    • b828a98c - Optimise includes
    • 62919a17 - Update tests and associations
    • 93f9e71b - Further optimise queries
    • 227a664e - Next round

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Kamil Trzcińśki resolved all discussions

    resolved all discussions

  • Author Maintainer

    I did reduce amount of queries. Base scenario uses 180, we are down to 50.

  • Kamil Trzcińśki unmarked as a Work In Progress

    unmarked as a Work In Progress

  • 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)
  • @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?

  • Author Maintainer

    Yes, if you can! Thanks!

  • @ayufan ok, and I am done with the other merge request, so please review it, thanks!

  • added 3 commits

    • ce55999b - We no longer has has_command? method
    • 7547e865 - Use preload because we don't want join here
    • 553c784b - Ignore those optimization associations in import/export

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading