Skip to content

Pedantic refactor#1

Merged
pgovind merged 5 commits into
pgovind:arm64from
adamsitnik:pedanticRefactor
Aug 18, 2020
Merged

Pedantic refactor#1
pgovind merged 5 commits into
pgovind:arm64from
adamsitnik:pedanticRefactor

Conversation

@adamsitnik

Copy link
Copy Markdown

@pgovind while reviewing dotnet#1445 I've come up with some pedantic refactoring ideas

since my code review was delayed by the 5.0 stuff and now I need these benchmarks to sign off 5.0 I decided to send a PR to your fork to speed up the process

PTAL

Sample results:

Method Toolchain Input Mean Ratio Allocated
ToChars netcoreapp3.1 EnglishAllAscii 17,728.09 ns 1.00 -
ToChars netcoreapp5.0 EnglishAllAscii 17,702.73 ns 1.00 -
IsAscii netcoreapp3.1 EnglishAllAscii 3,673.88 ns 1.00 -
IsAscii netcoreapp5.0 EnglishAllAscii 3,430.67 ns 0.93 -
IsNormalized netcoreapp3.1 EnglishAllAscii 133,466.28 ns 1.00 335120 B
IsNormalized netcoreapp5.0 EnglishAllAscii 130,560.00 ns 0.98 335120 B
ToCharArray netcoreapp3.1 EnglishAllAscii 59,725.94 ns 1.00 335116 B
ToCharArray netcoreapp5.0 EnglishAllAscii 61,426.65 ns 1.02 335117 B
ToChars netcoreapp3.1 EnglishMostlyAscii 72,505.19 ns 1.00 -
ToChars netcoreapp5.0 EnglishMostlyAscii 71,528.86 ns 0.99 -
IsAscii netcoreapp3.1 EnglishMostlyAscii 15.02 ns 1.00 -
IsAscii netcoreapp5.0 EnglishMostlyAscii 11.92 ns 0.79 -
IsNormalized netcoreapp3.1 EnglishMostlyAscii 333,324.30 ns 1.00 335128 B
IsNormalized netcoreapp5.0 EnglishMostlyAscii 413,591.01 ns 1.24 335128 B
ToCharArray netcoreapp3.1 EnglishMostlyAscii 183,631.69 ns 1.00 335126 B
ToCharArray netcoreapp5.0 EnglishMostlyAscii 155,377.17 ns 0.85 335126 B
ToChars netcoreapp3.1 Chinese 132,721.23 ns 1.00 -
ToChars netcoreapp5.0 Chinese 132,349.89 ns 1.00 -
IsAscii netcoreapp3.1 Chinese 27.36 ns 1.00 -
IsAscii netcoreapp5.0 Chinese 24.92 ns 0.91 -
IsNormalized netcoreapp3.1 Chinese 411,800.56 ns 1.00 155961 B
IsNormalized netcoreapp5.0 Chinese 433,824.27 ns 1.05 155960 B
ToCharArray netcoreapp3.1 Chinese 224,659.80 ns 1.00 155960 B
ToCharArray netcoreapp5.0 Chinese 219,481.70 ns 0.98 155960 B
ToChars netcoreapp3.1 Cyrillic 92,179.97 ns 1.00 -
ToChars netcoreapp5.0 Cyrillic 90,463.51 ns 0.98 -
IsAscii netcoreapp3.1 Cyrillic 30.15 ns 1.00 -
IsAscii netcoreapp5.0 Cyrillic 34.55 ns 1.15 -
IsNormalized netcoreapp3.1 Cyrillic 515,270.54 ns 1.00 133640 B
IsNormalized netcoreapp5.0 Cyrillic 360,300.03 ns 0.70 133640 B
ToCharArray netcoreapp3.1 Cyrillic 185,079.52 ns 1.00 133640 B
ToCharArray netcoreapp5.0 Cyrillic 182,600.64 ns 0.99 133640 B
ToChars netcoreapp3.1 Greek 147,888.70 ns 1.00 -
ToChars netcoreapp5.0 Greek 146,859.71 ns 0.99 -
IsAscii netcoreapp3.1 Greek 27.56 ns 1.00 -
IsAscii netcoreapp5.0 Greek 33.26 ns 1.21 -
IsNormalized netcoreapp3.1 Greek 727,875.98 ns 1.00 169352 B
IsNormalized netcoreapp5.0 Greek 533,343.36 ns 0.73 169352 B
ToCharArray netcoreapp3.1 Greek 291,101.71 ns 1.00 169352 B
ToCharArray netcoreapp5.0 Greek 284,594.27 ns 0.98 169352 B

@pgovind

pgovind commented Aug 18, 2020

Copy link
Copy Markdown
Owner

cc @carlossanlop @kunalspathak

Comment thread src/benchmarks/micro/libraries/System.Utf8String/Perf.Utf8String.cs
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<None Update="libraries\System.Utf8String.Experimental\*.txt">
<None Update="libraries\System.Utf8String\*.txt">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@adamsitnik Shouldn't the folder names match the assembly name?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I really don't mind (it helps to type less). Maybe I'm being too pedantic about it 😄 .

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@carlossanlop This is a very good question, however, I am not sure if there is a good answer to it.

The namespace, class name, method name, argument name and value are used to build benchmark ID which is used in the reporting system. Something like namespace.className.methoName(argName: argValue). Example: https://pvscmdupload.blob.core.windows.net/reports/allTestHistory/refs/heads/master_x64_Windows%2010.0.18362/Microsoft.Extensions.Primitives.StringSegmentBenchmark.EndsWith.html

So first of all, we need to choose good names because they are used as IDs to identify benchmarks: https://github.com/dotnet/performance/blob/master/docs/microbenchmark-design-guidelines.md#Benchmarks-are-Immutable

Here I hope that in 6.0 we are simply going to switch from Experimental to just not experimental and I would prefer to avoid using Experimental in the name.

The other problem is that this particular namespace is used to copy the output text files to output folder and the longer it is, the higher chance of hitting too long name path exceptions etc. So in general, the shorter names, the better.

@pgovind pgovind merged commit d028c7a into pgovind:arm64 Aug 18, 2020
@pgovind

pgovind commented Aug 18, 2020

Copy link
Copy Markdown
Owner

Merged. Thanks Adam!!

@pgovind

pgovind commented Aug 18, 2020

Copy link
Copy Markdown
Owner

@adamsitnik , these numbers are from an x64 run right (and no special envs)?

@adamsitnik

Copy link
Copy Markdown
Author

these numbers are from an x64 run right (and no special envs)?

yes, these were Windows x64 numbers with default settings

@adamsitnik adamsitnik deleted the pedanticRefactor branch August 18, 2020 19:56
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.

3 participants