IOBuffer back to Ruby #1980

nateberkopec · 2019-09-20T12:37:13Z

File under "weird idea".

eregon · 2019-09-23T22:47:31Z

lib/puma/io_buffer.rb

+    end
+
+    def append(*args)
+      args.each { |a| concat(a) }


FWIW, Ruby 2.4+ supports multi-args String#concat.

Nice! We have to support 2.2+ for now.

eregon · 2019-09-23T22:51:00Z

This looks great to me:
-155 lines of C
-72 lines of Java
+17 lines of Ruby

I guess the main concern is: how does it affect performance?

nateberkopec · 2019-09-24T08:22:01Z

@eregon Yeah, agree. I've got an open issue on #1981 surrounding that.

kares · 2019-10-07T13:18:42Z

a bit ugly that IOBuffer now has a "full" String API but I guess its not a user API anyway ...

nateberkopec · 2019-10-13T09:27:36Z

This branch, extra large request body:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.96ms    1.02ms  16.99ms   72.38%
    Req/Sec     1.04k    98.31     1.22k    79.08%
  Latency Distribution
     50%    1.90ms
     75%    2.52ms
     90%    3.24ms
     99%    4.86ms
  124562 requests in 1.00m, 127.62GB read
Requests/sec:   2074.35
Transfer/sec:      2.13GB

Master, large request body:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.74ms    0.86ms  16.93ms   71.69%
    Req/Sec     1.17k    66.38     1.29k    85.25%
  Latency Distribution
     50%    1.68ms
     75%    2.26ms
     90%    2.81ms
     99%    3.99ms
  139603 requests in 1.00m, 143.03GB read
Requests/sec:   2325.64
Transfer/sec:      2.38GB

This branch, hello world:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   242.78us  137.14us   2.63ms   97.29%
    Req/Sec     8.67k   661.24     9.04k    96.52%
  Latency Distribution
     50%  224.00us
     75%  258.00us
     90%  290.00us
     99%    1.06ms
  173345 requests in 10.11s, 12.56MB read
Requests/sec:  17154.11
Transfer/sec:      1.24MB

Master, hello world:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   235.08us  135.54us   3.09ms   97.58%
    Req/Sec     8.90k     0.91k    9.36k    98.02%
  Latency Distribution
     50%  217.00us
     75%  251.00us
     90%  283.00us
     99%    1.03ms
  178961 requests in 10.10s, 12.97MB read
Requests/sec:  17715.35
Transfer/sec:      1.28MB

Maybe a 5% slowdown but that's really within the statistical margin of error. I think this is good for a merge.

nateberkopec · 2019-10-13T09:36:05Z

This appears to just stall the ruby 2.3 build 😖

eregon · 2019-10-13T20:47:20Z

lib/puma/io_buffer.rb

+module Puma
+  class IOBuffer < String
+    def initialize
+      if RUBY_VERSION <= "2.3"


