Repository navigation
Conversation
dblock
left a comment
There was a problem hiding this comment.
Nice to see you here again @sferik! Interesting idea. I would want to hear an enthusiastic yes from @ericproulx who has been doing performance improvements left and right in grpe to merge it.
Nit, I'd refactor GHA workflows to be reusable so that they don't have to be copy-pasted all over, like we do here.
Thinking out loud, expanding CI to also check perf is something I'm 50/50 on. CI needs to get faster, not slower, to enable AI agents and benchmarks are as expensive as it gets.
Duel (https://github.com/sferik/duel-ruby) runs the benchmarks in duel/ on the branch a pull request targets and on the pull request, side by side, and fails the Perf workflow when the pull request is slower or allocates more. The 39 benchmarks send requests through an API's Rack interface in memory: routing among 200 routes, nested namespaces, requirements and wildcards, 404 and 405, params coercion and validation (query string, form, JSON body, nested arrays, a custom validator, a 400), declared, JSON formatting, present, content negotiation, error! and rescue_from, callbacks and helpers, path, header and param versioning, mounting, and compiling an API. Comparing master with itself passed three times in a row. An extra dup in Router#process_route failed 17 benchmarks on allocations, and reading the attribute three times in CoerceValidator#validate_param! failed 11 on instructions and CPU time.
|
Thanks @sferik, this is a nice piece of work. The benchmarks cover the paths I've spent the most time on lately: the router's fast paths, the versioner cascade, @dblock I'm a yes on a trimmed-down version, details below. What convinced me. The run on this PR changes nothing in The part I'd value most is the allocation counts. They're near-exact per request, so "this PR allocates more on the hot path" becomes a hard signal rather than a statistical one. What it will and won't catch. Some benchmarks are noisy on shared runners: What I'd change before merging:
Smaller things: the CHANGELOG entry is missing. It would also help to add a line on how |
|
@ericproulx Thank you for the thoughtful review. I will work on making these changes and push an updated review shortly.
I have added this feature on Duel’s main branch, which I’m hoping to release tomorrow. I’ll be sure to update the PR to use that as well. |
This adds Duel, a gem I wrote to catch performance regressions in CI. On every pull request it runs the benchmarks in
duel/againstmasterand the branch side by side, in alternating worker processes, and fails the build if the branch is slower or allocates more. When something regresses, it profiles both versions with StackProf and says which method the extra time went to.There are 39 benchmarks, and every one is a request sent through an API's Rack interface in memory, the way a server would hand it over, with no server or sockets involved. I went with whole requests rather than calling internal classes because that's what Grape users actually pay for, and because the router, the middleware stack and the endpoint have been tuned so carefully that most of the interesting work happens between them. The env for each request is built outside the measured time, since Grape and Rack write into the env and read
rack.input, so reusing one would measure cached work. Each benchmark also checks its status and part of its body once before measuring, so none of them can quietly end up timing a 404.grape-entity isn't in the development bundle, so the
presentbenchmarks use a small representer class thatpresentcalls the same way it calls an Entity. Compiling a 200 route API took about 40 ms, which is longer than a benchmark should run, so the compile benchmark uses 20 routes and callschange!before eachcompile!.A full comparison takes about 10 to 12 seconds here when nothing regressed.
The Duel job only reads. The comment on a pull request comes from a second workflow,
perf-comment.yml, which runs none of the pull request's code, onlyduel commentfrom Duel's gem, and hides Duel's earlier comments as outdated so the newest one is the one in view. If a change is slower on purpose, aduel: acceptlabel lets it through, and adding it runs the Duel job again.