Replace rack-mount with new router #1276

namusyaka · 2016-02-14T11:47:32Z

This is a first step for replcaing rack-mount with new router.
I'm going to improve performance to realize faster new router.

The new router has a few imcompatibility with old one.
The following cases:

route_* attribute should be deprecated because the feature is implemented by using method_missing. So it's very slow. (However, reviving the attribute is possible. )
Changed rack.routing_args to api.routing_args. The new router is for the Grape. I think the namespace is incorrect.

Thoughts?

dblock · 2016-02-14T14:20:06Z

This is excellent work. I am excited for getting this onto Grape master soon. It's really important to build some benchmarks and see how much improvement this is if at all, I think we owe this to the Grape community for switching something as essential as the router.

namusyaka · 2016-02-14T15:11:41Z

All required tests has been successful now.

arempe93 · 2016-02-15T15:22:26Z

👍 excited to see benchmarks

namusyaka · 2016-02-21T15:18:05Z

I've been tring to improve performance of the new router, and then I'd like to report a few points and hear your opinions.

Benchmark

First of all, I want you to know that the performance of rack-mount is excellent.
So I focused reducing memory utilization, and it was successful.

Please look at the following benchmark code and its results.

Sample code

Environment: Mac OS X, Ruby2.3

require 'rack'
require 'grape'
require 'benchmark'
require 'objspace'
require 'memprof2'

def memory_utilization
  Memprof2.start
  yield
  profiler = Memprof2.new
  profiler.configure
  mem = profiler.collect_info.values.inject(&:+)
  Memprof2.stop
  puts "Memory Utilization: #{mem/(1024.0 * 1024.0)}MB"
end

req1 = Rack::MockRequest.env_for('/100')
req2 = Rack::MockRequest.env_for('/users/1234', method: 'PUT')
req3 = Rack::MockRequest.env_for('/sources/hoge/aiueohogefuga', method: 'POST')
req4 = Rack::MockRequest.env_for('/sources/hoge/aiueohogefuga', method: 'PUT')

app = Class.new(Grape::API)
memory_utilization do

  500.times do |n|
    app.get("/#{n}") { n.to_s }
  end

  app.get('/') { 'hello' }
  app.put('/users/:id') { 'hello' }
  app.post('/sources', anchor: false) { 'hello' }

  app.call(req1)
  app.call(req2)
  app.call(req3)
  app.call(req4)
end

Benchmark.bm do |x|
  x.report do
    app.call(req1)
  end

  x.report do
    app.call(req2)
  end

  x.report do
    app.call(req3)
  end

  x.report do
    app.call(req4)
  end
end

rack-mount

Memory Utilization: 32.22776699066162MB
       user     system      total        real
   0.000000   0.000000   0.000000 (  0.000638)
   0.000000   0.000000   0.000000 (  0.000393)
   0.000000   0.000000   0.000000 (  0.000249)
   0.000000   0.000000   0.000000 (  0.000061)

new router

Memory Utilization: 14.685898780822754MB
       user     system      total        real
   0.000000   0.000000   0.000000 (  0.000495)
   0.000000   0.000000   0.000000 (  0.000263)
   0.000000   0.000000   0.000000 (  0.000231)
   0.000000   0.000000   0.000000 (  0.000534)

@dblock @arempe93 What do you think? Any ideas?

dblock · 2016-02-22T11:56:53Z

Do I read this correctly that the new router consumes half the memory and performs 30% faster (except for that last row which seems like an outlier for both?).

I think you want to make a lot of those "http" calls, at the very least those routers could have a cold start.

namusyaka · 2016-02-22T17:14:38Z

The new router consumes half the memory, but I think its performance is ambiguous.
Maybe the performance will be on the same level with rack-mount.

Okay, I tried to calculate benchmark for calling a lot of requests.
Please look at this sample code.

require 'rack'
require 'grape'
require 'benchmark'
require 'benchmark/ips'
require 'objspace'
require 'memprof2'

def memory_utilization
  Memprof2.start
  yield
  profiler = Memprof2.new
  profiler.configure
  mem = profiler.collect_info.values.inject(&:+)
  Memprof2.stop
  puts "Memory Utilization: #{mem/(1024.0 * 1024.0)}MB"
end

req1 = Rack::MockRequest.env_for('/100')
req2 = Rack::MockRequest.env_for('/users/1234', method: 'PUT')
req3 = Rack::MockRequest.env_for('/sources/hoge/aiueohogefuga', method: 'POST')
req4 = Rack::MockRequest.env_for('/sources/hoge/aiueohogefuga', method: 'PUT')

app = Class.new(Grape::API)
memory_utilization do

  500.times do |n|
    app.get("/#{n}") { n.to_s }
  end

  app.get('/') { 'hello' }
  app.put('/users/:id') { 'hello' }
  app.post('/sources', anchor: false) { 'hello' }

  app.call(req1)
  app.call(req2)
  app.call(req3)
  app.call(req4)
end

Benchmark.ips do |x|
  [req1, req2, req3, req4].each do |req|
    x.report { app.call(req) }
  end
end

Also, the results are:

rack-mount

Memory Utilization: 32.254828453063965MB
Calculating -------------------------------------
                       531.000  i/100ms
                       505.000  i/100ms
                       478.000  i/100ms
                         2.558k i/100ms
-------------------------------------------------
                          5.870k (±12.8%) i/s -     28.674k
                          5.507k (± 7.9%) i/s -     27.775k
                          5.594k (± 7.1%) i/s -     28.202k
                         46.889k (± 9.6%) i/s -    232.778k

new router

Memory Utilization: 14.55474853515625MB
Calculating -------------------------------------
                       396.000  i/100ms
                       552.000  i/100ms
                       561.000  i/100ms
                       259.000  i/100ms
-------------------------------------------------
                          4.008k (± 7.4%) i/s -     20.196k
                          5.638k (± 4.2%) i/s -     28.704k
                          5.417k (± 4.5%) i/s -     27.489k
                          2.650k (± 4.2%) i/s -     13.468k

dblock · 2016-02-22T19:23:47Z

spec/grape/path_spec.rb

@@ -244,7 +244,7 @@ module Grape
 allow(path).to receive(:uses_specific_format?) { true }
 allow(path).to receive(:settings) { { format: :json } }

- expect(path.path_with_suffix).to eql('/the/path(.json)')
+ expect(path.path_with_suffix).to eql('/the/path(.:format)')


Can you please explain this one?

Oops, this is my bad. Fixed.

dblock · 2016-02-22T19:24:39Z

I will run this against a production codebase to see if anything fails. Generally I am down with merging this, properly READMEed, CHANGELOGed, squashed, etc., please. We need to replace rack-mount since it's a dead project.

dblock · 2016-02-22T20:05:26Z

Haven't debugged, but the first failure in a real world project is with grape-swagger.

/Users/dblock/.rvm/gems/ruby-2.2.2/bundler/gems/grape-swagger-2f9aa49fc2e4/lib/grape-swagger.rb:27:in `block in add_swagger_documentation': undefined method `split' for nil:NilClass (NoMethodError)
    from /Users/dblock/.rvm/gems/ruby-2.2.2/bundler/gems/grape-swagger-2f9aa49fc2e4/lib/grape-swagger.rb:25:in `each'
    from /Users/dblock/.rvm/gems/ruby-2.2.2/bundler/gems/grape-swagger-2f9aa49fc2e4/lib/grape-swagger.rb:25:in `add_swagger_documentation'
    from /Users/dblock/source/artsy/gravity/dblock/app/api/v1/root_endpoint.rb:237:in `<class:RootEndpoint>'
    from /Users/dblock/source/artsy/gravity/dblock/app/api/v1/root_endpoint.rb:3:in `<module:V1>'
    from /Users/dblock/source/artsy/gravity/dblock/app/api/v1/root_endpoint.rb:2:in `<module:Api>'
    from /Users/dblock/source/artsy/gravity/dblock/app/api/v1/root_endpoint.rb:1:in `<top (required)>'

It's missing routes.

namusyaka · 2016-02-23T18:22:13Z

@dblock Sorry for the failing with grape-swagger.
This failure is caused by breaking comatibility of route_* attrs. So I fixed the problems.
Maybe you can use it now.

dblock · 2016-02-23T18:48:39Z

lib/grape/api.rb

- methods_per_path[route.route_path] ||= []
- methods_per_path[route.route_path] << route.route_method
+ # using the :any shorthand produces [nil] for route methods, substitute all manually
+ route_settings = (routes_map[route.pattern.to_regexp] ||= {})


I am not in love with this ||= assignment inside the right hand side. It's just a little difficult to read.

namusyaka · 2016-02-24T04:11:32Z

Done, thanks for pointing out!

dblock · 2016-02-25T19:38:15Z

First regression. We need specs on master for this since it wasn't caught in the Grape specs and I got it from my real world project, would appreciate a separate PR just for this story on master.

require 'spec_helper'

describe Grape::API do
  subject do
    Class.new(Grape::API) do
      format :json

      get ':id' do
        params
      end
    end
  end

  def app
    subject
  end

  it 'does not include extension in id' do
    get '/foo.bar'
    expect(last_response.status).to eq 404
  end