This condition is incorrect (e.g. 2.3.4 <= 2.3 doesn't hold), use capacity if RUBY_VERSION >= 2.4 should work fine instead, and that's likely what makes the 2.3 build fails:
https://github.com/puma/puma/pull/1980/checks?check_run_id=258129791

/home/runner/work/puma/puma/lib/puma/io_buffer.rb:9:in `initialize': unknown keyword: capacity (ArgumentError) from /home/runner/work/puma/puma/lib/puma/io_buffer.rb:9:in `initialize' from /home/runner/work/puma/puma/lib/puma/thread_pool.rb:97:in `new' from /home/runner/work/puma/puma/lib/puma/thread_pool.rb:97:in `block (2 levels) in spawn_thread' from /home/runner/work/puma/puma/lib/puma/thread_pool.rb:97:in `map' from /home/runner/work/puma/puma/lib/puma/thread_pool.rb:97:in `block in spawn_thread'

nateberkopec · 2019-10-14T17:25:39Z

I'm not sure the capacity optimization is worth it. Removed it and it's now benchmarking faster than master, and hello.sh benchmark on this branch is now Requests/sec: 18001.34 vs master Requests/sec: 16311.53. Big response becnhmark is Requests/sec: 2269.64 vs master's Requests/sec: 2191.85. So we are now even or even slightly faster than master branch 😆

EDIT: Those numbers are MRI, I'll go back and check JRuby too before merge.

headius · 2019-11-11T15:42:16Z

@nateberkopec Where are the benchmarks you ran above? I'd like to confirm there's no performance hit on JRuby and see if any tweaks might help improve things further.

headius · 2019-11-11T15:52:31Z

This is failing on JRuby because the HTTP parser ext still references the Java IOBuffer. I'll have a patch shortly.

headius · 2019-11-11T15:53:59Z

Just remove import:

diff --git a/ext/puma_http11/PumaHttp11Service.java b/ext/puma_http11/PumaHttp11Service.java
index da7c2719..00f63aa2 100644
--- a/ext/puma_http11/PumaHttp11Service.java
+++ b/ext/puma_http11/PumaHttp11Service.java
@@ -6,7 +6,6 @@ import org.jruby.Ruby;
 import org.jruby.runtime.load.BasicLibraryService;
 
 import org.jruby.puma.Http11;
-import org.jruby.puma.IOBuffer;
 import org.jruby.puma.MiniSSL;
 
 public class PumaHttp11Service implements BasicLibraryService {

dentarg · 2019-11-11T16:06:05Z

@headius maybe these? https://github.com/puma/puma/tree/v4.3.0/benchmarks/wrk

headius · 2019-11-11T16:30:51Z

On a simple Roda benchmark, this change appears to have no negative performance impact for JRuby. It's within the margin of error for this wrk-driven Roda benchmark. 👍

nateberkopec force-pushed the io-ruby branch 3 times, most recently from bde2c0b to 1fbae70 Compare September 20, 2019 13:55

nateberkopec added this to the 5.0.0 milestone Sep 23, 2019

eregon reviewed Sep 23, 2019

View reviewed changes

nateberkopec added perf maintenance labels Oct 2, 2019

nateberkopec force-pushed the io-ruby branch 3 times, most recently from 0b99359 to f32961a Compare October 2, 2019 20:52

nateberkopec force-pushed the io-ruby branch from f32961a to bfe826e Compare October 13, 2019 09:28

eregon reviewed Oct 13, 2019

View reviewed changes

IOBuffer back to Ruby

66c0b9a

nateberkopec force-pushed the io-ruby branch from bfe826e to 66c0b9a Compare October 14, 2019 17:17

nateberkopec marked this pull request as ready for review October 16, 2019 04:17

nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Feb 21, 2020

Merge branch 'master' into io-ruby

44c03c7

nateberkopec merged commit 510f39d into master Feb 27, 2020

nateberkopec deleted the io-ruby branch March 14, 2020 21:52

nateberkopec mentioned this pull request Oct 27, 2020

Puma::IOBuffer can be more efficient and used in more places #2457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IOBuffer back to Ruby #1980

IOBuffer back to Ruby #1980

nateberkopec commented Sep 20, 2019

eregon Sep 23, 2019

nateberkopec Sep 24, 2019

eregon commented Sep 23, 2019

nateberkopec commented Sep 24, 2019

kares commented Oct 7, 2019

nateberkopec commented Oct 13, 2019

nateberkopec commented Oct 13, 2019

eregon Oct 13, 2019 •

edited

Loading

nateberkopec commented Oct 14, 2019 •

edited

Loading

headius commented Nov 11, 2019

headius commented Nov 11, 2019

headius commented Nov 11, 2019

dentarg commented Nov 11, 2019

headius commented Nov 11, 2019

IOBuffer back to Ruby #1980

IOBuffer back to Ruby #1980

Conversation

nateberkopec commented Sep 20, 2019

eregon Sep 23, 2019

Choose a reason for hiding this comment

nateberkopec Sep 24, 2019

Choose a reason for hiding this comment

eregon commented Sep 23, 2019

nateberkopec commented Sep 24, 2019

kares commented Oct 7, 2019

nateberkopec commented Oct 13, 2019

nateberkopec commented Oct 13, 2019

eregon Oct 13, 2019 • edited Loading

Choose a reason for hiding this comment

nateberkopec commented Oct 14, 2019 • edited Loading

headius commented Nov 11, 2019

headius commented Nov 11, 2019

headius commented Nov 11, 2019

dentarg commented Nov 11, 2019

headius commented Nov 11, 2019

eregon Oct 13, 2019 •

edited

Loading

nateberkopec commented Oct 14, 2019 •

edited

Loading