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

Memory leak in Ruby protobuf library... #9384

Closed
AnashOommen opened this issue Jan 7, 2022 · 5 comments
Closed

Memory leak in Ruby protobuf library... #9384

AnashOommen opened this issue Jan 7, 2022 · 5 comments
Assignees
Labels

Comments

@AnashOommen
Copy link

While profiling the Google Ads API Ruby library, we noticed that there are memory leaks with the Ruby protobuf library. The findings (go/ruby-protobuf-leak) and the credentials to replicate the issue has been shared internally with Josh H. This can be replicated independently if you have access to Google Ads API (that's a reasonably high entry barrier, which is why I shared the creds for my test account with Josh H for easier replication). You need to follow the instructions at https://github.com/googleads/google-ads-ruby/blob/HEAD/README.md, then run test.zip attached with this bug.

test.zip

@elharo elharo added the ruby label Jan 7, 2022
@zhangskz zhangskz self-assigned this Jan 12, 2022
@zhangskz
Copy link
Member

Hi Anash, when replicating it seems the memory leak originates ~lines 17-21 when we access the field from the response protobufs (whether or not we print and thus convert to string does not seem to make a difference). Memory profiling does not show a leak in overall allocated/returned bytes when this part is removed, so I think we can rule out the search_stream call for the source of any potential leaks.

However, I did also notice that after adding a manual garbage dump per the code below, the memory profiler no longer shows any increase in allocated memory by gem for google-protobuf-3.19.2-x86_64-linux which seems to imply there might not be an actual memory leak in protobuf ruby. The previously seen growth could be unreachable objects that just hadn't been GC'd yet.

There does still seem to be an increase in the total allocated/retained bytes. Can you confirm if you're able to repro this, and confirm whether the leak might not be in protobuf itself? Note that message_exts.rb no longer seems to be a significant source of allocated memory by file (it no longer appears in memory profiler output) once manual GC is invoked. I also noticed that regardless of manual GC use, I wasn't able to replicate the increase in allocated memory for google_ads_client.rb which seems to stay consistent at ~2098768 bytes.

Code:

if __FILE__ == $0
  report = MemoryProfiler.report do
    100.times{ get_campaigns('9211913182') }
    GC.start()
  end

  report.pretty_print(to_file: 'get_campaign_100x.log')
end

Results (with GC):

1x
Total allocated: 6601484 bytes (53131 objects)
Total retained:  6038244 bytes (39050 objects)

allocated memory by gem
-----------------------------------
   4786723  google-ads-googleads-15.0.0
   1687578  google-protobuf-3.19.2-x86_64-linux
     48018  googleapis-common-protos-types-1.3.0
     33220  gapic-common-0.6.0
     24520  other

10x
Total allocated: 7067948 bytes (64784 objects)
Total retained:  6349348 bytes (46819 objects)

allocated memory by gem
-----------------------------------
   4787955  google-ads-googleads-15.0.0
   1687578  google-protobuf-3.19.2-x86_64-linux
    488440  other
     48018  googleapis-common-protos-types-1.3.0

100x
Total allocated: 11486596 bytes (175237 objects)
Total retained:  9295596 bytes (120462 objects)

allocated memory by gem
-----------------------------------
   4884128  other
   4798755  google-ads-googleads-15.0.0
   1687578  google-protobuf-3.19.2-x86_64-linux
     48018  googleapis-common-protos-types-1.3.0

I'm not certain what's up with the "other" allocated memory by gem but I think it might correspond to test.rb itself. I've also attached the logs.

get_campaign_1x_gc.log
get_campaign_10x_gc.log
get_campaign_100x_gc.log

@deannagarcia
Copy link
Member

Given no response for a while I'm going to close this issue, but feel free to reopen it if you're still facing the issue.

@AnashOommen
Copy link
Author

We will follow up internally. xfx@ is on paternity leave and switching teams, so we need to push this to next quarter.

@zhangskz
Copy link
Member

zhangskz commented Mar 8, 2022

Following second related report #9467 which helped us isolate the problem further, a fix was introduced in #9586 which should address the problem here as well. This should be included in the 3.20.0 release.

@casperisfine
Copy link
Contributor

@zhangskz just so you know, you can't use Ruby memory profilers to check wether google-protobuf cause memory growth, because all these profilers rely on ObjectSpace.memsize_of, and none of the google-protobuf extensions type define their memsize function.

E.g. for Message here:

{Message_mark, RUBY_DEFAULT_FREE, NULL},

So all these objects will report being 40 bytes when in reality they are much much bigger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants