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 synchronized to guard state, ReentrantReadWriteLock for blocking calls #8581

Open
swankjesse opened this issue Nov 19, 2024 · 2 comments
Labels
bug Bug in existing code

Comments

@swankjesse
Copy link
Collaborator

swankjesse commented Nov 19, 2024

This is what I wrote in #8290

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

  • 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.
  • 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.

I’d like to find all of the things we converted to ReentrantReadWriteLock to guard in-memory state, and change ’em back to use synchronized.

This link from @yschimke provides more discussion!

@swankjesse swankjesse added the bug Bug in existing code label Nov 19, 2024
@yschimke
Copy link
Collaborator

Let's make sure we have good coverage of Loom tests when we put this in. I think I landed a test that fails if we block loom.

@swankjesse
Copy link
Collaborator Author

Oooh good call.

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

No branches or pull requests

2 participants