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

URI decode path parts #51

Merged
merged 3 commits into from
Jun 1, 2021
Merged

Conversation

kimburgess
Copy link
Contributor

This PR introduces percent-decoding for information contained within path parts.

This ensures that all paths are normalised as per https://datatracker.ietf.org/doc/html/rfc3986#section-7.3 at the first valid opportunity.

Driving reason for this is to prevent the need for explicit decoding within Spider Gazelle's action-controller (which uses this) and could be placed there. This just appears to be the neat point to solve this more generally. If you feel that's a better spot, please do feel free to close this.

@jwoertink
Copy link
Member

Yeah, I think this makes sense. I'd be curious to know if there was any major performance impact by doing this, but I'd imagine it's probably minimal if anything.

@jwoertink
Copy link
Member

yikes... 😶 Average time: 308.7ms -> Average time: 519.2ms... Looks like this change almost doubles the average load time. I based that on running this benchmark in release mode before and after this PR.

I can see how this would be helpful. Do you have any ideas on how we can cut that time down? We could also merge this as is if there's savings somewhere else in this shard.

@kimburgess
Copy link
Contributor Author

Woah, thank you for catching that. Definitely something I missed checking. Leave it with me and I’ll see what I can do!

@kimburgess
Copy link
Contributor Author

That should be a little less intense now. As it peels off the path segments, it will only pipe those containing encoded chars through URI.decode.

Currently this happens strictly for the full path. There's potentially some additional gain possible by reading these segments lazily as the matcher walks the tree, but implementation of that requires a bit of modification to other areas of this lib and would introduce a fair bit of noise to the intent of this PR.

Thanks again for keeping an eye perf.

@jwoertink
Copy link
Member

wow! This even seems to speed it up a bit! 🥳 Average time: 282.7ms

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.

I think this looks pretty good. Overall it's not super complex, and it seems to be a small performance boost too. Thanks for working on this!

@jwoertink jwoertink merged commit d6020f0 into luckyframework:master Jun 1, 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