Skip to content
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

Memoize Virtus attribute and fix memory leak. #1109

Merged
merged 1 commit into from
Aug 18, 2015

Conversation

marshall-lee
Copy link
Member

This one fixes a serious problem we faced in production under a heavy load. Investigation led us to the fact that it happens only when params are defined with type: Array[SomeClass]. In this case Virtus dynamically create some virtual classes (with Class.new) which inherits from base class with DescendantsTracker added which saves descendant classes into the array. Here we have a leak because garbage collector doesn't free these classes because an array still holds a reference to them.

So at least I can say that Virtus isn't designed for creating things dynamically. They should be stored somehow statically.

@dblock
Look, this is funny! Remember ruby-grape/grape-entity#161 ? This is why it is bad to store descendant classes in the array. Look at the implementation of DescendantsTracker — is it similar, right?

@marshall-lee marshall-lee force-pushed the memoize_virtus_converter branch 3 times, most recently from 9696bf0 to 46a72f2 Compare August 18, 2015 18:22
This one fixes a serious problem we faced in production under a heavy
load. Investigation led us to the fact that it happens only when params
are defined with `type: Array[SomeClass]`. In this case `Virtus`
dynamically create some virtual classes (with `Class.new`) which
inherits from base class with `DescendantsTracker` added which saves
descendant classes into the array. Here we have a leak because garbage collector
doesn't free these classes because an array still holds a reference to
them.

So at least I can say that `Virtus` isn't designed for creating things
*dynamically*. They should be stored somehow statically.
@marshall-lee marshall-lee force-pushed the memoize_virtus_converter branch from 46a72f2 to bdd3f79 Compare August 18, 2015 18:24
@marshall-lee
Copy link
Member Author

Some demonstrating examples:

Non-leaking:

require 'virtus'

type = Integer

10.times do |i|
  ObjectSpace.garbage_collect
  puts ObjectSpace.count_objects[:T_CLASS]
  Virtus::Attribute.build(type)
end

outputs:

854
858
858
858
858
858
858
858
858
858

Leaking:

type = Array[String]

10.times do |i|
  ObjectSpace.garbage_collect
  puts ObjectSpace.count_objects[:T_CLASS]
  Virtus::Attribute.build(type)
end

outputs:

854
865
867
869
871
873
875
877
879
881

@dblock
Copy link
Member

dblock commented Aug 18, 2015

This is solid work @marshall-lee, thanks.

dblock added a commit that referenced this pull request Aug 18, 2015
Memoize Virtus attribute and fix memory leak.
@dblock dblock merged commit 4b6e2b6 into ruby-grape:master Aug 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants