Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Fix PathString over-encoding #667

Merged
merged 1 commit into from
Jul 8, 2016
Merged

Fix PathString over-encoding #667

merged 1 commit into from
Jul 8, 2016

Conversation

troydai
Copy link
Contributor

@troydai troydai commented Jul 7, 2016

Base on RFC 3986, ensure following characters are not encoded

alpha, digit, "-", "_", ".", "~", "@", ":", "/", "!", "$", ";", "=",
"'", "(", ")", "*", "+", ","

Issue: #660

/cc @Tratcher

c == 0x21 || // !
c == 0x24 || // $
c == 0x3B || // ;
c == 0x3D; // =
Copy link
Member

Choose a reason for hiding this comment

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

why use hex instead of '='?

Copy link
Member

Choose a reason for hiding this comment

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

&?

Copy link
Member

Choose a reason for hiding this comment

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

I have sometimes seen this implemented as a pre-initialized bool array with the max range (0-126) that you just index into.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. that would be faster.

Copy link
Contributor Author

@troydai troydai Jul 8, 2016

Choose a reason for hiding this comment

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

& is 0x26 included on the fourth line. missed in comment.

@Tratcher
Copy link
Member

Tratcher commented Jul 8, 2016

Have you tried basic profiling?

@troydai
Copy link
Contributor Author

troydai commented Jul 8, 2016

I haven't finished profiling yet. We don't have baseline either, so I'll find some samples and compare the old and new.

@Tratcher
Copy link
Member

Tratcher commented Jul 8, 2016

Not really interested in the baseline in this case, just checking for hot spots.

@troydai
Copy link
Contributor Author

troydai commented Jul 8, 2016

A simplest local profiling using this sample: https://github.com/troydai/HttpAbstractions660/blob/master/perf.sh

5000 requests, 20 currency, RPS improves from 1306 to 2220.

{
internal class PathStringHelper
{
private static bool[] escapeMap = {
Copy link
Member

Choose a reason for hiding this comment

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

escapeMap -> ValidPathChars

@Tratcher
Copy link
Member

Tratcher commented Jul 8, 2016

:shipit:

Base on RFC 3986, ensure following characters are not encoded

alpha, digit, "-", "_", ".", "~", "@", ":", "/", "!", "$", ";", "=",
"'", "(", ")", "*", "+", ","
@troydai
Copy link
Contributor Author

troydai commented Jul 8, 2016

rebase and squashed. waiting for CI to pass.

@troydai troydai merged commit 150b470 into dev Jul 8, 2016
@troydai troydai deleted the troy-660 branch July 8, 2016 23:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants