Skip to content

Conversation

@VSadov
Copy link
Member

@VSadov VSadov commented Aug 4, 2023

Fixes: #89993

When we did not have a way to disable inlining of threadstatic storage, suppressing inlining of a fraction of threadstatic variables in Checked/Debug builds made sense from code coverage and testing point of view.
Now that we have a compiler switch to disable inlining and have entire platform combinations that do not inline, the value of such instrumentation is not high.

On the other hand, it could result in issues like #89993
The culprit is that there is at least one variable ThreadStatics.t_inlinedThreadStaticBase that we assume to always participate in inlining (if there is inlining at all). The variable caches inlined threadstatic storage and assumes that mere accessing the variable will initiaize the cache. That will not be guaranteed if the variable itself is not on inlined plan.

We could, in theory, just ensure that we do not bump t_inlinedThreadStaticBase off the inlining plan as a part of random selection, but it seems better to stop doing that entirely.

@ghost ghost assigned VSadov Aug 4, 2023
@ghost
Copy link

ghost commented Aug 4, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes: #89993

When we did not have a way to disable inlining of threadstatic storage, suppressing inlining of a fraction of threadstatic variables in Checked/Debug builds made sense from code coverage and testing point of view.
Now that we have a compiler switch to disable inlining and have entire platform combinations that do not inline, the value of such instrumentation is not high.

On the other hand, it could result in issues like #89993
The culprit is that there is at least one variable ThreadStatics.t_inlinedThreadStaticBase that we assume to always participate in inlining (if there is inlining at all). The variable caches inlined threadstatic storage and assumes that mere accessing the variable will initiaize the cache. That will not be guaranteed if the variable is not on inlined plan.

We could, in theory, just ensure that we do not bump t_inlinedThreadStaticBase off the inlining plan as a part of random selection, but it seems better to just stop doing that entirely.

Author: VSadov
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@VSadov
Copy link
Member Author

VSadov commented Aug 5, 2023

This is a very simple change - just removing debug-only code that does more harm than benefit.

@VSadov VSadov requested a review from jkotas August 5, 2023 02:57
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jkotas jkotas merged commit 33c870e into dotnet:main Aug 5, 2023
@VSadov
Copy link
Member Author

VSadov commented Aug 5, 2023

Thanks!!

@VSadov VSadov deleted the noRndUnInline branch August 5, 2023 16:14
@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CheckProjects test failing

2 participants