-
Notifications
You must be signed in to change notification settings - Fork 191
Fix PathString over-encoding #667
Conversation
c == 0x21 || // ! | ||
c == 0x24 || // $ | ||
c == 0x3B || // ; | ||
c == 0x3D; // = |
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.
why use hex instead of '='
?
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.
&
?
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 have sometimes seen this implemented as a pre-initialized bool array with the max range (0-126) that you just index into.
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.
Good point. that would be faster.
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.
&
is 0x26
included on the fourth line. missed in comment.
Have you tried basic profiling? |
I haven't finished profiling yet. We don't have baseline either, so I'll find some samples and compare the old and new. |
Not really interested in the baseline in this case, just checking for hot spots. |
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 = { |
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.
escapeMap -> ValidPathChars
|
Base on RFC 3986, ensure following characters are not encoded alpha, digit, "-", "_", ".", "~", "@", ":", "/", "!", "$", ";", "=", "'", "(", ")", "*", "+", ","
rebase and squashed. waiting for CI to pass. |
Base on RFC 3986, ensure following characters are not encoded
alpha, digit, "-", "_", ".", "~", "@", ":", "/", "!", "$", ";", "=",
"'", "(", ")", "*", "+", ","
Issue: #660
/cc @Tratcher