Don't call builder on every request. #1201
Conversation
|
The most scary thing here is that if I put I need someone to carefully review this change. |
marshall-lee
force-pushed
the
rack_builder_used_right
branch
from
340ed31
to
c29c54b
Compare
|
I actually think this is absolutely right, it's amongst the oldest pieces of code that nobody dared touch. It also used to be much more involved when we didn't have all those stackable settings. I read the code and ran your branch against my large API test suite and everything but grape-newrelic worked, too. I'd like to merge this, as "in specs we trust". I'll debug that one unless someone beats me to it. It would be also great to get some benchmarks wrt what this change means to performance. Then I think you should send an email to the mailing list asking people to try HEAD, maybe someone will be brave enough to even try it in production :) |
|
Such a nice PR,
it overrides the original behaviour Anyway, the changes LGTM! |
Don't call builder on every request.
|
Right on @dm1try. I merged this and opened xinminlabs/newrelic-grape#31, lets come up with a better, documented, interface to register middleware other than a monkey patch? |
|
@dblock I agree. |
This PR follows the same logic as ruby-grape#1201.
| @@ -188,6 +188,7 @@ def compile_path(prepared_path, anchor = true, requirements = {}) | |||
| end | |||
|
|
|||
| def call(env) | |||
| build_app | |||
There was a problem hiding this comment.
is it threadsafe to do @app lazy initialization here?
There was a problem hiding this comment.
theoretically #build_stack can be called multiple times but I don't see any side effect here( see the implementation of #build_stack); just extra an extra calculation, if many threads will try to set @app.
Anyway if we create the interface which allows to extend an endpoint middleware stack(see #1202) and we guarantee that the build_stack only executed once, this part should be synchronized.
UPDATED
There was a problem hiding this comment.
@etehtsea , I updated the comment sorry :)
This PR follows the same logic as ruby-grape#1201.
Now I'm working on reducing calls to `extend` in `Grape` because they invalidate Ruby's method cache slowing everything down. I applied a *clever* trick with subclassing `Endpoint` and including helpers into this subclass. It also forced me to fix a thread safety issue reported by @etehtsea here: ruby-grape#1201 (comment) because calling `include` twice would be really inconsistent. Also I'm sorry, 06930da does not actually fix anything, I overlooked it :)
Now I'm working on reducing calls to `extend` in `Grape` because they invalidate Ruby's method cache slowing everything down. I applied a *clever* trick with subclassing `Endpoint` and including helpers into this subclass. It also forced me to fix a thread safety issue reported by @etehtsea here: ruby-grape#1201 (comment) because calling `include` twice would be really inconsistent. Also I'm sorry, 06930da does not actually fix anything, I overlooked it :)
Hello!
This is an attempt to fix a most wrong thing in Grape: middleware stack is initializing on every request and even more Grape
dupthem when they're called.I don't know should it be merged or not. It might break someone's code even when specs are green.