From 34c82c5757f4bc47c5608e031d38d6809d7a92dc Mon Sep 17 00:00:00 2001
From: Toon Willems <m@toonwillems.be>
Date: Wed, 16 Oct 2024 11:12:20 +0200
Subject: [PATCH] perf(Filters) - increase performance of updating charge
 filters (#2696)

## Description

Based on profiling information we found out that the `to_h` method on
ChargeFilter is called often when updating a charge with a large ( >
100) amount of filters.

A second observation is that the `touch` method means we're updating all
charge filters every time, which has poor performance when you have a
lot of those for a given charge.

## Changes

- cache `to_h`
- updated_at is used for ordering the filters, but if you have > 100
we've chosen to optimize and not touch all records. This means stuff
will change in the UI, but at that amount nobody is really managing them
based on order.
---
 Gemfile.lock                                     | 16 ++++++++--------
 app/models/charge_filter.rb                      |  4 ++--
 .../create_or_update_batch_service.rb            | 13 ++++++-------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index 19654d7cbb7..af4a12c70a3 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -316,7 +316,7 @@ GEM
     listen (3.9.0)
       rb-fsevent (~> 0.10, >= 0.10.3)
       rb-inotify (~> 0.9, >= 0.9.10)
-    logger (1.6.0)
+    logger (1.6.1)
     lograge (0.14.0)
       actionpack (>= 4)
       activesupport (>= 4)
@@ -591,7 +591,7 @@ GEM
       ast (~> 2.4.1)
       racc
     pg (1.5.7)
-    prism (0.30.0)
+    prism (1.2.0)
     pry (0.14.2)
       coderay (~> 1.1)
       method_source (~> 1.0)
@@ -652,7 +652,7 @@ GEM
     rb-fsevent (0.11.2)
     rb-inotify (0.11.1)
       ffi (~> 1.0)
-    rbs (3.5.2)
+    rbs (3.6.1)
       logger
     rdoc (6.7.0)
       psych (>= 4.0.0)
@@ -736,13 +736,13 @@ GEM
       rubocop (~> 1.61)
     rubocop-thread_safety (0.5.1)
       rubocop (>= 0.90.0)
-    ruby-lsp (0.17.15)
+    ruby-lsp (0.20.0)
       language_server-protocol (~> 3.17.0)
-      prism (>= 0.29.0, < 0.31)
+      prism (>= 1.2, < 2.0)
       rbs (>= 3, < 4)
       sorbet-runtime (>= 0.5.10782)
-    ruby-lsp-rails (0.3.13)
-      ruby-lsp (>= 0.17.12, < 0.18.0)
+    ruby-lsp-rails (0.3.19)
+      ruby-lsp (>= 0.20.0, < 0.21.0)
     ruby-progressbar (1.13.0)
     sass-rails (6.0.0)
       sassc-rails (~> 2.1, >= 2.1.1)
@@ -796,7 +796,7 @@ GEM
     snaky_hash (2.0.1)
       hashie
       version_gem (~> 1.1, >= 1.1.1)
-    sorbet-runtime (0.5.11535)
+    sorbet-runtime (0.5.11602)
     sprockets (4.2.1)
       concurrent-ruby (~> 1.0)
       rack (>= 2.2.4, < 4)
diff --git a/app/models/charge_filter.rb b/app/models/charge_filter.rb
index 7feeb3cc20e..82a73ddcefe 100644
--- a/app/models/charge_filter.rb
+++ b/app/models/charge_filter.rb
@@ -25,13 +25,13 @@ def display_name(separator: ', ')
   end
 
   def to_h
-    values.each_with_object({}) do |filter_value, result|
+    @to_h ||= values.each_with_object({}) do |filter_value, result|
       result[filter_value.billable_metric_filter.key] = filter_value.values
     end
   end
 
   def to_h_with_all_values
-    values.each_with_object({}) do |filter_value, result|
+    @to_h_with_all_values ||= values.each_with_object({}) do |filter_value, result|
       values = filter_value.values
       values = filter_value.billable_metric_filter.values if values == [ChargeFilterValue::ALL_FILTER_VALUES]
 
diff --git a/app/services/charge_filters/create_or_update_batch_service.rb b/app/services/charge_filters/create_or_update_batch_service.rb
index 65e093a8748..821f292e57b 100644
--- a/app/services/charge_filters/create_or_update_batch_service.rb
+++ b/app/services/charge_filters/create_or_update_batch_service.rb
@@ -18,23 +18,22 @@ def call
         return result
       end
 
+      # We only care about order when you have less than 100 filters.
+      touch = filters_params.size < 100
+
       ActiveRecord::Base.transaction do
         filters_params.each do |filter_param|
           # NOTE: since a filter could be a refinement of another one, we have to make sure
           #       that we are targeting the right one
           filter = filters.find do |f|
-            next unless f.to_h.sort == filter_param[:values].sort
-
-            f.values.all? do |value|
-              filter_param[:values][value.key].sort == value.values.sort
-            end
+            f.to_h.sort == filter_param[:values].sort
           end
 
           filter ||= charge.filters.new
 
           filter.invoice_display_name = filter_param[:invoice_display_name]
           filter.properties = filter_param[:properties]
-          if filter.save! && !filter.changed?
+          if filter.save! && touch && !filter.changed?
             # NOTE: Make sure update_at is touched even if not changed to keep the right order
             filter.touch # rubocop:disable Rails/SkipsModelValidations
           end
@@ -48,7 +47,7 @@ def call
             )
 
             filter_value.values = values
-            if filter_value.save! && !filter_value.changed?
+            if filter_value.save! && touch && !filter_value.changed?
               # NOTE: Make sure update_at is touched even if not changed to keep the right order
               filter_value.touch # rubocop:disable Rails/SkipsModelValidations
             end