router icon indicating copy to clipboard operation
router copied to clipboard

Performance regression from 2.1 to 2.2

Open cllns opened this issue 11 months ago • 4 comments

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

Number of routes hanami-router 2.1 hanami-router 2.2 Percent decrease of RPS
10 162282 RPS 150325 RPS 7.3%
100 125929 RPS 116285 RPS 7.7%
1000 93324 RPS 45422 RPS 51%
10000 74545 RPS 5480 RPS 93%

Runtime with startup

Number of routes hanami-router 2.1 hanami-router 2.2 Percent slower
10 0.218s 0.227s 4.1%
100 0.251s 0.266s 6.0%
1000 0.357s 0.559s 56%
10000 1.186s 4.098s 245%

Graphs

rps log_rps runtime_with_startup

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

  1. gem uninstall hanami-router to get a clean slate
  2. gem install hanami-api gruff (gruff is for generating graphs)
  3. gem install hanami-router --version 2.1.0
  4. gem list hanami-router should only show 2.1.0
  5. Clone https://github.com/jeremyevans/r10k
  6. From that folder, rake bench R10K_APPS="hanami-api"
  7. Add a suffix to every file in data/, so they don't get overwritten when we run it again.
  8. gem install hanami-router --version 2.2.0
  9. rake bench R10K_APPS="hanami-api" again
  10. Open the new data/*.csv files and copy over the rows from the suffixed files, and update each row to reflect which version of hanami-router was used for each.
  11. Finally, run rake graphs to generate graphs from those CSV's

Here 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.

@kyleplump @dcr8898 would either or both of you be willing to look into this? The apps generated in apps/ may be useful, especially hanami-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

cllns avatar Dec 31 '24 15:12 cllns

@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 avatar Dec 31 '24 17:12 kyleplump

@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!

cllns avatar Dec 31 '24 20:12 cllns

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 https://github.com/hanami/router/issues/278#issuecomment-2566700991, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7IFGDAJKNM2D54OTI7I3T2IL55HAVCNFSM6AAAAABUNTN45KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRWG4YDAOJZGE . You are receiving this because you were mentioned.Message ID: @.***>

dcr8898 avatar Dec 31 '24 22:12 dcr8898

hi @dcr8898, @cllns

opened an issue related to this: #279 - would love to hear what you guys think about it!

kyleplump avatar Dec 31 '24 23:12 kyleplump