putIfAbsent() regression?

[panghy]

We recently upgraded from 2.7.0 to 2.8.8 and noticed that our putIfAbsent code-paths are contending more heavily than before (at least that's our observation):

   java.lang.Thread.State: BLOCKED (on object monitor)
	at com.github.benmanes.caffeine.cache.BoundedLocalCache.put(BoundedLocalCache.java:2030)
	- waiting to lock <0x00007f00ba087828> (a com.github.benmanes.caffeine.cache.PSWMS)
	at com.github.benmanes.caffeine.cache.BoundedLocalCache.putIfAbsent(BoundedLocalCache.java:1965)

We are aware of the commit 614fe60 that should have improved putIfAbsent but we are somehow seeing the opposite. This is a bounded (1M) synchronous cache with a 1-day expiration.

[ben-manes]

Can you write a simple jmh benchmark to debug against? I want to make sure there is no confusion of how it’s being used. Offhand I’m surprised as well.

[panghy]

Will take a look at that

[ben-manes]

Taking a fresh look and nothing jumps out at me.

The looping around isAlive() check under the lock should be benign because all calls to transition the entry (retired, dead) occur just before the map compute finishes to remove the entry. That means it may have a stale lookup and spin temporarily, but the map should be updated promptly so that the loop doesn't find an entry.

The lock may be held for a long time if there is some long-running user computation occuring, either via a compute / load or writer. If that is not returning promptly then it blocks further updates to that entry. A mistake can be if an atomic operation on the entry performs a write back into the map, which can cause livelocks in some JDK releases. If not that, then maybe adding timeout safeguards. Searching the other stack traces might hint to the cause.

Of course if there is a regression and you can reproduce it, I'll jump right onto it. Does downgrading to 2.7 help?

[panghy]

All line numbers are 2.8.8.

Trying to understand how it can contend on the node at line 2030 (we are also looking at whether 2.7 truly helps with this). Is there a reason if multiple threads hit line 1987 and prior is null that putIfAbsent() on line 2005 returns a non-null value and it cannot also optimize and check for onlyIfAbsent at line 2009 instead of having to synchronize at line 2030 (or am I not reading the code properly?)

[ben-manes]

yeah, that does seem to have been overlooked. I think you're right that the optimization could be added there too.

[panghy]

Per our own analysis, it seems like we are calling putIfAbsent() more in our release that also took on 2.8.8. Perhaps we are seeing this, i.e. contention on prior != null when multiple threads are trying to call putIfAbsent for the same key at once which leads to the synchronization on prior.

[ben-manes]

Let me know how you want to proceed to confirm this. Either downgrade to 2.7 to see if its an issue (as we suspect) or upgrade with a patch. If you are amenable to it, I'd prefer to use a pre-release build to confirm with in case we have to continue digging. I can get a patch in tonight (PST). What do you want to do?

[kentwang929]

Thanks for the fast response. We are currently working on creating the jmh benchmark as well as trying to reproduce the issues consistently on our side. Once we can reproduce it consistently, it would be great if we can move forward with a patch build.

[ben-manes]

I pushed the patch to v2.dev. I think it will be hard to test or benchmark, but a nice catch. I didn't see anything cleaner than copying the 10 lines of code.

This is available on jitpack (via commit hash) or Sonatype's snapshot repository (as 2.9.1-SNAPSHOT).

[panghy]

Thanks, giving it a spin.

[ben-manes]

I forgot to copy into the notifyWriter block as well, if by chance you are using a CacheWriter. If so, you might need to take latest commit.

[panghy]

Just to share here as well but it seems like 2.8.8 is faster already (there is no regression): https://github.com/dbaghdasarya/jmh-benchmark

[ben-manes]

Oh, good. Thanks for doing that. Does the 2.9.1 snapshot help in your case?