Improve rendering speed of phone input component

[mitchellhenke]

I noticed we spend a disproportionate amount of time rendering templates on phone input pages in NewRelic

Ex:

I did a bit of profiling and it looks like it is in large part due to many calls to PhoneNumberCapabilities.translated_international_codes. It is not clear that it is an expensive operation that iterates over all of the country codes (currently 220) and translates them. PhoneInputComponent#international_phone_codes calls it for every country code (up to a total of 220^2 times).

I tried two approaches. The first being caching the results of PhoneNumberCapabilities.translated_international_codes and the second to skip recalculating PhoneNumberCapabilities.translated_international_codes in the component.

The caching did provide a ~25% improvement on top of the ~4500% improvement of simply not recalculating every time. However, I wasn't sure whether it was worth it since it's additional complexity and would require some more work to disable it in dev to avoid causing issues with updating translated content. If others think it's worth it, I can bring it in.

Both the caching code and benchmark code are on the mitchellhenke/phone-index-rendering-performance-tests branch.

Warming up --------------------------------------
no cache and recalculate
                         1.000  i/100ms
cache and recalculate
                         6.000  i/100ms
no cache and no recalculate
                         4.000  i/100ms
cache and no recalculate
                         6.000  i/100ms
Calculating -------------------------------------
no cache and recalculate
                          1.008  (± 0.0%) i/s -     31.000  in  30.760540s
cache and recalculate
                         59.919  (±13.4%) i/s -      1.734k in  30.023267s
no cache and no recalculate
                         45.959  (±10.9%) i/s -      1.352k in  30.056932s
cache and no recalculate
                         59.156  (±11.8%) i/s -      1.728k in  30.086327s

Comparison:
cache and recalculate:       59.9 i/s
cache and no recalculate:       59.2 i/s - same-ish: difference falls within error
no cache and no recalculate:       46.0 i/s - 1.30x  (± 0.00) slower
no cache and recalculate:        1.0 i/s - 59.42x  (± 0.00) slower

------------------------------------------------------------------------------


Warming up --------------------------------------
no cache and no recalculate
                         4.000  i/100ms
cache and no recalculate
                         6.000  i/100ms
Calculating -------------------------------------
no cache and no recalculate
                         47.595  (± 8.4%) i/s -      1.404k in  30.051126s
cache and no recalculate
                         59.313  (±10.1%) i/s -      1.740k in  30.005606s

Comparison:
cache and no recalculate:       59.3 i/s
no cache and no recalculate:       47.6 i/s - 1.25x  (± 0.00) slower

[zachmargolis]

LGTM! I think the cached code is worth a shot, since updating the names of countries is not a common development task. We could drop a comment that this wouldn't auto reload in development if needed?

[aduth]

Good find! This change alone looks to be quite a significant improvement, but I'm always up for some extra microoptimization in a hot path like this, so the caching could be a good supplement.

[aduth]

I'd wonder if it's even worth bothering with this development-mode handling. I'd be shocked if this caching ever turned out to be a problem.

(To future git blame-ers, I apologize if it turned out to be a problem)

[aduth]

Alternatively, could we just add a line like this at the start of the method, which would force the initialization each time?

remove_instance_variable(:@translated_intl_codes_data) if Rails.env.development?

[zachmargolis]

I agree, different develpment branch of code seems like more of a maintenance burden, I'd see if we can share code

[mitchellhenke]

There's some precedent within our code (and I ran into a hidden one while trying to profile with https://github.com/heartcombo/simple_form/blob/86429bceb950096df3c29616f31bd5a5ce706c06/lib/simple_form.rb#L175)

It's going to be inconsistent in some form along either path (either by not changing content when certain YML files are updated or having dev behave differently than other environments). Another option is configuration, but it doesn't feel like a good use-case for that.

I lean towards

remove_instance_variable(:@translated_intl_codes_data) if Rails.env.development?

since it's much simpler, but I don't feel incredibly strongly

[aduth]

Also, if it's always going to be a hash, could you just do a simple truthy check instead of defined? ?

def self.translated_international_codes
  @translated_intl_codes_data = nil if Rails.env.development?
  return @translated_intl_codes_data[I18n.locale] if @translated_intl_codes_data
  @translated_intl_codes_data = Hash.new { |h, k| h[k] = {} }
  # ...
end

[mitchellhenke]

Ah I like that, so I've pushed up a commit to do it.