Performance degradation (~6x) when using given to model a dependent relation

[senhalil]

When change one of the parameters of out API as dependent to another one, the post duration increased from ~30 seconds to ~600 seconds. Is this normal?

Our model is deeply nested but both the dependee and depender are on the first level.

params do
  optional :matrix
  given matrix: ->(val) { val.nil? || val.empty? } do
    requires :vehicle, type: Hash do
      requires :router_mode, type: String
    end
  end
  given matrix: ->(val) { val.any? } do
    requires :vehicle, type: Hash do
      optional :matrix_id, type: String
      optional :router_mode, type: String
      exactly_one_of :matrix_id, :router_mode
    end
  end
end

[dblock]

I bet it's fixed by #2096. Try HEAD?

[senhalil]

Unfortunately, the performance degradation still exists. Actually, I work together with @braktar and we tested on his computer with the fix.

[dblock]

Put this into a benchmark and let's fix it.

[dnesteryuk]

@senhalil are you going to work on the benchmark?

[senhalil]

@dnesteryuk this issue fell in the cracks apparently, thanks for the ping.

I don't think I will be able to work on this issue since we gave up using given. The performance issue is still there and can be observed by making one of the parameters dependent in the existing large_model benchmark https://github.com/ruby-grape/grape/blob/master/benchmark/large_model.rb

[dnesteryuk]

I wrote this simple benchmark to compare params without dependencies and with them:

require 'grape'
require 'benchmark/ips'

class GivenAPI < Grape::API
  prefix :api
  version 'v1', using: :path

  params do
    optional :matrix
    given matrix: ->(val) { val.nil? || val.empty? } do
      requires :vehicle, type: Hash do
        requires :router_mode, type: String
      end
    end
    given matrix: ->(val) { val.any? } do
      requires :vehicle, type: Hash do
        optional :matrix_id, type: String
        optional :router_mode, type: String
        exactly_one_of :matrix_id, :router_mode
      end
    end
  end
  post '/' do
    'hello'
  end
end

class SimpleAPI < Grape::API
  prefix :api
  version 'v1', using: :path

  params do
    optional :matrix

    requires :vehicle, type: Hash do
      optional :matrix_id, type: String
      optional :router_mode, type: String
      exactly_one_of :matrix_id, :router_mode
    end
  end
  post '/' do
    'hello'
  end
end

options = {
  method: 'POST',
  params: {
    matrix: {
      something: 'test'
    },
    vehicle: {
      router_mode: 'hello'
    }
  }
}

env = Rack::MockRequest.env_for('/api/v1', options)

# warm up
GivenAPI.call

Benchmark.ips do |ips|
  ips.report('Given') do
    GivenAPI.call env
  end

  ips.report('Simple') do
    SimpleAPI.call env
  end

  ips.compare!
end

The result:

Warming up --------------------------------------
               Given   388.000  i/100ms
              Simple   583.000  i/100ms
Calculating -------------------------------------
               Given      3.938k (± 2.6%) i/s -     19.788k in   5.028092s
              Simple      5.920k (± 2.1%) i/s -     29.733k in   5.024627s

Comparison:
              Simple:     5920.1 i/s
               Given:     3938.2 i/s - 1.50x  slower

I would say it isn't that bad.

[senhalil]

I happened to have sometime this weekend, I thought I can chip-in.

I suspect this issue is related to data size and model parameter type, that's why it doesn't show up on a simple model with a small instance. I am still surprised that it takes 1.50x slower even with this model and instance.

I should have shared the definition of the model parameters properly in my original post. The matrix parameter actually has parameters of type Array[Array[Float]] inside (which are not taking part in the conditional statements). Even though neither the definition of these matrix elements depends on the conditional statement nor they appear in the condition itself, somehow they decrease the performance.

I have found an error in my original code that produced 20x difference -- one of the parameters were not defined in the SimpleAPI 🤦 -- sorry for the hype!

I created the following smallish model from scratch by cropping the irrelevant parts of ./benchmark/large_model.rb. The perf diff is around 6x with vrp_w_matrix_example.json (which is the relevant parts of a not so small but definitely not big VRP problem of 432 points).

I suspect the bigger the data gets, the worse the performance difference gets -- even if the part of the data have nothing to do with the conditional part of the model.

Warming up --------------------------------------
               Given     1.000  i/100ms
              Simple     1.000  i/100ms
Calculating -------------------------------------
               Given      0.109  (± 0.0%) i/s -      7.000  in  65.676987s
              Simple      0.720  (± 0.0%) i/s -     43.000  in  60.040736s

Comparison:
              Simple:        0.7 i/s
               Given:        0.1 i/s - 6.58x  (± 0.00) slower

require 'grape'
require 'benchmark/ips'

