expireAfter(Expiry) does not reliably invoke removalListener

[facboy]

expireAfter(Expiry) does not reliably invoke the removalListener when used with expireAfterUpdate. test-case here: https://github.com/facboy/caffeine-test

as far as I can tell it happens when the Expiry only sets an expiry on expireAfterUpdate. the Node ends up in the TimerWheel with its original MAX_EXPIRY and this is never updated so getExpirationDelay() always returns some value years in the future, and the expiry task is thus also scheduled for years in the future.

[ben-manes]

Thanks for the test case! I think you're right, there is a mistake here when adding variable expiration that didn't fit well with the write tolerance handling. The logic here seems incorrect,

caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java

Lines 2075 to 2085 in a54afaa

	int weightedDifference = mayUpdate ? (newWeight - oldWeight) : 0;
	if ((oldValue == null) || (weightedDifference != 0) || expired) {
	afterWrite(new UpdateTask(prior, weightedDifference));
	} else if (!onlyIfAbsent && expiresAfterWrite() && exceedsTolerance) {
	afterWrite(new UpdateTask(prior, weightedDifference));
	} else {
	if (mayUpdate) {
	setWriteTime(prior, now);
	}
	afterRead(prior, now, /* recordHit */ false);
	}

This causes the expireAfter to degrade into the afterRead case, which swallows the reordering and delays handling. A few minor changes to this if-block locally resolves the problem, but I need to review it more thoroughly before pushing and releasing for you. Hopefully will get to it tonight PST.

[ben-manes]

Sorry, this is a very busy week for me. In the meantime you should be able to use asMap.compute as a workaround. I'll try to have a fix released by next Monday.

[ben-manes]

I believe the right fix is to update the calculation to be aware of variable expiration.

caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java

Line 2054 in a54afaa

exceedsTolerance = ((now - prior.getWriteTime()) > EXPIRE_WRITE_TOLERANCE);

should be replaced with

exceedsTolerance = ((now - prior.getWriteTime()) > EXPIRE_WRITE_TOLERANCE)
    | (expiresVariable()
        && Math.abs(varTime - prior.getVariableTime()) > EXPIRE_WRITE_TOLERANCE);

This passes that failure of your test case, though it fails for what I think is a logic bug in the test itself.

In a JMH write benchmark the optimization still proves itself valuable when write-time expiration is enabled. For Caffeine this avoids overwhelming the write buffer if the same entries are constantly being updated, which causes writes to be delayed due to backpressure in order to let the maintenance handling catch-up. The 1s tolerance helps avoid that scenario.

Expirable Cache	Score
Guava	5.2M/s
Caffeine w/o opt	5.9M/s
Caffeine with opt	23M/s

I need to understand why your test still fails and write my own for the test suite.

[facboy]

is there a reason you don't shortcut the check? (| vs ||)

doesn't this check need to be changed too?

caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java

Line 2078 in a54afaa

} else if (!onlyIfAbsent && expiresAfterWrite() && exceedsTolerance) {

[facboy]

#479

this fixes it for me, there was a logic error with the mocks in my test case so it failed on the second iteration. i think strictly speaking the iterations are not necessary. in my production code where i found this error it doesn't fail consistently, and i had to run it a few times to reliably cause a failure.

[ben-manes]

Thanks for the clarification. I thought that was the cause of the test failure, but had a long day so felt I might need a fresh look.

is there a reason you don't shortcut the check? (| vs ||)

In this case it should be safe and reduces branching, based on one of Josh Bloch's last major talks about best practices. It is not really a visible performance win, but something to remind oneself that little costs add up to for hot methods (e.g. like avoiding the slow modulus operator by using an & mask).

doesn't this check need to be changed too?

Good catch, thanks!

this fixes it for me

Thanks for the PR. I would like to add some test cases before the merge & release.

[ben-manes]

Released 2.8.7