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

Reduce memory usage for large file uploads #3062

Merged
merged 2 commits into from
Jan 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions benchmarks/local/sinatra/Gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
source "http://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }

ruby "3.2.0"

gem "sinatra"
gem "puma_worker_killer"

# current puma release
gem "puma"

# PR to reduce memory of large file uploads
# gem "puma", github: "willkoehler/puma", branch: "reduce_read_body_memory"
92 changes: 92 additions & 0 deletions benchmarks/local/sinatra/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# Large file upload demo

This is a simple app to demonstrate memory used by Puma for large file uploads and
compare it to proposed changes in PR https://github.com/puma/puma/pull/3062

### Steps to test memory improvements in https://github.com/puma/puma/pull/3062

- Run the app with puma_worker_killer: `bundle exec puma -p 9090 --config puma.rb`
- Make a POST request with curl: `curl --form "data=@some_large_file.mp4" --limit-rate 10M http://localhost:9090/`
- Puma will log memory usage in the console

Below is example of the results uploading a 115MB video.

### Puma 6.0.2

```
[11820] Puma starting in cluster mode...
[11820] * Puma version: 6.0.2 (ruby 3.2.0-p0) ("Sunflower")
[11820] * Min threads: 0
[11820] * Max threads: 5
[11820] * Environment: development
[11820] * Master PID: 11820
[11820] * Workers: 1
[11820] * Restarts: (✔) hot (✔) phased
[11820] * Listening on http://0.0.0.0:3000
[11820] Use Ctrl-C to stop
[11820] - Worker 0 (PID: 11949) booted in 0.06s, phase: 0
[11820] PumaWorkerKiller: Consuming 70.984375 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 70.984375 mb with master and 1 workers.

...curl request made - memory increases as file is received

[11820] PumaWorkerKiller: Consuming 72.796875 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 75.921875 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 78.953125 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 82.15625 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 85.265625 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 88.046875 mb with master and 1 workers.

...(clipped out lines) memory keeps increasing while request is received

[11820] PumaWorkerKiller: Consuming 121.53125 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 122.75 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 125.40625 mb with master and 1 workers.

...request handed off from Puma to Rack/Sinatra

[11820] PumaWorkerKiller: Consuming 220.6875 mb with master and 1 workers.
127.0.0.1 - - [26/Jan/2023:20:09:56 -0500] "POST /upload HTTP/1.1" 200 162 0.0553
[11820] PumaWorkerKiller: Consuming 228.96875 mb with master and 1 workers.
[11820] PumaWorkerKiller: Consuming 228.96875 mb with master and 1 workers.
```

### With PR https://github.com/puma/puma/pull/3062

```
[20815] Puma starting in cluster mode...
[20815] * Puma version: 6.0.2 (ruby 3.2.0-p0) ("Sunflower")
[20815] * Min threads: 0
[20815] * Max threads: 5
[20815] * Environment: development
[20815] * Master PID: 20815
[20815] * Workers: 1
[20815] * Restarts: (✔) hot (✔) phased
[20815] * Listening on http://0.0.0.0:3000
[20815] Use Ctrl-C to stop
[20815] - Worker 0 (PID: 20944) booted in 0.1s, phase: 0
[20815] PumaWorkerKiller: Consuming 73.25 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.25 mb with master and 1 workers.

...curl request made - memory stays level as file is received

[20815] PumaWorkerKiller: Consuming 73.28125 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.296875 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.34375 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.359375 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.359375 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.359375 mb with master and 1 workers.

...(clipped out lines) memory continues to stay level

[20815] PumaWorkerKiller: Consuming 73.703125 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.703125 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 73.703125 mb with master and 1 workers.

...request handed off from Puma to Rack/Sinatra

[20815] PumaWorkerKiller: Consuming 181.96875 mb with master and 1 workers.
127.0.0.1 - - [26/Jan/2023:20:27:16 -0500] "POST /upload HTTP/1.1" 200 162 0.0585
[20815] PumaWorkerKiller: Consuming 183.78125 mb with master and 1 workers.
[20815] PumaWorkerKiller: Consuming 183.78125 mb with master and 1 workers.
```
7 changes: 7 additions & 0 deletions benchmarks/local/sinatra/config.ru
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
require "sinatra"

post "/" do
204
end

run Sinatra::Application
15 changes: 15 additions & 0 deletions benchmarks/local/sinatra/puma.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
silence_single_worker_warning

workers 1

before_fork do
require "puma_worker_killer"

PumaWorkerKiller.config do |config|
config.ram = 1024 # mb
config.frequency = 0.3 # seconds
config.reaper_status_logs = true # Log memory: PumaWorkerKiller: Consuming 54.34765625 mb with master and 1 workers.
end

PumaWorkerKiller.start
end
6 changes: 4 additions & 2 deletions lib/puma/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ def initialize(io, env=nil)
@body_remain = 0

@in_last_chunk = false

@read_buffer = +""
Copy link
Member

Choose a reason for hiding this comment

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

What is this? I haven't seen this syntax before

Copy link
Member

Choose a reason for hiding this comment

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

It is the short syntax for a non frozen String (if strings was to be frozen by default). Same as String.new which would read better IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

+"" also has better performance.

Benchmark.bm do |bm|
  bm.report { 100000000.times { String.new } }
  bm.report { 100000000.times { +'' } }
end
       user     system      total        real
   4.359000   0.000000   4.359000 ( 10.255661)
   2.954000   0.000000   2.954000 (  6.286254)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally used String.new but this triggered a Rubocop Performance/UnfreezeString error.

I like String.new because it's intention is clear and we only allocate once. But I also see the benefit of using +"" as a rule app-wide if this is what the team prefers. Performance is important in Puma. There are some places where this gain may be meaningful.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, it's obviously not very easily googleable 😆

end

attr_reader :env, :to_io, :body, :io, :timeout_at, :ready, :hijacked,
Expand Down Expand Up @@ -433,7 +435,7 @@ def read_body
end

begin
chunk = @io.read_nonblock(want)
chunk = @io.read_nonblock(want, @read_buffer)
rescue IO::WaitReadable
return false
rescue SystemCallError, IOError
Expand Down Expand Up @@ -465,7 +467,7 @@ def read_body
def read_chunked_body
while true
begin
chunk = @io.read_nonblock(4096)
chunk = @io.read_nonblock(4096, @read_buffer)
rescue IO::WaitReadable
return false
rescue SystemCallError, IOError
Expand Down