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

Improve encoding performance #10124

Merged
merged 1 commit into from Mar 13, 2017

Conversation

Projects
None yet
5 participants
@jkotas
Member

jkotas commented Mar 11, 2017

This is low-risk stop gap measure to improve encoding performance because of we may not have enough time to improve the test coverage (dotnet/corefx#16334) and refactor to code to use corefxlab high-performance encoding routines for 2.0.

The change is to avoid passing the key loop control variables by ref to the invalid character fallback routines. Taking address of a variable prevents RuyJIT from enregistering it.

Results: UTF8 decoding 1k of ASCII characters is 1.65x faster, similar for other affected codepaths.

@@ -516,6 +534,7 @@ internal override unsafe int GetCharCount(byte* bytes, int count, DecoderNLS bas
// For fallback we may need a fallback buffer
DecoderFallbackBuffer fallbackBuffer = null;
char* charsForFallback;

This comment has been minimized.

@danmosemsft

danmosemsft Mar 12, 2017

Member

Is this used?

In the log i only see src\System\Text\UTF32Encoding.cs(537,19): warning CS0168: The variable 'charsForFallback' is declared but never used [D:\j\workspace\x64_release_w---0575cb46\src\mscorlib\System.Private.CoreLib.csproj]

@danmosemsft

danmosemsft Mar 12, 2017

Member

Is this used?

In the log i only see src\System\Text\UTF32Encoding.cs(537,19): warning CS0168: The variable 'charsForFallback' is declared but never used [D:\j\workspace\x64_release_w---0575cb46\src\mscorlib\System.Private.CoreLib.csproj]

This comment has been minimized.

@jkotas

jkotas Mar 12, 2017

Member

Fixed.

@jkotas

jkotas Mar 12, 2017

Member

Fixed.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Mar 12, 2017

Member

LGTM although there is a possibly related test failure.

Is there an issue to fix the JIT so it can do this by itself without this ugly workaround?

Member

danmosemsft commented Mar 12, 2017

LGTM although there is a possibly related test failure.

Is there an issue to fix the JIT so it can do this by itself without this ugly workaround?

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Mar 12, 2017

Member

Is there an issue to fix the JIT so it can do this by itself without this ugly workaround?

The analysis to prove that the address taken does not escape gets complex fast. The JIT does a little bit of it today, the C/C++ optimizing backends do some more of it; but you never want to take address of your perf critical loop control locals in any case and you need to structure your code accordingly.

The encoding fallback implementation is pretty poorly structured. I hope we will get a chance to refactor it as part of aligning the implementation with the corefxlab one. The fallback support were not in the code when I have fine tuned the encoding originally. They were added together with the ref on the perf critical locals later by somebody who did not know what he is doing.

Member

jkotas commented Mar 12, 2017

Is there an issue to fix the JIT so it can do this by itself without this ugly workaround?

The analysis to prove that the address taken does not escape gets complex fast. The JIT does a little bit of it today, the C/C++ optimizing backends do some more of it; but you never want to take address of your perf critical loop control locals in any case and you need to structure your code accordingly.

The encoding fallback implementation is pretty poorly structured. I hope we will get a chance to refactor it as part of aligning the implementation with the corefxlab one. The fallback support were not in the code when I have fine tuned the encoding originally. They were added together with the ref on the perf critical locals later by somebody who did not know what he is doing.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Mar 13, 2017

Member

@dotnet-bot test Windows_NT x86 Checked Build and Test please

Member

jkotas commented Mar 13, 2017

@dotnet-bot test Windows_NT x86 Checked Build and Test please

@jkotas jkotas merged commit 4259696 into dotnet:master Mar 13, 2017

14 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT arm64 Cross Debug Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details

@jkotas jkotas deleted the jkotas:encoding-perf branch Mar 13, 2017

jorive added a commit to guhuro/coreclr that referenced this pull request May 4, 2017

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment