Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SBLK_MASK_LOCK_THREADID allow tid up to 65535 #88772

Merged

Conversation

dickens-code
Copy link
Contributor

@dickens-code dickens-code commented Jul 12, 2023

This is an attempt to fix #88426 by avoiding sync block creation.

All test failures are known and is unrelated to the change.

@dotnet/gc

@cshung
Copy link
Member

cshung commented Jul 13, 2023

@jkotas, can you please take a look and see if there will be problems if we increase the number of bits used for thread id?

@jkotas
Copy link
Member

jkotas commented Jul 14, 2023

@jkotas, can you please take a look and see if there will be problems if we increase the number of bits used for thread id?

I do not see a problem with it. cc @VSadov @kouvel

@jkotas
Copy link
Member

jkotas commented Jul 14, 2023

This is an attempt to fix #88426 by avoiding sync block creation.

How many threads does the workload in question have? This change is only going to help if it has more than 1000 threads. If the syncblocks are created for other reasons, it is not going to help.

@VSadov
Copy link
Member

VSadov commented Jul 14, 2023

How many threads does the workload in question have?

Note that this is not exactly about simultaneous number of threads, but about the highest thread ID that a thread can have.

Thread pool often runs number of threads that is more than number of cores. And a high-end machine may have hundreds of logical cores this days, so 1024+ threads seems possible.

But also, there is some latency in thread termination and ID recycling. It may be possible for a few threads after a burst of activity to get IDs higher than 1024 even if you did not have this many threads at the same time, so you may not need a very "high-end" machine, to see this. Once you get threads with an ID >= 1024, they will inflate every thin lock that they use.

65535 may be enough for some time, so this is a reasonable change.

@VSadov
Copy link
Member

VSadov commented Jul 14, 2023

For the syncblocks to become a problem you'd also need to lock on a large number of objects.
It is possible in patterns that use fine-grained locking - like a UI framework may lock on widgets, (WebKit is known for fine-grained locking, but I do not know any examples in .Net). Some graph algorithms could conceivably lock nodes while updating.

There are other reasons for syncblocks, though - like COM interop.
While this change makes sense on its own, it may be worth looking at what causes syncblocks to be created.

@Maoni0
Copy link
Member

Maoni0 commented Jul 14, 2023

the reason why Andrew asked the question is because while this is not a problem on its own, we wanted to make sure we are not robbing other things of using these bits, eg, if this means something else will have 6 fewer bits to use, that's not good. but if when the bits are being used for locking, nothing else will share these 32 bit anyway (ie, those 6 bits wouldn't be used anyway), then it doesn't affect anything else. if anyone has the answer to that question that'd be really helpful. last time I looked at the code for who uses these bits was ages ago so I don't remember the details.

@VSadov
Copy link
Member

VSadov commented Jul 14, 2023

On CoreCLR the sync word has 10 bits that are not used for anything right now.
This change will leave 4 free bits for the future use - like if we find 65535 too small to store thread IDs in a few years, for example.

@VSadov
Copy link
Member

VSadov commented Jul 14, 2023

On CoreCLR the sync word has 10 bits that are not used for anything right now.

Well, that is not entirely correct. That is true when object header is used for locking.
When hashcode is stored in the header, it uses 26 bits and leaves no free ones.

@Maoni0
Copy link
Member

Maoni0 commented Jul 14, 2023

thanks for the info, @VSadov!

we are doing some investigation on our side to see what exactly the culprit of the perf problem was, so please do not merge this for the time being while we are doing that.

@Maoni0 Maoni0 added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 14, 2023
@Maoni0 Maoni0 removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 20, 2023
@Maoni0
Copy link
Member

Maoni0 commented Jul 20, 2023

@cshung and I talked offline and concluded that this is a reasonable fix.

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@VSadov VSadov merged commit b59d9f1 into dotnet:main Jul 20, 2023
107 checks passed
@dickens-code
Copy link
Contributor Author

thanks very much @VSadov @Maoni0 @cshung @jkotas :)

cshung pushed a commit to cshung/runtime that referenced this pull request Jul 22, 2023
* SBLK_MASK_LOCK_THREADID allow tid up to 65535

* copy comment from NativeAOT ObjectHeader.cs

---------

Co-authored-by: dickens <[email protected]>
jeffschwMSFT pushed a commit that referenced this pull request Aug 10, 2023
* SBLK_MASK_LOCK_THREADID allow tid up to 65535

* copy comment from NativeAOT ObjectHeader.cs

---------

Co-authored-by: DIcKeNs <[email protected]>
Co-authored-by: dickens <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1sec Long Pause (stop-the-world) on Gen1 GC
6 participants