Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Threadpool autorelease #47592
Threadpool autorelease #47592
Changes from 13 commits
aee6785
a6b59df
b0938eb
ed44c1e
d15f47b
284a865
43bd530
c4ab7e7
5671fa8
c7e2934
eb2e39d
d6bab12
9bd2af5
be9d6df
1c85528
b6053fd
c9eea84
c803877
483db22
f6c9313
bb19d85
a46dd70
0fe7547
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect these two AppContext switches to always default to false? The current naming pattern we've used is to use the pattern EnableXXX when XXX is off by default (or will be off by default in the future - like with BinaryFormatter).
My naive understanding from looking at the code below is this switch only affects MacOS. If you are on any platform other than Apple, this switch doesn't do anything. If you are on an Apple OS that isn't MacOS, the behavior will be "on".
Can you also update https://github.com/dotnet/runtime/blob/master/docs/workflow/trimming/feature-switches.md with your new feature switch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, there is a bit of efficiency that can be gained if we put the reading of the switch under old-fashined ifdef. As it is written, we will read the switch even on platforms where it does nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm questioning if we need a full-blown "feature switch" for this. Not all AppContext switches need to follow the feature switch design (added ILLink.Substitutions.xml entry, and MSBuild entry). If this only affects MacOS, we aren't really concerned about trimming out every bit of code on MacOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lambdageek specifically requested that we add a linker substitution for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't fully understand the value. Adding and maintaining these feature switches isn't "free". They are kind of like public API that we need to document and maintain compatibility.
The only thing I'm seeing this enable is if you wanted to trim an
osx
self contained application, you could set this switch to trim this code. Is it a lot of code being trimmed? Size ofosx
apps aren't really a major concern. I guess I just don't see the value outweighing the cost here.cc @stephentoub @vitek-karas @marek-safar as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the existing default is true /cc @rolfbjarne
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing default is
true
(and that's what it should be too in .NET 6).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing default on CoreCLR on macOS is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the following sound to everyone? @rolfbjarne @jkotas
The default for in the runtime for when the flag is not specified is
false
. So, for the net6.0 TFM without a target platform, the default isfalse
.The MSBuild SDK tooling for
net6.0-macos
,net6.0-ios
, etc. sets the feature flag default totrue
in MSBuild. That way people migrating tonet6.0-*
from the Xamarin world to the TFMs with target platform APIs will continue to have the same default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be the other way.
false
is the default in the code. The feature switch is usually the opposite of the default. See GlobalizationMode.Invariant above. The default in the code isfalse
and setting the feature switch totrue
trims away ICU dependencies.Which value for this setting trims code?
true
orfalse
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting this switch to
false
(which is the default in code) should trim out the call toDispatchItemWithAutoreleasePool
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Just know that the code won't be trimmed unless someone explicitly sets the switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the feature switch is now set up correctly.
I'm wondering how much code is actually going to be trimmed. It looks like only the
ThreadPoolWorkQueue.DispatchItemWithAutoreleasePool
static method, and 2 extern methods will be able to be trimmed. I still question if this is worth the effort of defining and maintaining a feature switch here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread pool's performance is very sensitive to inlining and branching decisions. By enabling trimming here, we can make some of these branches disappear and eek out a little more performance. This one isn't particularly focused on size reduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a
partial class
. Isn't there a file where you can put this property so it doesn't need to be duplicated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There actually isn't.
ThreadPool.cs
didn't actually have theThreadPool
type and I don't want to make a newThreadPool.cs
in the same PR where I'm renaming the old one because once it's squashed, the git history will get all messed up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put it in
ThreadPoolWorkQueue.AutoreleasePool.OSX.cs
?I understand that it is a minor violation of coding conventions, but I think it is acceptable.
Also, we won't be unnecessarily reading the property on non-OSX platforms anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!