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

Widen ascii to utf16 #39510

Merged
merged 3 commits into from
Aug 10, 2020
Merged

Widen ascii to utf16 #39510

merged 3 commits into from
Aug 10, 2020

Conversation

pgovind
Copy link

@pgovind pgovind commented Jul 17, 2020

Ready for review now.

Implements WidenAsciiToUtf16 from #35034

Perf:

summary:
better: 1, geomean: 2.172
total diff: 1

No Slower results for the provided threshold = 0.01% and noise filter = 0.3ns.

| Faster                                                       | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| ------------------------------------------------------------ | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Experimental.Perf.IsNormalized_WidenAsciiToUtf16 |      2.17 |         65021.15 |         29935.14 |         |

No file given

@jkotas jkotas added the NO-REVIEW Experimental/testing PR, do NOT review it label Jul 17, 2020
@pgovind pgovind force-pushed the WidenAsciiToUtf16 branch from f5b7a06 to 9aeccb9 Compare August 6, 2020 21:51
@pgovind pgovind added area-System.Runtime.Intrinsics and removed NO-REVIEW Experimental/testing PR, do NOT review it labels Aug 7, 2020
@ghost
Copy link

ghost commented Aug 7, 2020

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

@kunalspathak
Copy link
Member

Same here. Please add the benchmark in dotnet/performance.

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

LGTM (with nits)

@pgovind
Copy link
Author

pgovind commented Aug 10, 2020

Thanks for the reviews @eiriktsarpalis @echesakovMSFT and @kunalspathak

@pgovind
Copy link
Author

pgovind commented Aug 10, 2020

CI failures are unrelated. Merging

@pgovind pgovind merged commit b9557bc into dotnet:master Aug 10, 2020
@kunalspathak
Copy link
Member

Could you double check the benchmark IsNormalized_WidenAsciiToUtf16() that you added in dotnet/performance#1445 to see if it is actually hitting the code path. You don't do anything with the return value so does JIT optimize away calls that would have otherwise called the code path you wanted to measure?

@pgovind
Copy link
Author

pgovind commented Aug 11, 2020

Yup, it's hitting the code path. I added an exception that triggered by the benchmarking code:

// BeforeAnythingElse


Unhandled exception.
   Cannot print exception string because Exception.ToString() failed.
ExitCode != 0
// Benchmark Process 4827 has exited with code 134
No more Benchmark runs will be launched as NO measurements were obtained from the previous run!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants