Project

General

Profile

Actions

Patch #23328

open

Improve Update/Create issue speed

Added by Victor Campos over 7 years ago. Updated over 7 years ago.

Status:
New
Priority:
Normal
Category:
Performance
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Hi guys,
When Redmine look for what members it should send e-mail, they interate one by one fetching principal.
This is a N + 1 Query problem.

When we have more then 5K users in one project it is a problem. So with a single line change I drop the time for update issue from 5 to 2 seconds.

I hope this help you.

Date:   Tue Jul 12 19:37:14 2016 -0300

    improve update/create speed

diff --git a/app/models/project.rb b/app/models/project.rb
index 660a486..88bd8eb 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -524,7 +524,7 @@ class Project < ActiveRecord::Base
   # Returns the users that should be notified on project events
   def notified_users
     # TODO: User part should be extracted to User#notify_about?
-    members.select {|m| m.principal.present? && (m.mail_notification? || m.principal.mail_notification == 'all')}.collect {|m| m.principal}
+    members.includes(:principal).select {|m| m.principal.present? && (m.mail_notification? || m.principal.mail_notification == 'all')}.collect {|m| m.principal}
   end

   # Returns a scope of all custom fields enabled for project issues

Files

Actions #1

Updated by Lucas Arnaud over 7 years ago

I resolved this issue a bit different. I changed the includes to eager_load to explicitly eager load the principal association and added a find_each to save memory when the quantity of members is to big.

members.eager_load(:principal).find_each()
       .select {|m| m.principal.present? && (m.mail_notification? || m.principal.mail_notification == 'all')}
       .collect {|m| m.principal}
I've made some tests and these are the results:
# of project members current method after patch
6024 6.13s 1.15s
7933 7.57s 1.40s
7935 7.46s 1.32s
Actions #2

Updated by Victor Campos over 7 years ago

Yes, for memory it's a better solution.
=)

Thx for this patch

Actions #3

Updated by Go MAEDA over 7 years ago

  • Description updated (diff)
Actions #4

Updated by Go MAEDA over 7 years ago

  • Status changed from New to Needs feedback

Redmine 3.3.0 uses preload method in Project#notified_users. Please see r15518.
Could you test Redmine 3.3.0?

Actions #5

Updated by Victor Campos over 7 years ago

Go MAEDA wrote:

Redmine 3.3.0 uses preload method in Project#notified_users. Please see r15518.
Could you test Redmine 3.3.0?

Hi Go MAEDA,
What is the policy for update redmine stable branch? When 3.3-stable was lunch I update my redmine for it. When I read your comments I realided that there is a lot off new commits, with new features (redmine.lib changed a lot), performance issues fixed, etc.

About this issue, why preload and not eager_load? And I think the Lucas's idea with find_each is good to prevent memory problems.

Actions #6

Updated by Go MAEDA over 7 years ago

  • Status changed from Needs feedback to New
  • Assignee set to Jean-Philippe Lang
  • Target version set to Candidate for next major release

Thanks for the quick feedback.

Victor Campos wrote:

What is the policy for update redmine stable branch? When 3.3-stable was lunch I update my redmine for it. When I read your comments I realided that there is a lot off new commits, with new features (redmine.lib changed a lot), performance issues fixed, etc.

I am not a commiter, so I can't explain about the policy. But as I know, the branch was used to prepare releasing of 3.3.0. Many revisions were merged from trunk before 3.3.0 is released.

About this issue, why preload and not eager_load? And I think the Lucas's idea with find_each is good to prevent memory problems.

I would like Jean-Philippe Lang to make a judgment. Setting assignee to Jean-Philippe.

Actions #7

Updated by Go MAEDA over 7 years ago

  • Category set to Performance
Actions

Also available in: Atom PDF