Retained Grape::Http::Headers in memory instead of new ones created. #1942
Conversation
Generated by 🚫 danger |
|
So for your example it's 80K. How many endpoints? How much slower is endpoint mounting? |
|
A lot of endpoints .. Is there an easyway to count them ? Like |
lib/grape/router.rb
Outdated
| @@ -46,7 +46,7 @@ def compile! | |||
| end | |||
|
|
|||
| def append(route) | |||
| map[route.request_method.to_s.upcase] << route | |||
| map[Grape::Http::Headers.find_supported_method(route.request_method.to_s)] << route | |||
There was a problem hiding this comment.
The request_method method comes from this place, so upcase isn't needed here 🙂
| @@ -26,6 +26,10 @@ module Headers | |||
| HTTP_ACCEPT = 'HTTP_ACCEPT' | |||
|
|
|||
| FORMAT = 'format' | |||
|
|
|||
| def self.find_supported_method(route_method) | |||
There was a problem hiding this comment.
The name of the method is find supported method, and you are returning unsupported methods from it. I think this method should either .detect or return nil and the caller should || ....
Next, I think we should turn Grape::Http::Headers::SUPPORTED_METHODS into a case-insensitive hash and lookup with Grape::Http::Headers::SUPPORTED_METHODS[method], which will be a lot more efficient than detect.
|
@dblock true # frozen_string_literal: true
require 'grape'
require 'benchmark/ips'
h = {
Grape::Http::Headers::GET => Grape::Http::Headers::GET,
Grape::Http::Headers::POST => Grape::Http::Headers::POST,
Grape::Http::Headers::PUT => Grape::Http::Headers::PUT,
Grape::Http::Headers::DELETE => Grape::Http::Headers::DELETE,
Grape::Http::Headers::PATCH => Grape::Http::Headers::PATCH,
Grape::Http::Headers::HEAD => Grape::Http::Headers::HEAD,
Grape::Http::Headers::OPTIONS => Grape::Http::Headers::OPTIONS,
}
Benchmark.ips do |ips|
ips.report('Hash with case insensitive') do
h[Grape::Http::Headers::SUPPORTED_METHODS.sample]
end
ips.report('Detect') do
Grape::Http::Headers.find_supported_method Grape::Http::Headers::SUPPORTED_METHODS.sample
end
ips.compare!
end |
I am not sure, but I think
Let us know when you're ready/happy with this PR! Looking forward to merge. |
|
I dont know why but now I'm just 2x slower instead of almost 6 Anyway, the reality is having a case-insensitive hash is really hard (no easy way). I mean we could upcase/downcase everything like before but we wont have a substantial gain (less memory usage) anymore. Also, since its a static array, worst case scenario is O(7). So, I'm not sure the 2x slower is a big issue here. One thing for sure, is right now having more endpoints, mean more memory usage which could be stop we this code. |
|
I don't think it's a big issue, I was just trying to make this code simpler. Your call. |
lib/grape/router/route.rb
Outdated
| @@ -64,7 +64,8 @@ def route_path | |||
| end | |||
|
|
|||
| def initialize(method, pattern, **options) | |||
| upcased_method = Grape::Http::Headers.find_supported_method(method.to_s) | |||
| str_method = method.to_s | |||
There was a problem hiding this comment.
Minor, maybe method_s and method_upcase is a better name/model for these variables. Don't care enough.
Changelog Keep the original route.request_method since its already sanitized Moved || outside of the function. Renamed local variables
ericproulx
force-pushed
the
retained_http_method
branch
from
96eccea
to
eceea52
Compare
|
I merged this, thanks. |
I ran a memory profiling (memory_profiler gem) on my app by hitting our Grape api first. I found it odd that it retains a bunch of http verbs when they are defined as const in Grape::Http::Headers :
I looked at the code and made a fix by detecting
supported_methodsinstead of retaining the route method received.Actual memory footprint on first call
And with the improvement
1958 less objects allocated (79 200 bytes)
984 less objects retained (40 240 bytes)