end

This succeeds on master, fails on your branch with a 200, and returns a value of foo.bar for id, which is definitely wrong since in this case .bar should be interpreted as an extension causing a format selection.

Note that it works consistently without a format, ie. this passes:

require 'spec_helper'

describe Grape::API do
  subject do
    Class.new(Grape::API) do
      get ':id' do
        params.to_json
      end
    end
  end

  def app
    subject
  end

  it 'does not include extension in id' do
    get '/foo.bar'
    expect(last_response.status).to eq 200
    expect(last_response.body).to eq({ id: 'foo', format: 'bar' }.to_json)
  end
end

dblock · 2016-02-25T20:08:02Z

Do you think we can preserve route_xyz methods, possibly with a deprecation warning?

dblock · 2016-02-25T20:10:19Z

lib/grape/router.rb

+
+ def transaction(env)
+ input, method, routing_args = *extract_required_args(env)
+ response = yield(input, method, ROUTING_ARGS_SETTER.curry[env, routing_args])


Could you explain what this magic is? :) This is beyond my Ruby-fu.

Okay, I removed the tricky code ;)

Failing spec for ruby-grape#1276.

Spec for #1276.

namusyaka · 2016-03-14T13:09:15Z

@dblock Thanks for your spec, I've just fixed the behavior and squashed.
Could you please check it out?

dblock · 2016-03-14T13:45:21Z

Running my battery of tests ... you can write UPGRADING in the meantime maybe. Thanks.

dblock · 2016-03-14T13:46:01Z

README.md

 ```

+Note that `Route#route_xyz` methods have been deprecated since 0.15.0.


This should be in UPGRADING.

dblock · 2016-03-14T13:49:26Z

From my tests ... this should be in UPGRADINGn and possibly in code: route.route_method was replaced by route.request_method, not route.method as per the deprecation.

dblock · 2016-03-14T14:04:03Z

Another tricky one, namusyaka#2.

dblock · 2016-03-14T15:55:48Z

lib/grape/router/route.rb

+ path, line = *location.scan(SOURCE_LOCATION_REGEXP).first
+ path = File.realpath(path) if Pathname.new(path).relative?
+ warn <<-EOS
+#{path}:#{line}: The route_xxx methods such as route_#{name} have been deprecated, please use #{name}.


I think this will say the wrong thing for route_method ;)

namusyaka · 2016-03-14T16:52:29Z

@dblock Thank you for polite review! Fixed.

namusyaka · 2016-03-14T17:03:21Z

If the changes have no problem, I'm going to squashed them.

dblock · 2016-03-14T17:33:13Z

Finished in 46 minutes 33 seconds (files took 49.5 seconds to load)
4712 examples, 0 failures, 28 pending

Lets squash it and get it merged!

namusyaka · 2016-03-14T17:38:47Z

Wow, great! I'm so happy! Thank you!

Replace rack-mount with new router

dblock · 2016-03-14T17:45:53Z

Merged.

arempe93 · 2016-03-14T17:46:28Z

🎉

kuraga · 2016-03-29T23:22:48Z

Is this Pendragon or... ?..

dblock · 2016-03-30T11:03:28Z

https://github.com/rkh/mustermann

namusyaka force-pushed the implement-new-router branch 4 times, most recently from e4d1a14 to 33fdcbd Compare February 14, 2016 14:12

namusyaka force-pushed the implement-new-router branch from 33fdcbd to d3aaade Compare February 14, 2016 14:40

namusyaka force-pushed the implement-new-router branch 2 times, most recently from 1f0876a to 3a25cca Compare February 21, 2016 14:58

namusyaka force-pushed the implement-new-router branch from 3a25cca to 0597dd8 Compare February 22, 2016 17:15

dblock reviewed Feb 22, 2016
View reviewed changes

namusyaka force-pushed the implement-new-router branch from 2acfd29 to 64bd67e Compare February 23, 2016 18:10

dblock reviewed Feb 23, 2016
View reviewed changes

namusyaka force-pushed the implement-new-router branch from 9e06f27 to 1ab3595 Compare February 24, 2016 04:10

dblock reviewed Feb 25, 2016
View reviewed changes

dblock added a commit to dblock/grape that referenced this pull request Mar 14, 2016

Failing spec for ruby-grape#1276.

ad22b11

namusyaka added a commit to namusyaka/grape that referenced this pull request Mar 14, 2016

Merge pull request #1 from dblock/implement-new-router-1276

ca065e2

Failing spec for ruby-grape#1276.

dblock added a commit that referenced this pull request Mar 14, 2016

Merge pull request #1320 from dblock/invalid-format-spec

41808e3

