trailblazer / reform

Form objects decoupled from models.
https://trailblazer.to/2.1/docs/reform.html
MIT License
2.49k stars 184 forks source link

[~] fix memory leak when use "dry-validation" #525

Closed skcc321 closed 3 years ago

skcc321 commented 3 years ago

The issue was detected in two one places on each validation call:

  1. initialization of dry-validation contract
  2. initialization new array with constants inside

to reproduce: 1) install gem https://github.com/SamSaffron/memory_profiler 2) go to the repository and profile memory consumption by ruby script "mem_leak.rb".

ruby-memory-profiler --pretty -m 5 mem_leak.rb
# mem_leak.rb
require_relative "lib/reform"
require_relative "lib/reform/form/dry"

class TestForm < Reform::Form
  feature Reform::Form::Dry
end

Song = Struct.new(:title)
class Contr < TestForm
  validation do
    option :form
    params do
      required(:title).filled(min_size?: 2)
    end

    rule do
      if form == "hello"
        base.failure('creating events is allowed only on weekdays')
      end
    end
  end
end

form = Contr.new(Song.new(nil))

500.times do
  form.(title: "hello")
end
GC.start

3) download this PR and run it again.

here are results:

# before fix
{1:42}[2.7.2]~/reform:master ✗ ➭ ruby-memory-profiler --pretty -m 5 mem_leak.rb                                                                                                                       
Total allocated: 29.77 MB (261957 objects)                                                                                                                                                                                    
Total retained:  29.76 MB (261895 objects)  
...
# after fix
{1:42}[2.7.2]~/reform:master-fix-memory-leak ✗ ➭ ruby-memory-profiler --pretty -m 5 mem_leak.rb                                                                                                                 
Total allocated: 2.12 MB (20693 objects)                                                                                                                                                                                      
Total retained:  2.11 MB (20624 objects)
...

as a bonus, performance results:

# before
       user     system      total        real
   1.708815   0.072268   1.781083 (  1.782192)
# after 
       user     system      total        real
   0.181692   0.000599   0.182291 (  0.182303)
skcc321 commented 3 years ago

@seuros @emaglio could you guys take a look at that please?

apotonick commented 3 years ago

Thanks, this is great work!

I wonder why there are objects sticking around in the first place. Have you encountered this without dry-validation, too?

skcc321 commented 3 years ago

I faced with the issue on production. Due to urgency I had to do something ASAP. As a workaround, I switched reform to use ActiveModel validation on some services. Here is memory consumption graph before and after switching from "dry" to ActiveModel: image But we have other services where we still have dry + reform. That is why I started debug on dry contract first, but during investigation found not garbage collected arrays with references to the same constants.

Even with ActiveModel validation I noticed some (very) slow memory leak. But, that is much less dangerous.

I'll try to bring some evidences soon (unfortunately a bit loaded now)

apotonick commented 3 years ago

Well, I'm happy to merge that fix if it's urgent?

seuros commented 3 years ago

Im testing it

skcc321 commented 3 years ago

OK. Looks like I'm wrong about that array initialization. GC can garbage collect it. Initially, I tried to reproduce it inside tests, and I noticed it using that setup. It is not reproducible with the current setup. So looks like that is not the case. I'll remove that part from the PR.

skcc321 commented 3 years ago

any updates on that? @seuros

seuros commented 3 years ago

@skcc321 I think we are good. I found another memory leak, but it seem that it coming from dry-validation. I will write a report soon.

skcc321 commented 3 years ago

@seuros if so, how soon it can be released?

seuros commented 3 years ago

This weekend for sure.

seuros commented 3 years ago

@skcc321 on rubygems.

skcc321 commented 3 years ago

@seuros thank you!