class SimpleAPI < Grape::API
  prefix :api
  version 'v1', using: :path
  content_type :json, 'application/json; charset=UTF-8'
  default_format :json

  def self.vrp_request_point(this)
    this.requires(:id, type: String, allow_blank: false)
    this.optional(:matrix_index, type: Integer, allow_blank: false, desc: 'Index within the matrices')
    this.optional(:location, type: Hash, allow_blank: false) do
      requires(:lat, type: Float, allow_blank: false)
      requires(:lon, type: Float, allow_blank: false)
    end
    this.exactly_one_of :matrix_index, :location
  end

  params do
    optional(:vrp, type: Hash, documentation: { param_type: 'body' }) do
      optional(:name, type: String)

      optional(:matrices, type: Array, documentation: { desc: 'Define the distances' }) do
        requires(:id, type: String, allow_blank: false)
        optional(:time, type: Array[Array[Float]], allow_blank: false, desc: 'Matrix of time')
        optional(:distance, type: Array[Array[Float]], allow_blank: false, desc: 'Matrix of distance')
      end

      optional(:points, type: Array) do
        SimpleAPI.vrp_request_point(self)
      end
    end
  end

  post '/' do
    'hello'
  end
end


class GivenAPI < Grape::API
  prefix :api
  version 'v1', using: :path
  content_type :json, 'application/json; charset=UTF-8'
  default_format :json

  def self.vrp_request_point(this)
    this.requires(:id, type: String, allow_blank: false)
    this.optional(:location, type: Hash, allow_blank: false) do
      requires(:lat, type: Float, allow_blank: false)
      requires(:lon, type: Float, allow_blank: false)
    end
  end

  params do
    optional(:vrp, type: Hash, documentation: { param_type: 'body' }) do
      optional(:name, type: String)

      optional(:matrices, type: Array, documentation: { desc: 'Define the distances' }) do
        requires(:id, type: String, allow_blank: false)
        optional(:time, type: Array[Array[Float]], allow_blank: false, desc: 'Matrix of time')
        optional(:distance, type: Array[Array[Float]], allow_blank: false, desc: 'Matrix of distance')
      end

      given matrices: ->(val) { val.nil? || val.empty? } do
        optional(:points, type: Array, documentation: { desc: 'Particular place on the map' }) do
          GivenAPI.vrp_request_point(self)
          exactly_one_of :location
        end
      end
      given matrices: ->(val) { val && !val.empty? } do
        optional(:points, type: Array, documentation: { desc: 'Particular place in the map' }) do
          GivenAPI.vrp_request_point(self)
          optional(:matrix_index, type: Integer, allow_blank: false, desc: 'Index within the matrices')
          exactly_one_of :matrix_index, :location
        end
      end
    end
  end
  post '/' do
    'hello'
  end
end

options = {
  method: 'POST',
  params: JSON.parse(File.read('vrp_w_matrix_example.json')) # https://gist.github.com/senhalil/263188ebdbe04ef545fc4278838cd8d8#file-vrp_w_matrix_example-json
}

env = Rack::MockRequest.env_for('/api/v1', options)

Benchmark.ips do |ips|
  ips.config(time: 60, warmup: 1)

  ips.report('Given') do
    GivenAPI.call env
  end

  ips.report('Simple') do
    SimpleAPI.call env
  end

  ips.compare!
end

[senhalil]

Amazing! Thanks for following this through @dnesteryuk

[dnesteryuk]

@senhalil did you try the master branch on your project?

[senhalil]

@dnesteryuk tried with the same benchmark and it looks great 👇 Cheers!

((HEAD detached at origin/master))$ bundle exec ruby benchmark/large_model_with_given.rb
Warming up --------------------------------------
               Given     1.000  i/100ms
              Simple     1.000  i/100ms
Calculating -------------------------------------
               Given      1.452  (± 0.0%) i/s -     86.000  in  60.535269s
              Simple      1.517  (± 0.0%) i/s -     91.000  in  60.484706s

Comparison:
              Simple:        1.5 i/s
               Given:        1.5 i/s - 1.05x  (± 0.00) slower

[senhalil]

@dnesteryuk @dblock I don't know how it can be done in a proper way which can be used in an automated way but if you would like to turn this benchmark into a proper benchmark or a test, let me know!

The lowest I could drop the duration of the benchmark is around a minute in total which is kind of too much but I am afraid anything shorter than this would not give reliable results (at least that's the case on my machine).

((HEAD detached at origin/master))$ time bundle exec ruby benchmark/large_model_with_given.rb
Warming up --------------------------------------
               Given     1.000  i/100ms
              Simple     1.000  i/100ms
Calculating -------------------------------------
               Given      0.847  (± 0.0%) i/s -     26.000  in  30.945069s
              Simple      0.883  (± 0.0%) i/s -     26.000  in  30.037589s

Comparison:
              Simple:        0.9 i/s
               Given:        0.8 i/s - 1.04x  (± 0.00) slower


real    1m11,613s
user    1m6,482s
sys     0m0,433s

[dblock]

We check-in these benchmarks into https://github.com/ruby-grape/grape/tree/master/benchmark, I wonder if there's a way to run them as part of CI and have them fail if the threshold is breached?