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

Remove NoInlining/StackCrawlMarks from Tasks #9342

Merged
merged 2 commits into from Feb 5, 2017

Conversation

Projects
None yet
5 participants
@stephentoub
Copy link
Member

stephentoub commented Feb 4, 2017

These crawl marks in Tasks aren't relevant in coreclr (they're passed down to ExecutionContext, which in coreclr just ignores them), yet they required NoInlining, which does impact coreclr. Removing them all from tasks.

cc: @jkotas, @kouvel

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 5, 2017

Delete the method that takes StackCrawlMark from ExecutionContext?

LGTM otherwise.

@benaadams

This comment has been minimized.

Copy link
Collaborator

benaadams commented Feb 5, 2017

Has conflicts

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented Feb 5, 2017

Has conflicts

Yes, I'm working on this.

stephentoub added some commits Feb 4, 2017

Remove NoInlining/StackCrawlMarks from Tasks
These crawl marks in Tasks aren't relevant in coreclr (they're passed down to ExecutionContext, which in coreclr just ignores them), yet they required NoInlining, which does impact coreclr.  Removing them all from tasks.
Address PR feedback and other cleanup
Addressed PR comment about removing ExecutionContext.Capture(ref StackCrawlMark, ...), which led me to remove NoInlining/StackCrawlMarks from some other places in Timer, Thread, ThreadPool, and Overlapped.

In doing so, I found several other unnecessary members on ExecutionContext that could be removed with minor tweaks elsewhere in the source, e.g.
- Dispose is a nop, so we can remove explicit try/finally's to clean it up
- ExecutionContext.Run(..., bool) just ignores the bool arg, so it can be removed and all call sites redirected (but I've left it in EC, as it appears it's used via internals visible from a library in corefx)
- FastCapture() just calls Capture, so the call sites can be changed (but I've left it in EC for a similar reason)
- PreAllocatedDefault can be removed in favor of Default; etc.
- ExecutionContext.Capture itself checks whether flow is suppressed; doing a check before it adds a TLS lookup and in doing so optimizes for an uncommon case in exchange for making the common case more expensive.  I've removed those checks.

And in the process, I also noticed that several lazily initialized delegates that no longer need to be, and cleaned those up.  These were lazy due to needing to be for security checks that aren't relevant in coreclr.

@stephentoub stephentoub force-pushed the stephentoub:tasks_inlining branch from 1eb6ff0 to b72fc3a Feb 5, 2017

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented Feb 5, 2017

Delete the method that takes StackCrawlMark from ExecutionContext?

It was still being used by some other calls in Timer, Overlapped, ThreadPool, and Thread, so I went through and cleaned up those as well, and deleted this Capture overload. As long as I was doing so, I also cleaned up a few other things I noticed along the way.

@stephentoub stephentoub merged commit a8c08f3 into dotnet:master Feb 5, 2017

13 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 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

@stephentoub stephentoub deleted the stephentoub:tasks_inlining branch Feb 5, 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