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

Avoid passing stack crawl mark unnecessarily deep in the call stack. #21783

Merged
merged 2 commits into from Jan 4, 2019

Conversation

Projects
None yet
2 participants
@filipnavara
Copy link
Collaborator

filipnavara commented Jan 3, 2019

TypeName::GetTypeWorker was passed both a stack mark and requesting assembly. Only the requesting assembly is actually needed and the stack mark can be resolved earlier if necessary.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 3, 2019

Can this introduce (expensive) stackcrawl for caller assembly in some cases where it does not happen today? If yes, what are those cases and how important are they?

@filipnavara

This comment has been minimized.

Copy link
Collaborator Author

filipnavara commented Jan 4, 2019

Can this introduce (expensive) stackcrawl for caller assembly in some cases where it does not happen today?

As far as I can see it should not add any new stackcrawl that was not there before.

The code paths in TypeName::GetTypeWorker that didn't resolve the stack mark in the parameter can only occur if pRequestingAssembly != NULL || pAssemblyGetType != NULL. In every other case the stack mark was eventually resolved.

I moved up the stack mark resolution to TypeName::GetTypeManaged. GetTypeManaged always called GetTypeWorker with pRequestingAssembly: NULL. Thus we only need to consider the pAssemblyGetType != NULL code path.

There are three calls to TypeName::GetTypeManaged. Two of them don't pass in the stack mark, so there's nothing to resolve. The third one passes a stack mark, but it sets pAssemblyGetType: NULL. Thus even for the last one the stack mark resolution would always happen even before the change.

@filipnavara

This comment has been minimized.

Copy link
Collaborator Author

filipnavara commented Jan 4, 2019

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

@filipnavara

This comment has been minimized.

Copy link
Collaborator Author

filipnavara commented Jan 4, 2019

On second thought I can move the resolution from TypeName::GetTypeManaged to RuntimeTypeHandle::GetTypeByName since it's the only call site that actually uses it.

@filipnavara filipnavara changed the title Avoid passing stack crawl mark unnecessarily deep in the call stack. WIP: Avoid passing stack crawl mark unnecessarily deep in the call stack. Jan 4, 2019

@filipnavara

This comment has been minimized.

Copy link
Collaborator Author

filipnavara commented Jan 4, 2019

Additionally, one stack crawl is saved if GetTypeManaged gets to the fallback case with period prefixes where GetTypeWorker is called twice. Admittedly that's not a common case, but it's an improvement nonetheless.

@filipnavara filipnavara changed the title WIP: Avoid passing stack crawl mark unnecessarily deep in the call stack. Avoid passing stack crawl mark unnecessarily deep in the call stack. Jan 4, 2019

@filipnavara

This comment has been minimized.

Copy link
Collaborator Author

filipnavara commented Jan 4, 2019

@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 4, 2019

@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness

@jkotas

jkotas approved these changes Jan 4, 2019

Copy link
Member

jkotas left a comment

Thanks

@jkotas jkotas merged commit 2ee2cb3 into dotnet:master Jan 4, 2019

30 checks passed

CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked crossgen_comparison Build and Test Build finished.
Details
Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Build finished.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Windows_NT arm Cross Debug Innerloop Build Build finished.
Details
Windows_NT arm64 Cross Debug Innerloop Build Build finished.
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x86 Release Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
license/cla All CLA requirements met.
Details

@filipnavara filipnavara deleted the filipnavara:stackcrawl2 branch Jan 4, 2019

sandreenko added a commit to sandreenko/coreclr that referenced this pull request Jan 8, 2019

Avoid passing stack crawl mark unnecessarily deep in the call stack. (d…
…otnet#21783)

* Avoid passing stack crawl mark unnecessarily deep in the call stack.

* Move stack crawl from TypeName::GetTypeManaged to RuntimeTypeHandle::GetTypeByName.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment