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

use ReentrantLock in LockUtil (Switch) #1092

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pjfanning
Copy link
Contributor

an example change for #1089

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

lgtm

@laglangyue
Copy link
Contributor

lgtm

@pjfanning
Copy link
Contributor Author

I won't merge this yet but I'd appreciate some feedback. With Loom in mind, there doesn't appear to be a big reason not to start changing synchronized blocks to ReentrantLocks. There is a fair amount of documentation online encouraging this change.

@He-Pin
Copy link
Member

He-Pin commented Feb 1, 2024

I think we should only do this where io is involved.

The only issue of the current implementation is it introduce some allocation with the by name parameter.

And a user can keep tracking where the pin happens with -Djdk.tracePinnedThreads=short.

I have changed mostly of my $Work code, to ReentrantLocks :)

@pjfanning
Copy link
Contributor Author

pjfanning commented Feb 1, 2024

I'm open to abandoning this. But if we abandon this, I think that #1089 should be changed to not be a 'good first issue'. It isn't a good first issue if it needs serious expertise to decide on which 'synchronized' blocks need to be changed.

To flip this on it's head, maybe we need to get people to use Pekko with virtual threads and to report to us the pinned thread issues that they hit.

@He-Pin
Copy link
Member

He-Pin commented Feb 1, 2024

I will back my hometown tomorrow, I will try to work on the virtual thread support.

@mdedetrich
Copy link
Contributor

I'm open to abandoning this. But if we abandon this, I think that #1089 should be changed to not be a 'good first issue'. It isn't a good first issue if it needs serious expertise to decide on which 'synchronized' blocks need to be changed.

To flip this on it's head, maybe we need to get people to use Pekko with virtual threads and to report to us the pinned thread issues that they hit.

Completely agree with this, VirtualThread support is not a good first issue not only due to it deceptively easy but also because since its a JDK 21 feature as is well known we are having issues on whats the best way to integrate this into Pekko.

I am still heavily in the "lets just document how JDK 21 users can create their own VirtualThreadPool" camp rather than pushing these kind of changes in.

@pjfanning
Copy link
Contributor Author

pjfanning commented Feb 3, 2024

@mdedetrich this change does not use any new classes - it uses existing classes. synchronized has not been improved in Java 21 but locks like ReentrantLock have been. ReentrantLock does not block the carrier thread and other virtual threads can use the carrier thread while the lock is waiting. synchronized does block the carrier thread and other virtual threads will not get a chance to use the carrier thread during the synchronized blocking is waiting (known as pinning).

See https://softwaremill.com/what-is-blocking-in-loom/#retrofitting-blocking

@mdedetrich
Copy link
Contributor

@mdedetrich this change does not use any new classes - it uses existing classes. synchronized has not been improved in Java 21 but locks like ReentrantLock have been. ReentrantLock does not block the carrier thread and other virtual threads can use the carrier thread while the lock is waiting. synchronized does block the carrier thread and other virtual threads will not get a chance to use the carrier thread during the synchronized blocking is waiting (known as pinning).

See https://softwaremill.com/what-is-blocking-in-loom/#retrofitting-blocking

I wasn't talking about this PR but the other one with VirtualThreadPool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants