Skip to content

[NativeAOT] Making thread local storage completely managed. - #84060

Merged
VSadov merged 2 commits into
dotnet:mainfrom
VSadov:mngTls
Mar 30, 2023
Merged

[NativeAOT] Making thread local storage completely managed.#84060
VSadov merged 2 commits into
dotnet:mainfrom
VSadov:mngTls

Conversation

@VSadov

@VSadovVSadov commented Mar 29, 2023

Copy link
Copy Markdown
Member

Basically implementing m_pThreadLocalModuleStatics as a managed object[][] array

  • no need to involve unmanaged allocator and GC handles, thus access to threadstatics has one less indirection
  • managing the threadlocal storage happens entirely in managed code
  • destruction of the entire threadstatics structure is a noop

The threadstatics root is reported together with other thread-owned roots like locals.
Once the owning thread's lifecycle gets to the stage when it stops participating in root reporting, m_pThreadLocalModuleStatics can be reclaimed.

@ghost

Copy link
Copy Markdown

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

Issue Details

Basically implementing m_pThreadLocalModuleStatics as a managed object[][] array

  • no need to involve unmanaged allocator and GC handles, thus access to threadstatics has one less indirection
  • managing the threadlocal storage happens entirely in managed code
  • destruction of the entire threadstatics structure is a noop
Author:VSadov
Assignees:VSadov
Labels:

area-NativeAOT-coreclr

Milestone:-

Comment threadsrc/coreclr/nativeaot/Runtime/thread.h
Comment threadsrc/coreclr/nativeaot/Runtime/gcrhscan.cpp
@VSadovVSadov closed this Mar 29, 2023
@VSadovVSadov reopened this Mar 29, 2023
@VSadov

Copy link
Copy Markdown
MemberAuthor

closed/reopened to unstuck the CLA check

@VSadov

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@VSadov

Copy link
Copy Markdown
MemberAuthor

Assert in ilc (I think unrelated to this change)

 ILC: D:\a\_work\1\s\src\coreclr\jit\lir.cpp:1691
ILC: Assertion failed 'node->IsUnusedValue() && "found an unmarked unused value"' in 'System.Net.Sockets.Tests.ConnectEap+<>c__DisplayClass1_0+<<ConnectAsync_WithData_DataReceived>b__0>d:MoveNext():this' during 'Lowering nodeinfo' (IL size 473; hash 0x4698aa3f; FullOpts)

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@VSadov

VSadov commented Mar 30, 2023

Copy link
Copy Markdown
MemberAuthor

the ilc assert is #84112

@VSadov

Copy link
Copy Markdown
MemberAuthor

Thanks!

@VSadov
VSadov merged commit ef15cfb into dotnet:mainMar 30, 2023
@VSadov
VSadov deleted the mngTls branch March 30, 2023 02:51
@MichalStrehovsky

Copy link
Copy Markdown
Member

the ilc assert is #81356

That issue is closed. Maybe same assert, but definitely a different root cause.

Could you create an issue and exclude the test from building like I did in #83167 and #83250 (PR that added exclusion when I first noticed it).

We should not be merging PRs that have an entire test leg on the floor because of a build failure.

@VSadov

Copy link
Copy Markdown
MemberAuthor

Right. It is a slightly different assert. It needs a recent build but happens without my change.
(a testrun with the commit just prior to mine reproes the failure, but it might have been introduced earlier)

The failure is only in Chk builds, so who does not run "extra-platforms" will not see the failure.

@VSadov

VSadov commented Mar 30, 2023

Copy link
Copy Markdown
MemberAuthor

The assert looked very similar at first glance and I thought it was fixed since my PR hit it. I did not notice that the issue is fairly old.

@ghostghost locked as resolved and limited conversation to collaborators Apr 29, 2023
Sign up for freeto 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.

3 participants

@VSadov@MichalStrehovsky@jkotas