NotificationsYou must be signed in to change notification settings
Fork5.2k
Star17.2k

Enable trimming TLS arrays in ArrayPool.Shared#56316

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

stephentoub merged 3 commits intodotnet:mainfromstephentoub:trimmingtls

Jul 31, 2021

Merged

Enable trimming TLS arrays in ArrayPool.Shared#56316

stephentoub merged 3 commits intodotnet:mainfromstephentoub:trimmingtls

Jul 31, 2021

Conversation

Copy link

Member

stephentoub commentedJul 26, 2021

Today arrays stored inArrayPool<T>.Shared's per-core buckets have trimming applied, but arrays stored in the per-thread buckets are only trimmed when there's high memory pressure. This change enables all buffers to be trimmed (eventually). Every time our gen2 callback runs, it ensures any non-timestamped buffers have a timestamp, and also ensures that any timestamped buffers are still timely... if any aren't, they're eligible for trimming. The timestamp is reset for TLS arrays when they're stored, and for per-core buckets when they transition from empty to non-empty; the latter is just a tweak on the current behavior, which incurs the cost of Environment.TickCount upon that transition, whereas now we only pay it as part of the trimming pass.

Contributes to#52098
cc:@jkotas,@Maoni0

ghost added the area-System.Buffers label

Jul 26, 2021

Copy link

ghost commentedJul 26, 2021

Tagging subscribers to this area:@GrabYourPitchforks, @dotnet/area-system-buffers
See info inarea-owners.md if you want to be subscribed.

Issue Details

Contributes to#52098
cc:@jkotas,@Maoni0

Author:	stephentoub
Assignees:	-
Labels:	`area-System.Buffers`
Milestone:	-

stephentoub force-pushed thetrimmingtls branch from58b9768 to8f5ed03Compare

July 27, 2021 21:17

stephentoub added this to the6.0.0 milestone

Jul 27, 2021

stephentoub force-pushed thetrimmingtls branch from8f5ed03 tof9e92b3Compare

July 28, 2021 03:05

jkotas approved these changes

Jul 28, 2021

View reviewed changes

Copy link

Member

jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

src/libraries/System.Private.CoreLib/src/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.cs OutdatedShow resolvedHide resolved

stephentoub force-pushed thetrimmingtls branch fromf9e92b3 to96673dfCompare

July 28, 2021 14:52

steveisok mentioned this pull request

Jul 28, 2021

[Mono] AOT compiler segfaults when building tests#56480

Closed

Copy link

Contributor

vargaz commentedJul 29, 2021

The failures are caused by a linker problem. Here is the original finally clause in ::Trim ():

     finally    {      IL_009a:  ldloc.s    V_5      IL_009c:  brfalse.s  IL_00a5      IL_009e:  ldloc.s    V_5      IL_00a0:  callvirt   instance void System.IDisposable::Dispose()      IL_00a5:  endfinally    }  // end handler

and here is the linked version:

    finally    {      IL_0091:  ldloc.s    V_5      IL_0093:  brfalse.s  IL_009c      IL_0095:  ldloc.s    V_5      IL_0097:  callvirt   instance void System.IDisposable::Dispose()      IL_009c:  endfinally      IL_009d:  ldloc.1      IL_009e:  ldc.i4.1      IL_009f:  bne.un.s   IL_00aa      IL_00a1:  ldc.i4     0x3a98    }  // end handler

Copy link

MemberAuthor

stephentoub commentedJul 29, 2021

The failures are caused by a linker problem

cc:@sbomer,@vitek-karas

Copy link

Member

danmoseley commentedJul 29, 2021

@antonfirsov

antonfirsov reviewed

Jul 29, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.cs OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

stephentoub commentedJul 30, 2021

@steveisok, how can we unblock this PR?

stephentoub force-pushed thetrimmingtls branch from96673df to9d5e3baCompare

July 30, 2021 15:09

stephentoub mentioned this pull request

Jul 30, 2021

linker producing bad code in finally block that causes AOT compiler to seg faultdotnet/linker#2181

Closed

Copy link

Member

danmoseley commentedJul 30, 2021

@stephentoub failures in runtime-staging configurations do not block merge. Although, I thought they should not show up as red for that reason.

Copy link

Member

steveisok commentedJul 30, 2021•
edited
Loading

@stephentoub failures in runtime-staging configurations do not block merge. Although, I thought they should not show up as red for that reason.

That's true for test failures only. If you have a build failure, then it'll show up red.

Copy link

Member

danmoseley commentedJul 30, 2021

(I mean, they don't block merge if the failures aren't clearly related. If this change breaks them more, we probably don't want to merge it.)

Copy link

Member

danmoseley commentedJul 30, 2021

That makes sense.

Copy link

MemberAuthor

stephentoub commentedJul 30, 2021

I mean, they don't block merge if the failures aren't clearly related. If this change breaks them more, we probably don't want to merge it.

Right. The problem is that the correct C# code and generated IL introduced by this PR is then getting mutilated by the linker and causing the AOT compiler to seg fault. As a temporary workaround to unblock this PR, I'd be happy to tweak the C# code to avoid whatever pattern is tripping up the linker, if someone can suggest what that pattern is. But it seems to me there's both a bug in the linker (dotnet/linker#2181) and a bug in the AOT compiler (seg faulting in response to bad IL) to be fixed here.

Copy link

Member

jkotas commentedJul 30, 2021

if someone can suggest what that pattern is.

My guess is that ifdefing the logging specific paths in Trim method for Mono is going to workaround the linker bug:

#if !MONO // Workaround for http://github.com/...     if (log.IsEnabled())     {     ....     }     else#endif

Copy link

MemberAuthor

stephentoub commentedJul 30, 2021

Ok, thanks, I'll give that a go.

stephentoub added3 commits

July 31, 2021 06:45

Enable trimming TLS arrays in ArrayPool.Shared

9dfc854

Today arrays stored in `ArrayPool<T>.Shared`'s per-core buckets have trimming applied, but arrays stored in the per-thread buckets are only trimmed when there's high memory pressure.  This change enables all buffers to be trimmed (eventually).  Every time our gen2 callback runs, it ensures any non-timestamped buffers have a timestamp, and also ensures that any timestamped buffers are still timely... if any aren't, they're eligible for trimming.  The timestamp is reset for TLS arrays when they're stored, and for per-core buckets when they transition from empty to non-empty; the latter is just a tweak on the current behavior, which incurs the cost of Environment.TickCount upon that transition, whereas now we only pay it as part of the trimming pass.

Address PR feedback

de909a1

Work around bad linker transform

4ea83d2

stephentoub force-pushed thetrimmingtls branch from9d5e3ba to4ea83d2Compare

July 31, 2021 10:53

stephentoub merged commit7d9a08c intodotnet:main

Jul 31, 2021

stephentoub deleted the trimmingtls branch

July 31, 2021 16:13

Copy link

Member

DrewScoggins commentedAug 5, 2021

We are seeing some wins in the ArrayPool perf tests as a result of this change.

Copy link

MemberAuthor

stephentoub commentedAug 5, 2021

Thanks,@DrewScoggins. It wasn't the initial intention of the PR, but I'm glad it had this affect (I expect from removing the TickCount check every time we transitioned from 0 to 1 array stored in the per-core stack). Do you see any movement here that might counteract what you reported in#56555?