Spec for #1276.

namusyaka force-pushed the implement-new-router branch from ca065e2 to 92f4787 Compare March 14, 2016 13:08

dblock reviewed Mar 14, 2016
View reviewed changes

README.md

```

Note that `Route#route_xyz` methods have been deprecated since 0.15.0.

Copy link

Member

dblock Mar 14, 2016

Choose a reason for hiding this comment

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

This should be in UPGRADING.

dblock reviewed Mar 14, 2016
View reviewed changes

Replace rack-mount with new router

9f4ba67

namusyaka force-pushed the implement-new-router branch from 41b5e89 to 9f4ba67 Compare March 14, 2016 17:38

dblock added a commit that referenced this pull request Mar 14, 2016

Merge pull request #1276 from namusyaka/implement-new-router

2166237

Replace rack-mount with new router

dblock merged commit 2166237 into ruby-grape:master Mar 14, 2016

namusyaka deleted the implement-new-router branch March 14, 2016 17:47

This was referenced Mar 14, 2016

Add test for OPTIONS and HEAD requests with catch-all #1057

Closed

How grape recognizes a path? #1072

Closed

Support for the HTTP TRACE verb #1060

Open

Refactor Rack dependency #734

Open

przbadu pushed a commit to przbadu/grape that referenced this pull request Apr 5, 2016

Spec for ruby-grape#1276.

4766f66

dangnm mentioned this pull request May 16, 2016

API version can't take an array of versions on 0.16.2 #1400

Closed

starsirius mentioned this pull request Sep 8, 2016

No deprecation warnings on new Grape API artsy/gris#102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace rack-mount with new router #1276

Replace rack-mount with new router #1276

namusyaka commented Feb 14, 2016

dblock commented Feb 14, 2016

namusyaka commented Feb 14, 2016

arempe93 commented Feb 15, 2016

namusyaka commented Feb 21, 2016

dblock commented Feb 22, 2016

namusyaka commented Feb 22, 2016

dblock Feb 22, 2016

namusyaka Feb 23, 2016

dblock commented Feb 22, 2016

dblock commented Feb 22, 2016

namusyaka commented Feb 23, 2016

dblock Feb 23, 2016

namusyaka commented Feb 24, 2016

dblock commented Feb 25, 2016

dblock commented Feb 25, 2016

dblock Feb 25, 2016

namusyaka Feb 27, 2016

namusyaka commented Mar 14, 2016

dblock commented Mar 14, 2016

dblock Mar 14, 2016

dblock commented Mar 14, 2016

dblock commented Mar 14, 2016

dblock Mar 14, 2016

namusyaka commented Mar 14, 2016

namusyaka commented Mar 14, 2016

dblock commented Mar 14, 2016

namusyaka commented Mar 14, 2016

dblock commented Mar 14, 2016

arempe93 commented Mar 14, 2016

kuraga commented Mar 29, 2016

dblock commented Mar 30, 2016

		```

		Note that `Route#route_xyz` methods have been deprecated since 0.15.0.

Replace rack-mount with new router #1276

Replace rack-mount with new router #1276

Conversation

namusyaka commented Feb 14, 2016

dblock commented Feb 14, 2016

namusyaka commented Feb 14, 2016

arempe93 commented Feb 15, 2016

namusyaka commented Feb 21, 2016

Benchmark

Sample code

rack-mount

new router

dblock commented Feb 22, 2016

namusyaka commented Feb 22, 2016

rack-mount

new router

dblock Feb 22, 2016

Choose a reason for hiding this comment

namusyaka Feb 23, 2016

Choose a reason for hiding this comment

dblock commented Feb 22, 2016

dblock commented Feb 22, 2016

namusyaka commented Feb 23, 2016

dblock Feb 23, 2016

Choose a reason for hiding this comment

namusyaka commented Feb 24, 2016

dblock commented Feb 25, 2016

dblock commented Feb 25, 2016

dblock Feb 25, 2016

Choose a reason for hiding this comment

namusyaka Feb 27, 2016

Choose a reason for hiding this comment

namusyaka commented Mar 14, 2016

dblock commented Mar 14, 2016

dblock Mar 14, 2016

Choose a reason for hiding this comment

dblock commented Mar 14, 2016

dblock commented Mar 14, 2016

dblock Mar 14, 2016

Choose a reason for hiding this comment

namusyaka commented Mar 14, 2016

namusyaka commented Mar 14, 2016

dblock commented Mar 14, 2016

namusyaka commented Mar 14, 2016

dblock commented Mar 14, 2016

arempe93 commented Mar 14, 2016

kuraga commented Mar 29, 2016

dblock commented Mar 30, 2016