New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: severe memory leak in WeakValueMap #336

Merged
merged 1 commit into from Apr 7, 2015

Conversation

Projects
None yet
5 participants
@SamSaffron
Copy link
Contributor

SamSaffron commented Apr 1, 2015

entries were not being cleaned up correctly from the backing store. Impact is huge:

ENV['RAILS_ENV'] = 'production'
require 'objspace'
require 'memory_profiler'
require File.expand_path("../../config/environment", __FILE__)

def rss
 `ps -eo pid,rss | grep #{Process.pid} | awk '{print $2}'`.to_i
end

PrettyText.cook("hello world")

# MemoryProfiler has a helper that runs the GC multiple times to make
#  sure all objects that can be freed are freed.
MemoryProfiler::Helpers.full_gc
puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

20.times do

  5000.times { |i|
    PrettyText.cook("hello world")
  }
  MemoryProfiler::Helpers.full_gc
  puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

end

Before

rss: 137660 live objects 306775
rss: 259888 live objects 570055
rss: 301944 live objects 798467
rss: 332612 live objects 1052167
rss: 349328 live objects 1268447
rss: 411184 live objects 1494003
rss: 454588 live objects 1734071
rss: 451648 live objects 1976027
rss: 467364 live objects 2197295
rss: 536948 live objects 2448667
rss: 600696 live objects 2677959
rss: 613720 live objects 2891671
rss: 622716 live objects 3140339
rss: 624032 live objects 3368979
rss: 640780 live objects 3596883
rss: 641928 live objects 3820451
rss: 640112 live objects 4035747
rss: 722712 live objects 4278779
/home/sam/Source/discourse/lib/pretty_text.rb:185:in `block in markdown': Script Timed Out (PrettyText::JavaScriptError)
    from /home/sam/Source/discourse/lib/pretty_text.rb:350:in `block in protect'
    from /home/sam/Source/discourse/lib/pretty_text.rb:348:in `synchronize'
    from /home/sam/Source/discourse/lib/pretty_text.rb:348:in `protect'
    from /home/sam/Source/discourse/lib/pretty_text.rb:161:in `markdown'
    from /home/sam/Source/discourse/lib/pretty_text.rb:218:in `cook'
    from tmp/mem_leak.rb:30:in `block (2 levels) in <main>'
    from tmp/mem_leak.rb:29:in `times'
    from tmp/mem_leak.rb:29:in `block in <main>'
    from tmp/mem_leak.rb:27:in `times'
    from tmp/mem_leak.rb:27:in `<main>'

After

rss: 137556 live objects 306646
rss: 259576 live objects 314866
rss: 261052 live objects 336258
rss: 268052 live objects 333226
rss: 269516 live objects 327710
rss: 270436 live objects 338718
rss: 269828 live objects 329114
rss: 269064 live objects 325514
rss: 271112 live objects 337218
rss: 271224 live objects 327934
rss: 273624 live objects 343234
rss: 271752 live objects 333038
rss: 270212 live objects 329618
rss: 272004 live objects 340978
rss: 270160 live objects 333350
rss: 271084 live objects 319266
rss: 272012 live objects 339874
rss: 271564 live objects 331226
rss: 270544 live objects 322366
rss: 268480 live objects 333990
rss: 271676 live objects 330654

Recommend a new stable release with this asap. Deploying a monkey patch to Discourse

SamSaffron added a commit to discourse/discourse that referenced this pull request Apr 1, 2015

PERF: Fix memory leak
We used to leak some memory every time you cook a post

see: cowboyd/therubyracer#336
FIX: severe memory leak in WeakValueMap
entries were not being cleaned up correctly from the backing store

@SamSaffron SamSaffron force-pushed the SamSaffron:master branch from 3b4b2af to f3b8f84 Apr 1, 2015

@cowboyd

This comment has been minimized.

Copy link
Owner

cowboyd commented Apr 1, 2015

If I understand correctly, this is because while the referenced objects are being gc'd, the actual entries containing the week references are not?

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 1, 2015

yes, exactly, blogged a bit about this as well. http://samsaffron.com/archive/2015/03/31/debugging-memory-leaks-in-ruby

end

def self.ensure_cleanup(values,key,ref)
proc {

This comment has been minimized.

@novokshonovi

novokshonovi Apr 3, 2015

what the reason to use proc {} ? Asking just for educational purpose

This comment has been minimized.

@SamSaffron

SamSaffron Apr 6, 2015

Contributor

@novokshonovi regarding the reasoning for the class method and rather awkward definition of finalizers, you need to be ultra careful with closures: see http://www.mikeperham.com/2010/02/24/the-trouble-with-ruby-finalizers/

@dedene

This comment has been minimized.

Copy link

dedene commented Apr 3, 2015

👍

@cowboyd cowboyd merged commit f3b8f84 into cowboyd:master Apr 7, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@cowboyd

This comment has been minimized.

Copy link
Owner

cowboyd commented Apr 7, 2015

Released with v0.12.2

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 7, 2015

wow, awesome. time to kill my monkey patch :)

On Tue, Apr 7, 2015 at 1:52 PM, Charles Lowell notifications@github.com
wrote:

Released with v0.12.2
https://rubygems.org/gems/therubyracer/versions/0.12.2


Reply to this email directly or view it on GitHub
#336 (comment).

ruseel pushed a commit to ruseel/fluent-logger-ruby that referenced this pull request Jul 15, 2015

ruseel
rm "at_exit"
at_exit seems to leak memory.

checked with below script
(copied from SamSaffron's
cowboyd/therubyracer#336)

```
require 'objspace'
require 'memory_profiler'
require 'fluent-logger'

def rss
 `ps -eo pid,rss | grep #{Process.pid} | awk '{print $2}'`.to_i
end

MemoryProfiler::Helpers.full_gc
puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

20.times do
  500.times { |i|
    Fluent::Logger::FluentLogger.new.post("tag", {"a"=>1})
  }

  MemoryProfiler::Helpers.full_gc
  puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"
end
```

before this patch

```
rss: 19528 live objects 38181
rss: 20836 live objects 46999
rss: 21108 live objects 54998
rss: 21948 live objects 62998
rss: 22992 live objects 70998
rss: 24192 live objects 78998
rss: 25248 live objects 86998
rss: 26324 live objects 94998
rss: 27432 live objects 102998
rss: 28556 live objects 110998
...
...

```

after

```
rss: 19840 live objects 38181
rss: 20988 live objects 38998
rss: 21048 live objects 38997
rss: 21056 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
...
...
```

@ruseel ruseel referenced this pull request Jul 15, 2015

Closed

rm "at_exit" #28

cowboyd added a commit that referenced this pull request Aug 15, 2015

ensure idenity map entries are cleaned up:
This adds a regression for:

#336

@wasafiri wasafiri referenced this pull request Jan 14, 2016

Open

dj: memory leak #978

pnguyen1097 added a commit to pnguyen1097/billion-web that referenced this pull request Apr 25, 2016

pnguyen1097 added a commit to pnguyen1097/billion-web that referenced this pull request Jul 18, 2016

vlexaargas added a commit to billioneffect/billion-web that referenced this pull request Aug 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment