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

Remove some uses of synchronized #3024

Merged

Conversation

bryce-anderson
Copy link
Contributor

Motivation:

With java-21 and v-threads synchronization can cause deadlocks because carrier threads cannot release a v-thread that is in a synchronized block.

Modifications:

Identify the cases where we're using synchronization and move them to the equivalent v-thread safe ReentrantLock.

Result:

Better v-thread safety.

Motivation:

With java-21 and v-threads synchronization can cause deadlocks
because carrier threads cannot release a v-thread that is in a
synchronized block.

Modifications:

Identify the cases where we're using synchronization and move
them to the equivalent v-thread safe ReentrantLock.

Result:

Better v-thread safety.
@@ -67,7 +68,7 @@ final class GradientCapacityLimiter implements CapacityLimiter {

private static final Logger LOGGER = LoggerFactory.getLogger(GradientCapacityLimiter.class);

private final Object lock = new Object();
private final ReentrantLock lock = new ReentrantLock();
Copy link
Member

Choose a reason for hiding this comment

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

AimdCapacityLimiter uses stateUpdater.compareAndSet(this, UNLOCKED, LOCKED). While I don't have any personal preference towards one of the approaches (not sure if it's worth a banchmark), it would be great to make both implementations consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer not to introduce behavior changes in this PR and moving from a blocking lock to a spin-lock feels non-trivial.

Tangentially, I suspect spin-locks are not going to play well with v-threads either since that will likely cause a v-thread to not be able to be de-schedule since it's not 'blocked' by anything, resulting in a similar deadlock situation.

Copy link
Member

Choose a reason for hiding this comment

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

agreed on lets keep the same synchronization approach in this PR, and can consider changes/improvements in approach as followup.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the follow-up. With the above argument about v-threads, seems reasonable to use ReentrantLock in Aimd as well. IIRC our discussion with @tkountis, there were no other motivations for that spin-lock except just an attempt to avoid synchronized.

Copy link
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

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

any reason why CacheConnectionFactory isn't tackled in this PR?

@@ -67,7 +68,7 @@ final class GradientCapacityLimiter implements CapacityLimiter {

private static final Logger LOGGER = LoggerFactory.getLogger(GradientCapacityLimiter.class);

private final Object lock = new Object();
private final ReentrantLock lock = new ReentrantLock();
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the follow-up. With the above argument about v-threads, seems reasonable to use ReentrantLock in Aimd as well. IIRC our discussion with @tkountis, there were no other motivations for that spin-lock except just an attempt to avoid synchronized.

@bryce-anderson bryce-anderson merged commit 1c2afec into apple:main Jul 30, 2024
11 checks passed
@bryce-anderson bryce-anderson deleted the bl_anderson/rm_some_synchronized branch July 30, 2024 20:39
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.

3 participants