Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[no merge] Introduce a new attribute indicating a method uses StackCrawlMark #7449

Closed
wants to merge 2 commits into from

Conversation

jamesqo
Copy link

@jamesqo jamesqo commented Oct 2, 2016

Currently we assume a method has a StackCrawlMark local var if it is in mscorlib and marked NoInlining. This can lead to potential pessimizations when we want to use NoInlining for other purposes (namely, performance), e.g. #7007, #6890.

This PR adds a new System.Runtime.CompilerServices.UsesStackCrawlMarkAttribute, and marks methods that use StackCrawlMark w/ that instead of NoInlining. (relevant commit) To be honest, I am not quite sure I know what I'm doing, I basically copy-and-pasted the logic to specially recognize NativeCallableAttribute and just replaced it with UsesStackCrawlMark.

@jkotas is that all that needs to be done?

@jkotas
Copy link
Member

jkotas commented Oct 3, 2016

@jkotas is that all that needs to be done?

There are likely tweaks needed in more places to make it actually work (the CI tests are failing...).

Also, the JIT throughput impact should be checked. The custom attribute looks in JIT-EE interface tend to have measurable impact on JIT throughput.

cc @dotnet/jit-contrib

@jamesqo
Copy link
Author

jamesqo commented Oct 3, 2016

(the CI tests are failing...)

Hm, I just noticed that I have the attribute defined in the macro as System.Threading._ when it should be System.Runtime.CompilerServices._. I'll try changing that and see if they pass. (edit: actually it is Runtime.CompilerServices, I had already fixed it)

The custom attribute looks in JIT-EE interface tend to have measurable impact on JIT throughput.

This is only being checked for mscorlib though (lookup is preceded by IsSystem() check), which is precompiled.

@pgavlin
Copy link

pgavlin commented Oct 3, 2016

@jamesqo: even though this is only being checked for the core library, it may impact the time it takes for the JIT to precompile the core library (which is a metric that we track pretty closely). I'll take a look and see if there's any significant throughput hit here.

@jkotas
Copy link
Member

jkotas commented Oct 3, 2016

@pgavlin I believe it impacts all code - when it is calling methods in corelib.

@pgavlin
Copy link

pgavlin commented Oct 3, 2016

@jkotas: yes, you're correct. Serves me for not looking closely enough at the context for the method that changed. :)

@jkotas jkotas added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 3, 2016
@jamesqo
Copy link
Author

jamesqo commented Oct 4, 2016

If custom attributes have too much overhead, then maybe we could have contradictory flag values that are unlikely to show up for actual uses, e.g. we could mark StackCrawlMark methods w/ AggressiveInlining | NoInlining.

@danmoseley
Copy link
Member

@jamesqo are you still working on this PR?

@jamesqo
Copy link
Author

jamesqo commented Nov 12, 2016

@danmosemsft I can move this to an issue for now. Sorry, I seem to be forgetting about a lot of my coreclr PRs (in particular ones that change the C++ side of things, since the runtime takes an hour to build).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
* NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants