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

Fix memory leak in Oj.dump if raise an exception #676

Merged
merged 1 commit into from Aug 2, 2021

Conversation

Watson1978
Copy link
Collaborator

If an exception occurs inside Oj.dump, the heap area has not been released.
The heap area allocated by ALLOC_N must be released.
This patch will fix this problem.

Test code

require 'oj'

data = {
  "foo" => "a" * 1024 * 1024 * 10,
  1 => "Invalid hash key with strict mode"
}

1000.times do |i|
  begin
    Oj.dump(data, mode: :strict)
  rescue
  end

  if i % 100 == 0
    rss = Integer(`ps -o rss= -p #{Process.pid}`) / 1024.0
    puts "#{i}, #{rss}"
  end
end

Before

$ ruby json.rb
0, 49.4375
100, 1051.0
200, 1674.03125
300, 1773.625
400, 1822.171875
500, 1842.078125
600, 1841.015625
700, 1815.65625
800, 1781.53125
900, 1792.28125

1.8 GB of memory was used in my enviroment.

After

$ ruby json.rb
0, 49.40625
100, 49.421875
200, 49.53125
300, 49.59375
400, 49.65625
500, 49.6875
600, 49.71875
700, 49.734375
800, 49.765625
900, 49.796875

50 MB of memory was used in my enviroment.

@Watson1978 Watson1978 changed the title Fix memory leak in Oj.dump if raisee an exception Fix memory leak in Oj.dump if raise an exception Jul 31, 2021
@ohler55
Copy link
Owner

ohler55 commented Jul 31, 2021

Nicely done. Just one tiny request for a change.

@Watson1978
Copy link
Collaborator Author

Just one tiny request for a change.

OK, sure. What kind of request is that?

ext/oj/oj.c Outdated
Comment on lines 1246 to 1247
static VALUE dump_body(VALUE a);
static VALUE dump_ensure(VALUE a);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit. Move both dump_body and dump_ensure functions above the dump function and then remove these 2 lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Fixed at 0ab66a6

Thanks

@ohler55
Copy link
Owner

ohler55 commented Aug 1, 2021

Sorry, I've gotten used to using gitlab and forgot to click the submit review.

If an exception occurs inside Oj.dump, the heap area has not been released.
The heap area allocated by ALLOC_N must be released.
This patch will fix this problem.

### Test code
```ruby
require 'oj'

data = {
  "foo" => "a" * 1024 * 1024 * 10,
  1 => "Invalid hash key with strict mode"
}

1000.times do |i|
  begin
    Oj.dump(data, mode: :strict)
  rescue
  end

  if i % 100 == 0
    rss = Integer(`ps -o rss= -p #{Process.pid}`) / 1024.0
    puts "#{i}, #{rss}"
  end
end
```

### Before
```
$ ruby json.rb
0, 49.4375
100, 1051.0
200, 1674.03125
300, 1773.625
400, 1822.171875
500, 1842.078125
600, 1841.015625
700, 1815.65625
800, 1781.53125
900, 1792.28125
```

1.8 GB of memory was used in my enviroment.

### After
```
$ ruby json.rb
0, 49.40625
100, 49.421875
200, 49.53125
300, 49.59375
400, 49.65625
500, 49.6875
600, 49.71875
700, 49.734375
800, 49.765625
900, 49.796875
```

50 MB of memory was used in my enviroment.
@ohler55 ohler55 merged commit c6ad6e9 into ohler55:develop Aug 2, 2021
@ohler55
Copy link
Owner

ohler55 commented Aug 2, 2021

Thank you for the fix.

@ohler55
Copy link
Owner

ohler55 commented Aug 2, 2021

I don't have your email address but Oj is getting a parser rework and I would be honoured if you would like to act as a reviewer of the code when it is ready for a merge. Early result show the new parser could be as much as twice as fast as the current Oj parser.

@Watson1978 Watson1978 deleted the fix-memory-leak branch August 2, 2021 02:37
@Watson1978
Copy link
Collaborator Author

Sounds great. OK, sure :)

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.

None yet

2 participants