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 usage bump after upgrading to the latest version #1835

Closed
filipebarros opened this issue Dec 6, 2018 · 14 comments
Closed

Memory usage bump after upgrading to the latest version #1835

filipebarros opened this issue Dec 6, 2018 · 14 comments

Comments

@filipebarros
Copy link

filipebarros commented Dec 6, 2018

After upgrading to the latest version I've noticed the memory consumption keeps going up so I created a dummy application to check if the same happened with the previous version:

require 'grape'

class API < Grape::API
  get :heartbeat do
    'OK'
  end
end

run API

Here's the memory usage with both 1.2.1 version and 1.1.0 at the start of the process and after 1000 and 10000 calls:

1.2.1 - 1000 1.1.0 - 1000 1.2.1 - 10000 1.1.0 - 10000
Start 25.4Mb 26.4Mb 25.2Mb 25.1Mb
Finish 45.0Mb 29.0Mb 198.5Mb 29.7Mb

Here's the script I used to run the API calls.

for run in {1..1000} do
  curl -s "http://localhost:9292/heartbeat"
done
@dblock
Copy link
Member

dblock commented Dec 6, 2018

Let's debug this more thoroughly?

Check out #665 where we debugged a memory leak successfully for next steps.

cc: @myxoh

@dblock dblock added the bug? label Dec 6, 2018
@nijikon
Copy link

nijikon commented Dec 6, 2018

We are experiencing this as well, reverting to 1.1.0 fixed this issue for us.

@myxoh
Copy link
Member

myxoh commented Dec 6, 2018

Interesting, I wouldn't have thought the changes I've implemented should have impacted the memory consumption other than ever so slightly at load time. I'll take a look into this somewhere between late today and early tomorrow

@myxoh
Copy link
Member

myxoh commented Dec 6, 2018

oh wow. I think I've found the issue. This is unique to mounting the API directly on Rack I believe. call and call! are being called on the base instance, and hence continously added ot the setup of the application.
I already have a fix in place that passes all specs. I'll be creating a PR in a minute now.

Thanks for realizing this!

@myxoh
Copy link
Member

myxoh commented Dec 6, 2018

As a colour note, on my local testing this fix puts the memory footprint of 1.2.2 in the same values as the 1.1.0 .

@dblock
Copy link
Member

dblock commented Dec 6, 2018

@nijikon @filipebarros Please do try the code from #1836, which I just merged on master, and confirm whether the problem is fixed?

@dblock dblock added confirmed bug and removed bug? labels Dec 6, 2018
@myxoh
Copy link
Member

myxoh commented Dec 7, 2018

Setup:

# configu.ru
require 'grape'

class API < Grape::API
  get :heartbeat do
    'OK'
  end
end

run API

tab 1:
> rackup
tab 2:
> irb

> 1_000.times { `curl http://localhost:9292/heartbeat` }

Results:
master:
1,000 requests
32.8 MB
10,000 requests
32.9 MB

1.2.1
1,000 requests
46.5 MB
10,000 requests
214 MB

1.1.0
1,000 requests
32.0 MB
10,000 requests
32.1 MB

EDIT: 1.1.0 has been updated with the correct test results

@myxoh
Copy link
Member

myxoh commented Dec 7, 2018

Also, benchmarking the time for 1,000 calls gives me similar results for both 1.1.0 and master

@dblock
Copy link
Member

dblock commented Dec 7, 2018

Good. I say cut a release.

@myxoh
Copy link
Member

myxoh commented Dec 7, 2018

Agreed with the release. Could you look at #1833 before it please? 👍
Thanks

@filipebarros
Copy link
Author

Tested against 34610f56ef99dbc1d9bd1efd6fa564482020e7d2 and memory usage is steady around 29Mb for 10000 calls 👍

@myxoh
Copy link
Member

myxoh commented Dec 7, 2018

OK, I'll close this issue and will open another one for the release of 1.2.2.

Thanks again 😄

@myxoh myxoh closed this as completed Dec 7, 2018
@nijikon
Copy link

nijikon commented Dec 13, 2018

@dblock 1.2.2 works for us, thanks!

@aceunreal
Copy link

Thanks for the release 👍

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

No branches or pull requests

5 participants