amplitude / experiment-ruby-server

Amplitude Experiment Ruby Server SDK
MIT License
2 stars 4 forks source link

Memory Leak on native side during local experiment eval #65

Closed gtoubassi closed 3 months ago

gtoubassi commented 4 months ago

Expected Behavior

Calls to experiment.evaluate should not leak memory

Current Behavior

Calls to experiment.evaluate do leak memory

Possible Solution

The leak is in the interface between ruby and the kotlin evaluator. Note there is no call to dispose the returned string from evaluate. If you compare to the python version you can see a fix went in in October 2023 to fix what I expect is related to the problem in this ruby sdk. See bgiori's commit to fix the python side:

https://github.com/amplitude/experiment-python-server/commit/93c647f4da429b75be6eea6c7611b771d4d786cf

I did some hacking of the ruby side to try and dispose the string and it seemed to help (?) but didn't solve:

26c26
<            :DisposeString, callback([:string], :void),
---
>            :DisposeString, callback([:pointer], :void),
63c63,64
<   result_json = fn.call(rule_json, user_json).read_string
---
>   fn_retval = fn.call(rule_json, user_json)
>   result_json = fn_retval.read_string
64a66,67
>   lib[:DisposeString].call(fn_retval)
> 

I am not familiar with ffi so was merely throwing darts.

Steps to Reproduce

The below reproduces on my mac. If you watch memory in ps/top/ActivityMonitor it will grow, for me starting at about 230mb to 500+mb. Note I print out the ruby memsize_of_all and that hardly moves. That implies its all on the native side.

  require 'amplitude-experiment'
  api_key = ENV['AMPLITUDE_API_KEY']
  assignment_config = AmplitudeExperiment::AssignmentConfig.new(api_key)
  eval_config = AmplitudeExperiment::LocalEvaluationConfig.new(assignment_config:, debug: false)
  local_experiments = AmplitudeExperiment.initialize_local(api_key, eval_config)
  local_experiments.start

  user = AmplitudeExperiment::User.new(user_id: "xyz123abc123")
  GC.start
  puts "mem: #{ObjectSpace.memsize_of_all}"
  10_000_000.times do
    user_flags = local_experiments.evaluate(user)
    user_flags&.dig("foo_bar")&.value
  end
  GC.start
  puts "mem: #{ObjectSpace.memsize_of_all}"

Environment

bgiori commented 4 months ago

Hi @gtoubassi . Thanks for submitting such a detailed issue. I'll take a look at this ASAP.

gtoubassi commented 4 months ago

Hi @gtoubassi . Thanks for submitting such a detailed issue. I'll take a look at this ASAP.

Thanks @bgiori ! Was hoping you'd see this, appreciate the attention!!

bgiori commented 4 months ago

This should be fixed in v1.3.1

Please confirm on your end and we can close the issue.

gtoubassi commented 3 months ago

Confirmed. Closing issue (apologies if this is improper protocol and I should let you close it)

bgiori commented 3 months ago

No problem! Thanks again for submitting.