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

Xs dancer core request #132

Merged
merged 6 commits into from Dec 28, 2012
Merged

Xs dancer core request #132

merged 6 commits into from Dec 28, 2012

Conversation

celogeek
Copy link
Contributor

I have add improvement on Dancer::Core::Request.

If URL::Encode::XS is installed, it will use it instead of pure perl _url_encode
To parse the params I check if CGI::Deurl::XS is present, and I use parse_query_string from it.

Both is a a lot of faster than pure perl version.

I have made a speed test to compare,

pure perl (master branch): 3472 Dancer::Core::Request->new per second
with XS (my branch) : 3900 Dancer::Core::Request->new per second

I have add a comment in the request for extra speed, and add a diag in the request.t to tell people to install them for extra speed.

It is not a dependencies, but it speed up dancer if they are present.

@bigpresh
Copy link
Member

Looks pretty clean & sane, and the speed boost is nice.

I'm mildly wary in case the XS versions differ from the pure-Perl versions at all in any way (now or in the future) which could cause odd side effects / hard to track bugs, though. Perhaps we could add a test which, if the XS modules are detected, compares the result of CGI::Deurl::XS::parse_query_string() and URL::Encode::XS::url_decode() against the pure-Perl equivalents, so that if the behaviour begins to differ at any point, we'll find out.

So, cautious approval here.

@celogeek
Copy link
Contributor Author

You are right. I was thinking of doing the request test twice, one with and one without the XS module if present.
I will work on it.

@celogeek
Copy link
Contributor Author

I have rebase the code on the latest master to permit an easy merge.

bigpresh added a commit that referenced this pull request Dec 28, 2012
@bigpresh bigpresh merged commit 4473845 into PerlDancer:master Dec 28, 2012
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.

None yet

2 participants