Patch #26122

Implementation of visible conditions with inner join instead of subselect

Added by Pavel Rosický 2 months ago. Updated about 1 month ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Performance
Target version:4.1.0

Description

The change from #21608 should be reverted because it's a speed regression:

1/ current version

EXISTS (SELECT 1 AS one FROM enabled_modules em WHERE em.project_id = projects.id AND em. NAME = 'issue_tracking')

2/ previous version (actually faster even without an index on enabled modules)

projects.id IN (SELECT project_id FROM enabled_modules em WHERE em.project_id = projects.id AND em. NAME = 'issue_tracking'

3/ fastest version (in some cases better indexes were used)

INNER JOIN `enabled_modules` ON `enabled_modules`.`project_id` = `projects`.`id` WHERE `enabled_modules`.`name` = 'issue_tracking'
or
INNER JOIN `enabled_modules` ON `enabled_modules`.`project_id` = `projects`.`id` AND `enabled_modules`.`name` = 'issue_tracking'

Patches:
  • revert of #21608, because the previous version was faster (about 30%)
  • new index "enabled_modules_name", but it's not very helpful unless you have many projects (3000+)
  • implementation of visible conditions with inner join instead of subselect, it passes all tests, but it should be refactored. I want to know what do you think about it first
Rails version             4.2.8
Ruby version              2.1.9-p490 (x64-mingw32)
RubyGems version          2.6.12
Rack version              1.6.8
Middleware                Rack::Sendfile, Rack::ContentLength, ActionDispatch::Static, Rack::Lock, #<ActiveSupport::Cache::Strategy::LocalCache::Middleware:0x00000007745610>, Rack::Runtime, Rack::MethodOverride, ActionDispatch::RequestId, Rails::Rack::Logger, ActionDispatch::ShowExceptions, ActionDispatch::DebugExceptions, ActionDispatch::RemoteIp, ActionDispatch::Reloader, ActionDispatch::Callbacks, ActiveRecord::ConnectionAdapters::ConnectionManagement, ActiveRecord::QueryCache, ActionDispatch::Cookies, ActionDispatch::Session::CookieStore, ActionDispatch::Flash, ActionDispatch::ParamsParser, ActionDispatch::XmlParamsParser, Rack::Head, Rack::ConditionalGet, Rack::ETag, RequestStore::Middleware, OpenIdAuthentication
Environment               development
Database adapter          mysql2
Database schema version   20170607051650

20170607051650_add_index_on_enabled_modules_name.rb.patch Magnifier (416 Bytes) Pavel Rosický, 2017-06-07 22:51

revert_exists.patch Magnifier (959 Bytes) Pavel Rosický, 2017-06-07 22:51

joins_enabled_modules.patch Magnifier (11 KB) Pavel Rosický, 2017-06-07 22:51

History

#1 Updated by Pavel Rosický 2 months ago

  • Status changed from New to Resolved

#2 Updated by Pavel Rosický 2 months ago

I did some additional benchmarking and I was wrong. Exists really performs better.

so the only relevant patch is joins_enabled_modules.patch

Issue.visible is faster on my environment with this patch about 10% compared to exists with all caching disabled. I don't think it's worth to use it on other places.

#3 Updated by Mischa The Evil 2 months ago

  • Subject changed from revert #21608 to Implementation of visible conditions with inner join instead of subselect
  • Description updated (diff)
  • Status changed from Resolved to New

Edited issue properties based on the feedback and slightly improved the markup of the description.

#4 Updated by Toshi MARUYAMA about 1 month ago

  • Target version set to 3.4.0

#5 Updated by Toshi MARUYAMA about 1 month ago

  • Target version changed from 3.4.0 to 4.1.0

#6 Updated by Fernando Hartmann about 1 month ago

As a speed regression, I suggest to put this on 3.4.1

#7 Updated by Pavel Rosický about 1 month ago

Well, it havily depends on what filters and other conditions are used, it isn't easy to measure. For my long time observations using INNER JOINS is slightly faster (especially on big tables like issues) and mysql has better working plans with it.
On the other side, joins can't be passed directly into WHERE statement which is what allowed_to_condition should do - it increases code complexity. I want some opinions if you think it's worth to use it or not.
@Fernando - as discused before, it's not a speed regression just an improvement, so 3.5.0 is fine.

Also available in: Atom PDF