heartcombo / simple_form

Forms made easy for Rails! It's tied to a simple DSL, with no opinion on markup.
http://blog.plataformatec.com.br/tag/simple_form
MIT License
8.21k stars 1.31k forks source link

FormBuilder.attempt_mapping is taking > 1 second #1790

Closed egeek closed 1 year ago

egeek commented 1 year ago

Environment

Current behavior

This may be related to our Gemfile, and some additional gems that are extending the exception message processing, however what we are seeing is that in FormBuilder

    def attempt_mapping(mapping, at)
      return if SimpleForm.inputs_discovery == false && at == Object

      begin
        at.const_get(mapping)
      rescue NameError => e
        raise if e.message !~ /#{mapping}$/
      end
    end

the raise if e.message !~ /#{mapping}$/ is taking over 1000ms for the first lookup of that type of field (e.g. NumericInput) on a form. I believe that it's attempting to check if the exception message contains the name of the class that was searched for, e.g. (from my hacking about earlier to see what the issue was)

14:54:44 web.1                        | "NumericInput"
14:54:44 web.1                        | Object
14:54:44 web.1                        | #<NameError: uninitialized constant NumericInput
14:54:44 web.1                        | 
14:54:44 web.1                        |         res = at.const_get(mapping)
14:54:44 web.1                        |                 ^^^^^^^^^^>
14:54:44 web.1                        | 
14:54:44 web.1                        | 
14:54:44 web.1                        | SJH attempt_mapping RAISE TIME = 1159.357387

If I change the above line of code to raise unless e.is_a? NameError then the execution time drops to < 1 ms, e.g.

15:42:17 web.1                        | SJH attempt_mapping RAISE TIME = 0.008567

I believe that this keeps the same behaviour, as it will raise the exception if there's anything other than the NameError that was being checked for.

I don't believe the slow down is due to simple_form, the line of code in question hasn't changed in 10 years, and we can see the same slow down if we just log out the exception message in the function, e.g.

puts "SJH attempt_mapping #{e.inspect}"

also takes 1000 ms. But I believe the change from raise if e.message !~ /#{mapping}$/ to raise unless e.is_a? NameError should be relatively low risk to implement in simple_form and would prevent others from tripping over this. We aren't experiencing this behaviour elsewhere in our application as we handling exceptions based on their type rather than their contents.

carlosantoniodasilva commented 4 months ago

I'm reverting the merged change, as it caused a regression. See #1824 for more info.

Happy to review something else that fixes the original problem without the regression, but for now we'll have to live with it.