Performance optimisation of `available_tags` method

[matthew-andrews]

We have a fairly good size redmine install with ~13,000 issues and we have grown to rely on this plugin - about ~500 tags. Unfortunately this plugin has become quite a significant bottleneck.

The issue seems to lie within this line:

conditions << ids_scope.map{ |issue| issue.id }.push(-1)

The underlying SQL Rails generates is:-

SELECT `issues`.`id` AS t0_r0, `issues`.`tracker_id` AS t0_r1, `issues`.`project_id` AS t0_r2, `issues`.`subject` AS t0_r3, `issues`.`description` AS t0_r4, `issues`.`due_date` AS t0_r5, `issues`.`category_id` AS t0_r6, `issues`.`status_id` AS t0_r7, `issues`.`assigned_to_id` AS t0_r8, `issues`.`priority_id` AS t0_r9, `issues`.`fixed_version_id` AS t0_r10, `issues`.`author_id` AS t0_r11, `issues`.`lock_version` AS t0_r12, `issues`.`created_on` AS t0_r13, `issues`.`updated_on` AS t0_r14, `issues`.`start_date` AS t0_r15, `issues`.`done_ratio` AS t0_r16, `issues`.`estimated_hours` AS t0_r17, `issues`.`parent_id` AS t0_r18, `issues`.`root_id` AS t0_r19, `issues`.`lft` AS t0_r20, `issues`.`rgt` AS t0_r21, `issues`.`is_private` AS t0_r22, `issues`.`blocked` AS t0_r23, `issues`.`closed_on` AS t0_r24, `projects`.`id` AS t1_r0, `projects`.`name` AS t1_r1, `projects`.`description` AS t1_r2, `projects`.`homepage` AS t1_r3, `projects`.`is_public` AS t1_r4, `projects`.`parent_id` AS t1_r5, `projects`.`created_on` AS t1_r6, `projects`.`updated_on` AS t1_r7, `projects`.`identifier` AS t1_r8, `projects`.`status` AS t1_r9, `projects`.`lft` AS t1_r10, `projects`.`rgt` AS t1_r11, `projects`.`inherit_members` AS t1_r12 FROM `issues` LEFT OUTER JOIN `projects` ON `projects`.`id` = `issues`.`project_id` WHERE (projects.status <> 9 AND projects.id IN (SELECT em.project_id FROM enabled_modules em WHERE em.name='issue_tracking')) AND (projects.id=12)

Which, considering it only needs the id, is far more information that it really needs.

My initial thought was to swap out the line above with pluck:-

conditions << ids_scope.pluck(:id).push(-1)

But I immediately ran into a Rails bug (https://github.com/rails/rails/issues/6132?source=cc, https://github.com/rails/rails/issues/8743?source=cc) within to_sql, which pluck makes use of.

Also pluck only fixes part of the performance issue - even using it ids_scope.pluck(:id).push(-1) on our redmine install would generate an array of 7505 ids.

I then thought instead of downloading all the ids into rails so that they can be converted into an SQL query to send back again to MySQL it would be better to pass the query that ids_scope would have generated in as a subquery, which is what I've done (plus a little hack to add in the bits of SQL rails forgets to add).

Before:

Rendered plugins/redmine_tags/app/views/issues/_tags_sidebar.html.erb (4328.5ms)

After:

Rendered plugins/redmine_tags/app/views/issues/_tags_sidebar.html.erb (831.2ms)

Thank you for an otherwise marvellous plugin,

Matt

(PS. The commit "Way, way cleaner implementation" is in reference to my first attempt at addressing this, which was horrible :-) https://github.com/matthew-andrews/redmine_tags/commit/f6e49c795cd386a7bbcfa2ac2f8d5ba7e63f42aa)

[ixti]

+1 from me

[hdgarrood]

getting an email about a new issue and seeing that it includes a patch with a 5x performance improvement is awesome :) thanks!

It looks good to me. I've never touched this part of the code before though, so I'll leave it to @ixti or @PowerKiKi to decide when to merge.

[ixti]

:shipit: from me. I don't see any potential problems here...

[matthew-andrews]

@ixti Hold on, it does not pass the tests at the moment... (Have you considered using travis-ci to build on PR?)

@hdgarrood Well it varies... Sometimes it's only about a 3x performance improvement :).

[ixti]

@matthew-andrews I was considering to use travis-ci, but it required some work to do, and then i had not time, so I stepped out of maintenance (thanks @PowerKiKi and @hdgarrood for their work on maintaining this project). Nowdays I think about complete rewriting of a plugin nearly from scratch :D

[matthew-andrews]

Ok the tests pass now. (Sorry, I should have checked that first).

@ixti - Ah yes that makes sense - this plugin depends on redmine being there, doesn't it...

[ixti]

@matthew-andrews Yeah, but that part is solvable in fact :D Just didn't had much time to achieve that goal.

[matthew-andrews]

@PowerKiKi did you have a chance to have a look through this pull request?

Many thanks, Matt.

[PowerKiKi]

Sorry for the delay. And thanks a lot for the PR

[matthew-andrews]

No problem, thanks for merging!