Enable trimming TLS arrays in ArrayPool.Shared

[stephentoub]

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.

Contributes to #52098
cc: @jkotas, @Maoni0

[ghost]

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

Issue Details

---

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.

Contributes to #52098
cc: @jkotas, @Maoni0

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

[jkotas]

Thank you!

[vargaz]

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

[stephentoub]

The failures are caused by a linker problem

cc: @sbomer, @vitek-karas

[danmoseley]

@antonfirsov

[stephentoub]

@steveisok, how can we unblock this PR?

[danmoseley]

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

[steveisok]

@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.

[danmoseley]

(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.)

[danmoseley]

That makes sense.

[stephentoub]

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.

[jkotas]

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

[stephentoub]

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

[DrewScoggins]

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

[stephentoub]

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?

[DrewScoggins]

Yes, we saw some of it come back, but not all.