-
-
Notifications
You must be signed in to change notification settings - Fork 103
Remove Mustermann leaf matcher to improve performance #284
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
Open
dcr8898
wants to merge
30
commits into
hanami:main
Choose a base branch
from
dcr8898:router-saga
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 20 commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
fd355d2
store Mustermann matchers (instead of routes) in leaves
dcr8898 b82b44d
split routes and paths using string instead of regular expression
dcr8898 6c87d3f
basic proof of concept for Mustermann removal
dcr8898 891d752
implement basic regexp constraints
dcr8898 dcb35a3
refactor unit tests for leaf to match updated leaf interface
dcr8898 3962c53
remove unnecessary guard clause in leaf#match
dcr8898 ecf0f93
refactor unit tests for node to match updated node interface
dcr8898 f650ebd
remove deprecated attr_reader for :to in node
dcr8898 3ca7d5c
refactor unit tests for trie to match use standard regex (not posix)
dcr8898 900b6f5
skip remaining failing tests as not presently supported
dcr8898 1aea397
appease rubocop
dcr8898 a134c19
unskip generation_spec test for route with optional clause
dcr8898 af8d51d
add tests for routes with optional clauses in recognition_spec
dcr8898 3125d49
create InvalidRouteDefinitionError to use with broken optional clauses
dcr8898 3840939
update Leaf#match to ignore constraints placed on non-existent parama…
dcr8898 e89a025
implement optional route definition clauses
dcr8898 65dd7a1
remove namesaces when raising InvalidRouteDefinition in Router#add_op…
dcr8898 8110cb0
replace regex test for variable segments with string test
dcr8898 3b44f9c
remove some indirection in Trie when parsing segments
dcr8898 43ef3d1
convert globbed route detection from regexp to string method
dcr8898 4eedc9c
inline path splitting in Trie#find
dcr8898 f1c9bda
remove unnecessary local variable derived_paths in Router#add_optiona…
dcr8898 8f3e8e9
update Node#put unit test to expect key without ':' prefix
dcr8898 8de377b
update Node to transform param keys on the fly, instead of in Leaf
dcr8898 183639a
do not process param_keys in Leaf
dcr8898 e96b123
update Leaf#match unit test to remove leading ':' in test param_keys …
dcr8898 bc81af1
simplify Trie#find method
dcr8898 4286baf
improved the simplification of Trie#find method :)
dcr8898 341bb81
use #slice instead of #[] in Trie#find
dcr8898 a61b405
use interpolation instead of << in Router#add_optional_routes
dcr8898 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating this intermediary array to iterate over it, could we repeat the conditional below for each new_path we create? Less DRY but possibly more performant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try this. It makes sense.
This point also highlights the difference between writing speed-sensitive library code and writing apps: apps are optimized for maintainability, while the library code is optimized for performance. That often requires abandoning a lot of code hygiene practices that we usually promote (like DRY in this case). Many of those good practices are based on indirection, like using constants to label magic values, but that indirection impacts performance. This was the basis of my question above: How much performance is enough? I'm not sure it's worth in-lining all of the constants in the Router, for example, but others might disagree.
It's also hard to know if these micro-optimizations have an effect when we are using gross measurements like r10k. With that said, I'm curious to try this one and see what happens. 🤓
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a fresh look at this, since it only applies to optional routes, which are uncommon and not exercised by this benchmark at all, I wonder if this change is worthwhile. Thoughts?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Fast Ruby, interpolation should be slightly faster than the shovel method. Do you think this change is worth making?
This is what I was getting at with the "Question for All." I like searching for performance tweaks, and I think a router should be as performant as possible, but I agonize over the loss of readability and maintainability. In this case, I think switching to interpolation is probably okay, because the code remains understandable (I think). But I hesitate to un-DRY the code for what is a rare usage (optional clauses).
What do you all think?