-
-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance regression from 2.1 to 2.2 #278
Comments
@cllns that "345% slower" metric made me physically recoil 😐 i suspect it might be because of the fact that 2.2 delays creating Mustermann matchers - leading to potentially some significant overhead that didn't exist before. i'd be happy to reproduce and experiment! |
@kyleplump Sorry about that! 10,000 routes is a situation we don't need to optimize for but it helps illustrate the problem and hopefully make it faster to find the problem to fix it. Also, I messed up the "percent slower" computation! Also I messed up the calculation, so it's "only" 245% slower. Small solace I guess 😅 Thanks for looking into it! |
Hi, Sean! Thank you for bringing this to light! It's definitely from our
refactor of the trie implementation. In fact, I wouldn't call this a
regression. It's a straight degradation. 🤣😭😰
I don't think the problem is the Mustermann matchers. This implementation
creates far fewer than before, and doesn't create one until there is a
potential matching route to match against.
I believe the algorithm is sound, but the implementation is inefficient. I
would love to work on it further with Kyle, you, and/or anyone else with
performance experience. I have none and was not sure how to test that at
the time.
Maybe we can arrange a time together? Mastodon might be the best way to
coordinate.
Thanks again! Happy New Year! 🥳
Damian
…On Tue, Dec 31, 2024, 3:36 PM Sean Collins ***@***.***> wrote:
@kyleplump <https://github.com/kyleplump> Sorry about that! 10,000 routes
is a situation we don't need to optimize for but it helps illustrate the
problem and hopefully make it faster to find the problem to fix it.
Thanks for looking into it!
—
Reply to this email directly, view it on GitHub
<#278 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC7IFGDAJKNM2D54OTI7I3T2IL55HAVCNFSM6AAAAABUNTN45KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRWG4YDAOJZGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I found a large performance regression from hanami-router 2.1 to 2.2.
I've compiled figures and graphs below using r10k. Special thanks to @jeremyevans for building r10k; it's a great tool.
I did these using hanami-api, which is a thin layer on top of hanami-router. Testing the full hanami gem has similar results, but hanami-api is closer to hanami-router. You can also test hanami-router yourself if you'd like (see below).
RPS
Runtime with startup
Graphs
All generated from M1 Macbook Pro w/ 16GB of RAM, with Ruby 3.3.6 (not 3.4), and some normal apps in the background (not isolated at all)
Reproduction steps
gem uninstall hanami-router
to get a clean slategem install hanami-api gruff
(gruff is for generating graphs)gem install hanami-router --version 2.1.0
gem list hanami-router
should only show 2.1.0rake bench R10K_APPS="hanami-api"
data/
, so they don't get overwritten when we run it again.gem install hanami-router --version 2.2.0
rake bench R10K_APPS="hanami-api"
againrake graphs
to generate graphs from those CSV'sHere are the results I got.
Requests per scond: rps.csv
Runtime including startup: runtime_with_startup.csv
Also here's a memory file I did too, but the figures are almost the same so it doesn't look like a memory issue: memory.csv
Suspected source of regression
I suspect it has to do with changing the trie implementation in #273.
@chongfun @dcr8898 would either or both of you be willing to look into this? The apps generated in
apps/
may be useful, especiallyhanami-api_4_10.rb
since that's where things get really bad.I also wrote a builder that uses hanami-router directly. It's slightly faster than hanami-api, as one would expect, but not drastically different. https://github.com/cllns/r10k/blob/add-hanami-router/builders/hanami-router.rb
The text was updated successfully, but these errors were encountered: