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

Suppliers.memoize() thread pinning #7140

Open
3 tasks done
steven-sheehy opened this issue Apr 8, 2024 · 6 comments
Open
3 tasks done

Suppliers.memoize() thread pinning #7140

steven-sheehy opened this issue Apr 8, 2024 · 6 comments

Comments

@steven-sheehy
Copy link

steven-sheehy commented Apr 8, 2024

API(s)

Suppliers.memoize()

How do you want it to be improved?

Change it from synchronized to ReentrantLock

Why do we need it to be improved?

Suppliers.memoize() internally uses the double-checked locking idiom with the synchronized keyword. Using synchronized with virtual threads can cause thread pinning and slow down performance.

Example

Suppliers.memoize(() -> someLongRunningTask());

Current Behavior

Functions properly but causes performance degradation when used with virtual threads on Java 21

Desired Behavior

Performs optimally with virtual threads.

Concrete Use Cases

In our virtual thread enabled REST API, added lazy loading using Suppliers.memoize() around some of our database interactions to improve performance but ended up seeing worse performance than going directly to database.

Checklist

@steven-sheehy steven-sheehy added the type=enhancement Make an existing feature better label Apr 8, 2024
@chaoren chaoren added package=base status=triaged P3 type=performance Related to performance P2 and removed type=enhancement Make an existing feature better P3 labels Apr 9, 2024
@chaoren chaoren self-assigned this Apr 9, 2024
@ben-manes
Copy link
Contributor

This is already addressed by the JDK team’s EA build. That’ll avoid surprises as a better long term solution.

@steven-sheehy
Copy link
Author

That's cool to see and would definitely help long term, but unfortunately my company only uses LTS versions of Java so that'd mean we'd have to wait until Java 25 (or whatever it is) to be released.

@ben-manes
Copy link
Contributor

If you read the thread, you’ll see that VTs still have serious problems and must be used very carefully for now. You might not want to use them until 25 when hopefully they’ll be less error prone. Libraries trying to support them early might not really help; imho it’s best to wait.

the summary here is that addressing the issue with object
monitors pinning is great but the hoorays may be short lived as the spot
light moves to other cases where carriers are pinned, and specifically
native frames due to resolving references to classes in the constant
pool and the resulting class loading, or class initializers. There are
some ideas around this that may provide some relief on these cases. We
had to shake out issues with object monitors first.

@chaoren chaoren added P3 and removed P2 labels Apr 9, 2024
@chaoren
Copy link
Member

chaoren commented Apr 9, 2024

Thanks for the info @ben-manes. Given that information, would you say that it's better to keep the current implementation as-is?

@cpovirk
Copy link
Member

cpovirk commented Apr 9, 2024

+1, thanks.

One other note: We did a little work to improve the memory usage of Suppliers.memoize recently. I would suspect that a switch to ReentrantLock would more than undo the gains from that. And the reason that we did the optimization is that it appeared to matter at least a little to our fleet in aggregate, so I'd expect a switch to ReentrantLock hurt in a small but measurable way. [edit: We have subsequently made changes that offset those gains, but using ReentrantLock would increase size even further.]

There is probably a way to write our own even more efficient implementation of Suppliers.memoize, which would use LockSupport.park and friends directly. (I'd played around with something similar for Dagger in my internal cl/209143332, as has another developer in cl/448147583.) That could not only be more compact but also avoid making waiters take turns in receiving the produced value. It hasn't been clearly worth the effort, but it's possible that virtual threads will change the calculus.

Whatever we decide for now, it's possible that we'll revisit as we get more experience with virtual threads. (If we encounter the level of problems that Ben Manes passes along, then we may not get that experience anytime soon :))

@chaoren chaoren removed their assignment Apr 9, 2024
@cpovirk
Copy link
Member

cpovirk commented Apr 9, 2024

Oh, also: As the Java platform develops more broadly, we may also see alternatives to Suppliers.memoize become available there, reducing the need for Guava's version. See, for example, https://openjdk.org/jeps/8312611 or even https://openjdk.org/jeps/8209964.

cpovirk referenced this issue Aug 6, 2024
This let us move away from the current hack (which was never in a Guava _release_) of using a lock of `new Integer(1)` (because we needed something `Serializable` while we figured this all out).

RELNOTES=n/a
PiperOrigin-RevId: 659714359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants