Skip to content
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

Optimizations #54

Merged
merged 2 commits into from
Sep 15, 2021
Merged

Optimizations #54

merged 2 commits into from
Sep 15, 2021

Conversation

asterite
Copy link
Contributor

A couple of optimizations.

I used this benchmark (I replaced the contents of "benchmark.cr" with this):

require "benchmark"
require "./lucky_router"

router = LuckyRouter::Matcher(Symbol).new

router.add("get", "/users", :index)
router.add("post", "/users", :create)
router.add("get", "/users/:id", :show)
router.add("delete", "/users/:id", :delete)
router.add("put", "/users/:id", :update)
router.add("get", "/users/:id/edit", :edit)
router.add("get", "/users/:id/new", :new)

Benchmark.ips do |x|
  x.report("router") do
    router.match!("post", "/users")
    router.match!("get", "/users/1")
    router.match!("delete", "/users/1")
    router.match!("put", "/users/1")
    router.match!("get", "/users/1/edit")
    router.match!("get", "/users/1/new")
  end
end

Output in master:

router 336.80k (  2.97µs) (± 7.73%)  3.12kB/op  fastest

Output in this PR:

router 489.51k (  2.04µs) (± 4.33%)  2.02kB/op  fastest

The second commit makes the code slightly longer or harder to understand, so feel free to ignore this PR or change it.

It's immutable and it's instantiated once per route match. Avoiding this
allocation gives a small performance boost.
This further improves the router's performance.
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Thanks for this! 😄 It looks great to me 🥳

@jwoertink jwoertink merged commit 11bdf52 into luckyframework:master Sep 15, 2021
@asterite asterite deleted the optimizations branch September 15, 2021 15:18
@asterite
Copy link
Contributor Author

Do you think it's worth it for me to send a next PR with more optimizations? I was able to reach something like this:

router 712.03k (  1.40µs) (± 4.64%)  1.33kB/op  fastest

This is by using a StaticArray on the heap of some size (say, 64 elements). If the route to match has less than 64 segments we put their values there, avoiding an array allocation on every match. Then we turn that into a Slice and use that for matching. Otherwise, we can allocate a new array, but that should be rare (a path with more than 64 segments).

But maybe it's too low-level by then... do you have any idea how much the router's performance affects overall endpoint performance?

@jwoertink
Copy link
Member

Yeah! I say let's do it. This router is actually used by actually used by SpiderGazelle too https://github.com/spider-gazelle/action-controller/blob/master/shard.yml#L12 so it not only benefits us, but it benefits them as well!

do you have any idea how much the router's performance affects overall endpoint performance?

It does help a ton, especially with larger apps adding more and more routes. The one part that can still affect it though is that once in Lucky, we still have some macro code to pick apart the routes https://github.com/luckyframework/lucky/blob/master/src/lucky/routable.cr#L193. But I guess optimizing that side is another story.

@asterite asterite mentioned this pull request Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants