Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Use ordinal when checking for hex prefix #5027

Merged
merged 1 commit into from
Dec 17, 2015

Conversation

ellismg
Copy link
Contributor

@ellismg ellismg commented Dec 16, 2015

In general lingustic comparsion is slower than ordinal comparision.
When using ICU for globalization, the lingustic StartsWith code path
actually ends up doing a lot of work inside ICU itself. While we'll try
to make things better (see dotnet/corefx#3672) for common cases like
ASCII only, if code doesn't need ligustic comparision the best practice
is to not request it.

BaseNumberConverter was using a lingustic StartsWith to detect a hex
prefix when trying to parse a number but it is better served by just
always using ordinal comparisons.

This results in a ~35% QPS improvement in some very barebones ASP.NET
scenarios (like the one outlined in richardkiene/CoreCLRWebAPISample) on
my local machine.

In general lingustic comparsion is slower than ordinal comparision.
When using ICU for globalization, the lingustic StartsWith code path
actually ends up doing a lot of work inside ICU itself. While we'll try
to make things better (see dotnet/corefx#3672) for common cases like
ASCII only, if code doesn't need ligustic comparision the best practice
is to not request it.

BaseNumberConverter was using a lingustic StartsWith to detect a hex
prefix when trying to parse a number but it is better served by just
always using ordinal comparisons.

This results in a ~35% QPS improvement in some very barebones ASP.NET
scenarios (like the one outlined in richardkiene/CoreCLRWebAPISample) on
my local machine.
@ellismg
Copy link
Contributor Author

ellismg commented Dec 16, 2015

@ianhays or @krwq Can you please take a look?

/cc @richardkiene

@ianhays
Copy link
Contributor

ianhays commented Dec 16, 2015

LGTM. Nice little fix :) Thanks, Matt.

ellismg added a commit that referenced this pull request Dec 17, 2015
Use ordinal when checking for hex prefix
@ellismg ellismg merged commit 329e828 into dotnet:master Dec 17, 2015
@viktor-evdokimov
Copy link

That might be naive question, but shouldn't

 else if (this.AllowHex && text.StartsWith("0x", StringComparison.OrdinalIgnoreCase)
                                      || text.StartsWith("&h", StringComparison.OrdinalIgnoreCase))

be

else if (this.AllowHex && (text.StartsWith("0x", StringComparison.OrdinalIgnoreCase)
                               || text.StartsWith("&h", StringComparison.OrdinalIgnoreCase)))

i.e. check for prefix if flag is true?

@JesperTreetop
Copy link
Contributor

@ianhays I'm wondering what @ojosdegris wondered too. This is intact in the current version.

@ianhays
Copy link
Contributor

ianhays commented Feb 11, 2016

Well it's consistent with what was there before this commit, but it does seem like an issue.

@JesperTreetop
Copy link
Contributor

The .NET Framework version does this too so this may be bug compatibility.

@JesperTreetop
Copy link
Contributor

Then again, if bug compatibility is to be maintained, 0X should be matched without the hex flag.

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.

9 participants