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

Remove paths part size hash from Matcher #28

Merged
merged 3 commits into from
Jul 14, 2020
Merged

Remove paths part size hash from Matcher #28

merged 3 commits into from
Jul 14, 2020

Conversation

matthewmcgarvey
Copy link
Member

@matthewmcgarvey matthewmcgarvey commented Jun 24, 2020

Summary

Refer to #27 for a more detailed explanation of why this change is being made, but TLDR is that this speeds up the case of finding a matching request without sacrificing behavior.

This benchmark update is to better compare the code when it is warmed up. It runs the router much more but fewer times.

Benchmark (with --release)

  • Before: 224 ms
  • After: 212 ms
  • Difference: 12 ms
  • Improvement: 5%

@paulcsmith
Copy link
Member

paulcsmith commented Jun 24, 2020

Will take a look at this soon! One thing to note is that I think those benchmarks are without the --release flag. Sometimes in release mode things can change a lot. So I think it'd be good to check the previous merged change and this one with --release to make sure the numbers really are better. Maybe they're even bigger than we previously thought!

EDIT: One more thought, maybe it should run fewer times, but for much longer. That way it has some time to warm up and may produce more accurate results:

elapsed_times = [] of Time::Span
10.times do
  elapsed = Time.measure do
    100_000.times 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
  elapsed_times << elapsed
end

@matthewmcgarvey
Copy link
Member Author

matthewmcgarvey commented Jun 24, 2020

@paulcsmith Good call! I updated the benchmark and re-ran with release flag and am seeing very different results. I updated the description with the changes.

@matthewmcgarvey
Copy link
Member Author

@paulcsmith Is this ready to be merged?

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.

This looks good to me. I think we'll just have to try it out some apps to make sure we're catching all of the different use cases.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

I think this is good to go. Would love to try this out on some apps, but I think the specs cover everything so this should be good!

@paulcsmith paulcsmith merged commit 95fcaf7 into luckyframework:master Jul 14, 2020
@matthewmcgarvey matthewmcgarvey deleted the temp-2 branch July 29, 2020 17:12
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.

3 participants