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

@Watson1978 Watson1978 commented Jul 31, 2021

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 ohler55 commented Jul 31, 2021

Nicely done. Just one tiny request for a change.

@Watson1978
Copy link
Collaborator Author

@Watson1978 Watson1978 commented Aug 1, 2021

Just one tiny request for a change.

OK, sure. What kind of request is that?

ext/oj/oj.c Outdated
static VALUE dump_body(VALUE a);
static VALUE dump_ensure(VALUE a);
Copy link
Owner

@ohler55 ohler55 Aug 1, 2021

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

Copy link
Collaborator Author

@Watson1978 Watson1978 Aug 2, 2021

OK. Fixed at 0ab66a6

Thanks

@ohler55
Copy link
Owner

@ohler55 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
29 of 31 checks passed
@ohler55
Copy link
Owner

@ohler55 ohler55 commented Aug 2, 2021

Thank you for the fix.

@ohler55
Copy link
Owner

@ohler55 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 Aug 2, 2021
@Watson1978
Copy link
Collaborator Author

@Watson1978 Watson1978 commented Aug 2, 2021

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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants