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.

Sign up for GitHub

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

Jump to bottom

Don't cache elements for 1 second, makes no sense #11516

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Don't cache elements for 1 second, makes no sense #11516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d56dc16
Don't cache elements for 1 second, makes no sense
mkurz Oct 26, 2022
8c2df35
Comments
mkurz Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@ import play.cache.{ SyncCacheApi => JavaSyncCacheApi }

import scala.jdk.FutureConverters._
import scala.concurrent.duration.Duration
import scala.concurrent.duration._
import scala.concurrent.ExecutionContext
import scala.concurrent.Future
import scala.reflect.ClassTag
Expand Down Expand Up @@ -183,7 +184,10 @@ class SyncCaffeineCacheApi @Inject() (val cache: NamedCaffeineCache[Any, Any]) e
private val syncCache: Cache[Any, Any] = cache.synchronous()

override def set(key: String, value: Any, expiration: Duration): Unit = {
syncCache.put(key, ExpirableCacheValue(value, Some(expiration)))
if (!expiration.isFinite || !expiration.lteq(0.seconds)) {
// Cache only if expiration is greater than 0 (0 and below won't cache)
syncCache.put(key, ExpirableCacheValue(value, Some(expiration)))
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This if condition was done just to be on par with the ehcache implementation. Actually belows change in calculateExpirationTime from 1 to 0 should already be enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mkurz I didn't understand.
For me, if some uses the set(Key: String, value: Any, expiration: Duration) method it won't be saved into syncCache. So, it seems that this method wouldn't work...

IMHO doesn't matter if your pass 0+ seconds, must work.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does work for 1 and more seconds, but an element will not be cached if 0 seconds or lower.

The condition means

if (expiration is infinite/forever OR expiration is greater than 0 seconds)
  then cache it
end if

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a negation of the if below case Some(duration) if duration.isFinite && duration.lteq(0.second) =>...

Copy link
Contributor

Choose a reason for hiding this comment

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

So, what I've said seems to be true.
If someone uses 0 seconds it wouldn't be saved, right?

Why wouldn't we set it into the cache as a value without expiration when is defined as 0 secs?

Copy link
Member Author

@mkurz mkurz Oct 26, 2022 •
edited

Choose a reason for hiding this comment

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

If someone uses 0 seconds it wouldn't be saved, right?

Yes, that is the current behavour in Play 2.8. as well. OK, not really, currently, if you pass 0 that means cache it just for 1 second.... this is what I am changing right now because it does not make sense in my opinion...

Maybe this code explains the current behavour best:

playframework/cache/play-ehcache/src/main/scala/play/api/cache/ehcache/EhCacheApi.scala

Lines 190 to 204 in ccf99ba

val element = new Element(key, value)
expiration match {
case infinite: Duration.Infinite => element.setEternal(true)
case finite: FiniteDuration =>
val seconds = finite.toSeconds
if (seconds <= 0) {
element.setTimeToLive(1)
} else if (seconds > Int.MaxValue) {
element.setTimeToLive(Int.MaxValue)
} else {
element.setTimeToLive(seconds.toInt)
}
}
cache.put(element)
Done

Why wouldn't we set it into the cache as a value without expiration when is defined as 0 secs?

Because someone, long long time ago, decided that when passing 0 as expiration it should mean "do not cache" (or like: cache for 1 second) in Play. Be aware, that different libraries use different approaches. So for ehcache if you pass 0 that means cache it forever, however, in caffeine if you return 0 from expireAfterCreate it means do not cache it at all (see this coment "...How well could expireAfter(expiry) fit your use-case? That returns a duration, which may be zero to indicate immediate expiration...")
So different libraries use different approaches. And Play choose to say 0 expiration = no caching. I didn't decided that.
So my changes here do not change anything actually, I just do not cache it for 1 second but do not cache it at all anymore because it's senseless.

Copy link
Member Author

Choose a reason for hiding this comment

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

@felipebonezi Please let me know if you don't understand what I am saying. In my opinion my code does not really change the behaviour of the cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, I do understand now.
Thanks for being patient with me, hehehehe. 🤣

Done
}

Expand Down
7 changes: 4 additions & 3 deletions cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/DefaultCaffeineExpiry.scala
View file
Open in desktop
Expand Up @@ -31,9 +31,10 @@ private[caffeine] class DefaultCaffeineExpiry extends Expiry[String, ExpirableCa

private def calculateExpirationTime(durationMaybe: Option[Duration]): Long = {
durationMaybe match {
case Some(duration) if duration.isFinite && duration.lteq(0.second) => 1.second.toNanos
Copy link
Member Author

@mkurz mkurz Oct 26, 2022 •
edited

Choose a reason for hiding this comment

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

The 1 second for caffeine was introduced in #9615 - there is no comment why this was done, so my guess would be the author just copied the ehcache behaviour... Update: comment was added.

Copy link
Member Author

Choose a reason for hiding this comment

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

See ben-manes/caffeine#803

Copy link
Member Author

Choose a reason for hiding this comment

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

So in ben-manes/caffeine#803 it turns out I was right and returning 0 will therefore always end up in a cache miss for the next read from the cache from the same key, so it's like not caching at all.

case Some(duration) if duration.isFinite => duration.toNanos
case _ => Long.MaxValue
case Some(duration) if duration.isFinite && duration.lteq(0.second) =>
0.seconds.toNanos // Will always end up in a cache miss, so this equals to not caching at all: https://github.com/ben-manes/caffeine/discussions/803
case Some(duration) if duration.isFinite => duration.toNanos
case _ => Long.MaxValue
}
}
}
9 changes: 7 additions & 2 deletions cache/play-ehcache/src/main/scala/play/api/cache/ehcache/EhCacheApi.scala
View file
Open in desktop
Expand Up @@ -188,19 +188,24 @@ private[play] case class EhCacheExistsException(msg: String, cause: Throwable) e
class SyncEhCacheApi @Inject() (private[ehcache] val cache: Ehcache) extends SyncCacheApi {
override def set(key: String, value: Any, expiration: Duration): Unit = {
val element = new Element(key, value)
var doCache = true
expiration match {
case infinite: Duration.Infinite => element.setEternal(true)
case finite: FiniteDuration =>
val seconds = finite.toSeconds
if (seconds <= 0) {
element.setTimeToLive(1)
Copy link
Member Author

@mkurz mkurz Oct 26, 2022 •
edited

Choose a reason for hiding this comment

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

First, Play's ehcache implementation introduced that behaviour first, see https://github.com/playframework/playframework/pull/5923/files#r1005645691

Copy link
Member

@gmethvin gmethvin Oct 26, 2022 •
edited by mkurz

Choose a reason for hiding this comment

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

As I mentioned in my reply on that PR, we should consider cases where the duration is a positive value less than 1 second. If the duration is actually zero or less I agree it's reasonable behavior not to cache at all.

// We don't even put the element in the cache, why should we?
// Obviously someone wants to put something in the cache for 0 (or less) seconds...
doCache = false
} else if (seconds > Int.MaxValue) {
element.setTimeToLive(Int.MaxValue)
} else {
element.setTimeToLive(seconds.toInt)
}
}
cache.put(element)
if (doCache) {
Copy link
Member

Choose a reason for hiding this comment

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

If there is an existing value in the cache, I think you want to remove the element from the cache rather than just doing nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that is something to consider, thanks!

cache.put(element)
}
Done
}

Expand Down