Skip to content

SBLK_MASK_LOCK_THREADID allow tid up to 65535 (#88772) - #89335

Merged
jeffschwMSFT merged 1 commit into
dotnet:release/7.0-stagingfrom
cshung:backport/pr-88772-to-release/7.0-staging
Aug 10, 2023
Merged

SBLK_MASK_LOCK_THREADID allow tid up to 65535 (#88772)#89335
jeffschwMSFT merged 1 commit into
dotnet:release/7.0-stagingfrom
cshung:backport/pr-88772-to-release/7.0-staging

Conversation

@cshung

@cshungcshung commented Jul 22, 2023

Copy link
Copy Markdown
Contributor

Customer Impact - from @dickens-code

Our apps require low latency performance and we are trying hard to reduce memory allocation in order to minimise GC occurrences.

We noticed that one of our apps experiences a very long GC pause of 2 seconds, and with the help of @cshung, we discovered that our app is having very large sync block memory space (20 million sync blocks), which introduced a very long time of sync block scanning during GC events.

We found out that new sync blocks are introduced when thread ID > 1024 is taking object locks and we tried to release the limit of thread ID to 65535 and the sync block memory space size was obviously improved

With the custom-built coreclr.dll we observed that our app's GC pauses were brought down to around 100ms which makes more sense according to our business requirements

Testing

  • GC Perf Sim
    • Normal Server
    • Normal Workstation
    • Large Pages Server
    • Large Pages Workstation
    • Low Memory Container
    • High Memory Load
  • Microbenchmarks
    • Workstation
    • Server
  • ASPNet Benchmarks
    • JsonMin_Windows
    • Fortunes ETF_Windows
    • Stage1Grpc_Windows

Test failures are known - on my machine, I don't have sufficient memory to pass the 20GB large pages test case, all other large page test cases are passing.

Risk

Low - the bits were not used for anything before this fix, and the change is merged to main for 2 weeks with no regression found.

* SBLK_MASK_LOCK_THREADID allow tid up to 65535
* copy comment from NativeAOT ObjectHeader.cs
---------
Co-authored-by: dickens <dickens.tam@pulsartradingcap.com>
@ghostghost assigned cshungJul 22, 2023
@ghostghost added the area-VM-coreclr label Jul 22, 2023
@teo-tsirpanisteo-tsirpanis added this to the 7.0.x milestone Jul 23, 2023
@carlossanlop

Copy link
Copy Markdown
Contributor

Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th.

@cshung
cshung requested a review from Maoni0August 3, 2023 03:43
@jeffschwMSFTjeffschwMSFT added the Servicing-consider Issue for next servicing release review label Aug 9, 2023

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

approved. we will take for consideration in 7.0.x

@jeffschwMSFTjeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 10, 2023
@jeffschwMSFTjeffschwMSFT modified the milestones: 7.0.x, 7.0.11Aug 10, 2023
@jeffschwMSFT
jeffschwMSFT merged commit 078e669 into dotnet:release/7.0-stagingAug 10, 2023
@cshung
cshung deleted the backport/pr-88772-to-release/7.0-staging branch August 10, 2023 20:50
@ghostghost locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-coreclrServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@cshung@carlossanlop@jeffschwMSFT@Maoni0@teo-tsirpanis@dickens-code