Skip to content
Snippets Groups Projects

Improve NOT IssuableFinder via memoization

Closed Mario de la Ossa requested to merge 198324-slow-not-filters into master

What does this MR do?

Share the memoized values of IssuableFinder with the inner instantiated class if they're present.

This minimizes the need of the inner class to do the same queries already performed by the outer class.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Refs #198324 (closed)

Edited by Mario de la Ossa

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
  • 103 instance_variable_set(:"@#{param}", memoized_params[param]) unless memoized_params[param].nil?
    104 end
    105 end
    106
    107 def memoized_params
    108 @memo = MEMOIZED_PARAMS.map do |param|
    109 next unless instance_variable_defined?(:"@#{param}") && !skip_memoized_param?(param)
    110
    111 val = instance_variable_get(:"@#{param}")
    112 [param, val] unless val.nil?
    113 end.compact.to_h
    114 end
    115
    116 # We want to avoid passing the memoized groups/projects to the negated self.class if we're passing groups or projects
    117 # into as NOT params, otherwise the search just doesn't work.
    118 def skip_memoized_param?(param)
  • 91 93 end
    92 94 end
    93 95
    94 def initialize(current_user, params = {})
    96 def initialize(current_user, params = {}, memoized_params = {})
    95 97 @current_user = current_user
    96 98 @params = params
    99 ## If these params are set, we're a class instantiated inside itself, so let's share pre-loaded memoized values
  • 92 94 end
    93 95
    94 def initialize(current_user, params = {})
    96 def initialize(current_user, params = {}, memoized_params = {})
    95 97 @current_user = current_user
    96 98 @params = params
    99 ## If these params are set, we're a class instantiated inside itself, so let's share pre-loaded memoized values
    100 return unless memoized_params.present?
    101
    102 MEMOIZED_PARAMS.each do |param|
    103 instance_variable_set(:"@#{param}", memoized_params[param]) unless memoized_params[param].nil?
    104 end
    105 end
    106
    107 def memoized_params
    108 @memo = MEMOIZED_PARAMS.map do |param|
  • 91 93 end
    92 94 end
    93 95
    94 def initialize(current_user, params = {})
    96 def initialize(current_user, params = {}, memoized_params = {})
    95 97 @current_user = current_user
    96 98 @params = params
    99 ## If these params are set, we're a class instantiated inside itself, so let's share pre-loaded memoized values
    100 return unless memoized_params.present?
    101
    102 MEMOIZED_PARAMS.each do |param|
    103 instance_variable_set(:"@#{param}", memoized_params[param]) unless memoized_params[param].nil?
  • 99 ## If these params are set, we're a class instantiated inside itself, so let's share pre-loaded memoized values
    100 return unless memoized_params.present?
    101
    102 MEMOIZED_PARAMS.each do |param|
    103 instance_variable_set(:"@#{param}", memoized_params[param]) unless memoized_params[param].nil?
    104 end
    105 end
    106
    107 def memoized_params
    108 @memo = MEMOIZED_PARAMS.map do |param|
    109 next unless instance_variable_defined?(:"@#{param}") && !skip_memoized_param?(param)
    110
    111 val = instance_variable_get(:"@#{param}")
    112 [param, val] unless val.nil?
    113 end.compact.to_h
    114 end
    • @mdelaossa I've left mostly nitpicks for somewhat better readability, but I would like to get more context on the change to understand what we are doing here.

      From a few glances it seems that for negation we generate a query to pull items and then use that as a subquery under NOT IN (subquery) form, which does not sound too optimal 🤔

    • Author Contributor

      Yeah sorry, I should have explained how the negation works currently!

      Right now, for each negatable param, we create an inner IssuableFinder that returns a non-executed SQL query that then gets where.not(negation) called on it. So yes, currently they're used as subqueries in a NOT IN (subquery) form.

      Because of the chance of creating multiple IssuableFinders (for example if you're negating both the author and some labels at the same time) passing in these memoized values is worthwhile.

    • right, I am asking myself why are we doing subqueries and not just negating the params within original query ? what is the challenge in doing so? Obviously I am not aware of the whole picture, just want to understand why it would not be possible or difficult ?

      I'll take a scenario here, lets say we have the search field with following params on issues list: label = 'X' label != 'Y' assignee != 'UserX'

      so in my understanding we end up having a query like:

      select * from issues 
      inner join labels where label = 'X' and 
      issues.id not in (select id from issues where label = 'Y') AND 
      issues.id NOT IN (select id from issues where assignee = 'UserX')

      So we are basically joining the issues table as many times as many negation arguments we have. This is obviously not going to scale well. And this change is not going to add any visible improvement. I think we need to radically change what we are doing 🤔

      Why can we not do:

      select * from issues inner join labels where label = 'X' and label != 'Y' assignee != 'UserX' ?

      What is the challenge in doing so ?

    • here is a sample sql generated right now: https://gitlab.com/snippets/1952338

    • Author Contributor

      Initially this was done this way due to speed. We have a large amount of parameters that need negation and a finder that already knew how it all worked, so negating it all was a quick win.

      I do agree that negating each param directly in the main SQL will be our biggest improvement, but that's out of scope for this particular MR. That IS something we should do however

    • I just think maybe we should take a step back and reconsider adding this at all? I think it adds complexity that we will have to deal with further on and does not seem to add considerable improvement. 🤔

    • Author Contributor

      that's fair. Maybe I should just avoid adding more at all and sit down and properly code this without subqueries. Thanks for the suggestion, I'll keep this open for a bit more and probably just close it

    • I think the slowness and 500s in search come primarily from sql timing out? I am not 100% sure but I don't think problems are because the HTTP request itself takes too long. Or is there the later as well ?

    • Author Contributor

      Yes, most of the problem is SQL timing out. This MR was an incidental improvement, I have other improvements in the works where I was trying to get the internal subqueries to not have extraneous authorization checks that the external query doesn't have. That would have improved the actual slow queries themselves

    • Please register or sign in to reply
  • 91 93 end
    92 94 end
    93 95
    94 def initialize(current_user, params = {})
    96 def initialize(current_user, params = {}, memoized_params = {})
    95 97 @current_user = current_user
    96 98 @params = params
    99 ## If these params are set, we're a class instantiated inside itself, so let's share pre-loaded memoized values
    100 return unless memoized_params.present?
  • Krasimir Angelov approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading