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

Move RealCall and RealConnection to loom safe locks #8290

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Mar 17, 2024

Main fix for #8284

replace synchronized with ReentrantLock.withLock, and wait/notify with lock.newCondition.signal/await.

Relies on test results from BasicLoomTest

@yschimke yschimke added the loom Relates to Loom JDK support label Mar 17, 2024
@yschimke yschimke marked this pull request as ready for review March 18, 2024 05:49
Copy link
Member

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

What are the best practices when synchronized is only used to guard state, and never used for a blocking or slow call? From our concurrency rules, a bunch of these might be unnecessary?

### Locks

We have 3 different things that we synchronize on.

#### Http2Connection

This lock guards internal state of each connection. This lock is never held for blocking operations. That means that we acquire the lock, read or write a few fields and release the lock. No I/O and no application-layer callbacks.

#### Http2Stream

This lock guards the internal state of each stream. As above, it is never held for blocking operations. When we need to hold an application thread to block a read, we use wait/notify on this lock. This works because the lock is released while `wait()` is waiting.

#### Http2Writer

Socket writes are guarded by the Http2Writer. Only one stream can write at a time so that messages are not interleaved. Writes are either made by application-layer threads or the do-stuff-later pool.

...

#### Per-Connection Locks

Each connection has its own lock. The connections in the pool are all in a `ConcurrentLinkedQueue`. Due to data races, iterators of this queue may return removed connections. Callers must check the connection's `noNewExchanges` property before using connections from the pool.

The connection lock is never held while doing I/O (even closing a socket) to prevent contention.

A lock-per-connection is used to maximize concurrency.

(We definitely need the Http2Writer part of this)

@swankjesse
Copy link
Member

Maybe to frame it more precisely:

  • what’s the runtime performance difference for locks vs. synchronized on physical threads?
  • what’s the runtime performance difference for locks vs. synchronized on virtual threads?

Maybe we can find benchmarks on this.

@yschimke
Copy link
Collaborator Author

I'll create a tracking bug to benchmark. And review and rescope the locks.

I'll land this as the tests show it's a real problem unless we change the locks.

@swankjesse
Copy link
Member

I don’t think we need to do any benchmarking work. For my own sake I just want an intuition on which locks to use where.

@yschimke yschimke marked this pull request as draft April 14, 2024 21:07
@yschimke yschimke marked this pull request as ready for review April 15, 2024 06:02
@yschimke yschimke merged commit a673f45 into square:master Apr 15, 2024
20 checks passed
swankjesse added a commit that referenced this pull request Apr 17, 2024
yschimke pushed a commit that referenced this pull request Apr 17, 2024
@yschimke
Copy link
Collaborator Author

@swankjesse Are we saying that the locks are not needed for RealCall, RealConnection, Dispatcher? That these should be reworked?

yschimke added a commit to yschimke/okhttp that referenced this pull request Apr 17, 2024
@swankjesse
Copy link
Member

I think there’s two very different ways we use synchronized:

  1. To guard in-memory state. For the accesses we acquire the lock, read or write some fields, and release the lock. There’s unlikely to be any contention for these, and if there is it’s likely to be very short-lived.
  2. To guard I/O. For these accesses we acquire the lock, perform some potentially-slow I/O operation, and release the lock. These ones are likely to have contention.

I think we definitely want to change 2 to Loom-safe locks. I am unconvinced that there’s a benefit for changing 1. But there is a potential memory + performance cost.

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

Successfully merging this pull request may close these issues.

None yet

2 participants