Added Caffeine Cache module and made it the default Cache #8025

dusanstanojeviccs · 2017-11-21T07:52:41Z

Pull Request Checklist

Have you read How to write the perfect pull request?
Have you read through the contributor guidelines?
Have you signed the Lightbend CLA?
Have you referenced any issues you're fixing using commit message keywords?
Have you added copyright headers to new files?
Have you checked that both Scala and Java APIs are updated?
Have you updated the documentation for both Scala and Java sections?
Have you added tests for any changed functionality?

Helpful things

Fixes

Fixes #7974

Purpose

This PR is adding Caffeine Cache module to Play and setting Caffeine Cache as the default cache provider.

Background Context

I've mostly based this solution on previous Ehcache implementation.

References

#7974

marcospereira

Thank you so much for taking care of this, @dusanstanojeviccs. Some observations:

We need to provide a clear migration path to users, so this change needs to be explained in https://github.com/playframework/playframework/blob/master/documentation/manual/releases/release27/migration27/Migration27.md. Ideally, we should tell users how to use EhCache if they don't want (or can't) migrate to Caffeine.
It could be good to have a small page showing how to use EhCache if users want to use it instead.

ben-manes · 2017-11-21T20:10:08Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md


 ## JCache Support

-Ehcache implements the [JSR 107](https://github.com/jsr107/jsr107spec) specification, also known as JCache, but Play does not bind `javax.caching.CacheManager` by default.  To bind `javax.caching.CacheManager` to the default provider, add the following to your dependencies list:
+Caffeine Cache implements the [JSR 107](https://github.com/jsr107/jsr107spec) specification, also known as JCache, but Play does not bind `javax.caching.CacheManager` by default.  To bind `javax.caching.CacheManager` to the default provider, add the following to your dependencies list:


Ehcache is a native JCache provider, since they were the main authors of the spec.

Caffeine isn't, since I consider the spec to be pretty bad. Instead it provides an optional implementation as an additional dependency. So you may need to clarify the additional caffeine-jcache dependency is required, too. Or you could consider it self-evident since jcache configuration is mostly non-standard.

Got it, changing the docs now will make a commit with more information and a reference to caffeine-jcache dependency.

ben-manes · 2017-11-21T20:17:10Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md

@@ -55,7 +57,7 @@ You can also supply a `Callable` that generates stores the value if no value is

 @[get-or-else](code/javaguide/cache/JavaCache.java)

-**Note**: `getOrElseUpdate` is not an atomic operation in Ehcache and is implemented as a `get` followed by computing the value from the `Callable`, then a `set`. This means it's possible for the value to be computed multiple times if multiple threads are calling `getOrElse` simultaneously.
+**Note**: `getOrElseUpdate` is not an atomic operation in Caffeine Cache and is implemented as a `get` followed by computing the value from the `Callable`, then a `set`. This means it's possible for the value to be computed multiple times if multiple threads are calling `getOrElse` simultaneously.


This is an unfortunate aspect of the Play! api. Caffeine does offer atomic operations, but not with a custom expiration passed through. Instead the Expiry is used to evaluate, since often the calls should prefer to use a LoadingCache rather than a cache.get(key, func). This is the same problem with Ehcache and others, where that Play! api doesn't map neatly.

@gmethvin Is the lack of memoization a negative for Play! users? If so, you might want to implement this generically using lock striping for all providers to take advantage of. That's merely Lock lock = locks[hash(key) mod locks.length], etc. There some minor optimization tricks, but would note.

If I understood that correctly get and set are not atomic if expiration is set and we should do locking at the integration level?

Yes, in that we don't provide a computeIfAbsent(key, func, expiration) type of call like Play! does. Neither does Ehcache, so both are using get-compute-put. There are atomic computeIfAbsent(key, func) type methods, but they call back into the Expiry to get the information. You would have to do some nasty thread-local to hack it where the TL is set, the Expiry queries it, and the TL is unset. I doubt that's really desirable.

Oh got it, so it would stay the same, right? Before there was just a note in the docs that the method was doing get-compute-put and everyone was ok with it, so I guess it would be ok if that stayed the same since Play! users would expect it.

Yes, its just a quirk of Play's APIs rather than a missing feature. That API method does not map neatly to any caching library, and isn't worth providing ad hoc just for Play when it is fairly ugly (when compared to the preferred). Since this is an issue across all providers, the best fix might be to add the feature into Play's abstraction. Using lock striping is cheap and simple code, so a suggestion for @gmethvin to consider someday.

Yes, this is an unfortunate design quirk of the Play cache. There has also been discussion of allowing a configurable default for the methods when an explicit expiration is not passed. That would probably make more sense than defaulting to infinite.

@dusanstanojeviccs I think for the purposes of this PR, the goal should be parity with the current ehcache-based implementation, so this disclaimer would apply to both caffeine and ehcache. If you have interest in implementing lock striping to solve the problem as @ben-manes described, that would be a nice addition, but it could also be done in a future PR.

Yes, I am only putting this on @gmethvin radar and not asking for it in this PR. Ignore this distraction.

I can probably do this and I like the locks[hash(key) mod locks.length], but what should the size of locks be, should it be scaled up and down depending on the number of keys currently in the cache (like an arraylist) in sync(locks) blocks or should it just have a constant size set up front in the config file?

It would be in a subsequent PR.

Generally you size ad hoc, since any decent number will offer enough concurrency. If you try to be fancy, you can base it on Runtime.getRuntime().availableProcessors() but it probably won't help. A setting like 2048 is plentiful and perhaps larger than necessary. Generally you want to minimize collisions but since writes are rarer, the likelihood of overlap isn't high. You would use a power-of-two in order to use the cheaper modulus trick of N & (N - 1). There always reaches a point where increasing the number of stripes stops being beneficial and a redesign is required instead.

The stripe can be an immutable Object[] array with unique instances. Then one would use double-checked locking to avoid synchronizing in the happy-path of the entry being present. This way calls for the same key will collide and be blocked, with a high probability for distinct keys compute concurrently.

V value = cache.getIfPresent(key); if (value != null) { return value; } Object lock = locks[hash(key) & MASK]; synchronized (lock) { value = cache.getIfPresent(key); if (value == null) { value = // compute cache.put(key, value, expiration); } return value; }

A supplementary hash function, like murmur3 or wang-jenkins, would help the distribution if key.hashCode() was of poor quality (which I think String is). For example the spreader in the original ConcurrentHashMap (based on fixed stripes vs dynamic tree-bins in JDK8) was

private static int hash(int h) { // Spread bits to regularize both segment and index locations, // using variant of single-word Wang/Jenkins hash. h += (h << 15) ^ 0xffffcd7d; h ^= (h >>> 10); h += (h << 3); h ^= (h >>> 6); h += (h << 2) + (h << 14); return h ^ (h >>> 16); }

I would imagine a striped CacheApi would be a trait or decorator for reuse - whatever is idiomatic these days. The underlying cache would assumed to be concurrent, so only set calls would need to use the lock to compute with.

@ben-manes Sorry I have not responded sooner, I was traveling and then had to catch up on work. Wow! That is beautiful and makes total sense, the whole N & (N - 1) is brilliant! I will definitely do this in a future PR. Thank you for the pointers.

ben-manes · 2017-11-21T20:17:43Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md


-    play.cache.createBoundCaches = false
+    play.cache.caffeine.spec = {}


I think you changed from the spec string to the parser?

I did, but I named the configuration value 'play.cache.caffeine.spec'. Would it be clearer for Caffeine users if I changed it to 'play.cache.caffeine.parser'?

Oh, nevermind then. I thought this was the unparsed spec string. It might be good to see an example of how a named cache works (just here for me to grok it). I'd have thought it would be play.cache.caffeine.<name>.

I've looked at the current ehcache.xml example and it looked like it was never bound to a cache name so I figured that was not part of the spec before. But to be honest that makes a lot of sense. Doing it now.

Done, running tests now.

Why not drop the spec?

play.cache.caffeine { play { maximum-size: 200 } custom { ... } }

The Caffeine JCache adapter uses this type of configuration. but I don't know if it is considered idiomatic. Hopefully the core devs here have an opinion.

No good reason to keep the spec so I dropped it. I have not seen any modules that do this so I am not sure if it is idiomatic either. I would imagine it would be fine as long as it got documented in the official docs but let's hold off on it until we get an opinion from one of the core devs. :)

How about something like:

play.cache.caffeine { defaults { maximumSize = 200 ... } caches { play = {} custom-cache = {} } }

This is similar to what we do on database configuration: https://github.com/playframework/playframework/blob/master/framework/src/play-jdbc/src/main/resources/reference.conf#L28.

We can also include the reference.conf directly in the page like we do on other pages, e.g. https://github.com/playframework/playframework/blob/2.6.x/documentation/manual/working/commonGuide/configuration/SettingsAkkaHttp.md

@gmethvin Looks good, I've added the code for this in my last commit. So to be clear defaults is used if "play.cache.caffeine.caches" does not contain the cache name as a child.

The configuration for each cache would be layered on top of the default. The presence of the play key would indicate that a cache called play should be created, and the values in that object would override the default values with the same name.

ben-manes · 2017-11-21T20:24:42Z

framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineParser.java

+    private void parse(String key) {
+        switch (key) {
+            case "initial-capacity":
+                System.out.println("initial-capacity:" + config.getInt(key));


Does Play! not have a logger?

It does, I've put this for checking if everything works, I just personally like using System.out because I see no advantage of logger when testing... I've removed it now.

ben-manes · 2017-11-21T20:25:22Z

framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineParser.java

+                System.out.println("maximum-weight:" + config.getMemorySize(key));
+                cacheBuilder.maximumWeight(config.getMemorySize(key).toBytes());
+                break;
+            case "expire-after-access":


Since variable expiration is required for the play API, these fixed versions cannot be used simultaneously. So you should remove.

Got it, I've removed: expire-after-access, expire-after-write, refreshAfterWrite.

ben-manes · 2017-11-21T20:27:49Z

framework/src/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

+        val seconds = finite.toSeconds
+        if (seconds <= 0) {
+          cache.policy().expireVariably().get().put(key, value, 1, TimeUnit.SECONDS)
+        } else if (seconds > Int.MaxValue) {


Since the duration is a long parameter, you can probably drop this?

Got it, because this was only needed before because the expiration was int, now it's pointless, good catch.

ben-manes · 2017-11-21T20:34:39Z

What is Play's approach to exporting statistics?

Ehcache does it using JMX, but it is more framework-oriented than Caffeine. For us, we expose stats as pull (Cache#stats()) or push (recordStats(collector)). Then users can bridge into their preferred system. Would users inject in the named cache(s) to do that? It would be good to have a short note on how to collect the stats.

gmethvin · 2017-11-21T22:09:24Z

@dusanstanojeviccs Haven't had a chance to fully review, but this should be another module, in addition to the existing ehcache module. It should be no problem to have both modules documented and available to use.

dusanstanojeviccs · 2017-11-22T03:02:14Z

@gmethvin Got it, I think I removed it when I started working on this because I did not have a good grasp of how the whole Play build system with sbt is organized, but I have brought it back and all 'sbt tests' have passed :)

I have addressed the coding issues.

Now I am working on improving the docs for Play 2.7 migration and adding/correcting a few things about caffeine and jcache. Hopefully, the docs will be done tonight.

1. Removed testing loggers 2. removed not needed code 3. added support for named cache configs

ben-manes · 2017-11-22T03:55:42Z

framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineCacheManager.java

+        Caffeine cacheBuilder;
+
+        // we create the cache builder
+        // depending on weather the config for


dusanstanojeviccs · 2017-11-22T04:00:39Z

@marcospereira Loved the doc suggestions. I will do a few more doc changes tonight, but I think I'll do both of your requested changes tomorrow.

gmethvin · 2017-11-22T06:13:57Z

framework/build.sbt

-  ).dependsOn(PlayWsProject, PlayEhcacheProject % "test")
-  .dependsOn(PlaySpecs2Project % "test")
-  .dependsOn(PlayTestProject % "test->test")
+  ).dependsOn(PlayWsProject, PlayCaffeineCacheProject)


Does this need to be a compile dependency rather than just a test dependency?

If it was test before it should be test now since I did not change the Ahc module in any way, fixed and committed

gmethvin · 2017-12-03T00:56:51Z

framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineCacheManager.java

+
+            if (config.hasPath(pathKey)) {
+                cacheBuilder = CaffeineParser.from(config.getConfig(pathKey)).expireAfter(defaultExpiry);
+            } else if (config.hasPath(PLAY_CACHE_CAFFEINE_DEFAULTS)) {


You shouldn't need to check hasPath if it's in reference.conf. Basically the config is:

val configDefaults = config.getConfig(PLAY_CACHE_CAFFEINE_DEFAULTS) config.getConfig(pathKey).withFallback(configDefaults)

YES! I did not know withFallback existed, it made the code so much simpler. Thank you!

gmethvin · 2017-12-03T01:27:53Z

framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineParser.java

+    }
+
+    @SuppressWarnings("fallthrough")
+    private void parse(String key) {


I would prefer if we explicitly got the configuration elements we need.

These should all be documented in reference.conf. Ideally our defaults should look something like:

defaults = { initial-capacity = null maximum-size = null maximum-weight = null weak-keys = false soft-values = false record-stats = false }

In this case null can mean to use caffeine's default value. If Play wants to override a value by default, then it's as easy as updating the reference.conf.

Then we can explicitly get each of the keys. I don't really mind if you write this in Java but it would probably be simpler to write it in Scala using the play Configuration helper:

configuration.get[Option[Int]]("initial-capacity") foreach cacheBuilder.initialCapacity configuration.get[Option[Long]]("maximum-size") foreach cacheBuilder.maximumSize if (configuration.get[Boolean]("soft-values")) cacheBuilder.softValues()

and so on.

Cool, fallthrough has been implemented, I've kept the file in Java and just put 3 if statements around it for null to not set the values. It should be pretty easy to understand.

maximum-weight requires a Weigher, but I don’t think there is a current way to pass in an instance. A class literal with a Guice injector query would be a sad solution, though. So I don’t think this feature can be exposed without some additional work.

Got it, so I've removed maximum-weight, is that the right thing to do for now? If the feature becomes exposed we can always add it to a future PR.

I'm not sure I buy that fallthrough is the right way to do this. Even if you don't want to use the scala Configuration wrapper, you can use:

private void nullable(String key, Consumer<String> ifNotNull) { if (!config.getIsNull(key)) { ifNotNull(key); } }

then getting nullable keys is as easy as:

nullable("initial-capacity", key -> cacheBuilder.initialCapacity(config.getInt(key))) nullable("maximum-size", key -> cacheBuilder.maximumSize(config.getLong(key)))

If we have default values in reference.conf we don't need to worry about the case where the key does not exist.

And I don't think this code here works for null values.

I might not be understanding what you mean. Nulls are not connected to the fallthrough and are handled on line 64 in CaffeineParser, since there are 2 places they are called the whole nullable method seems like an overkill as compared to 2 if statements. I might be wrong but let me know if I misunderstood something.

Probably doesn't help, but I use fall-through and a default template in a similar way in my JCache adapter. It has to deal with some merging logic, too.

https://github.com/ben-manes/caffeine/blob/master/jcache/src/main/java/com/github/benmanes/caffeine/jcache/configuration/TypesafeConfigurator.java

Oh, I think you made the changes to handle nulls after I commented here. Still, I don't understand why the switch statement is necessary for this at all. Seems like overkill to have extra code to iterate over a map looking at keys, then other code to again get those keys. If you know what keys you're looking for you can just get them right away, since if we have default values in reference.conf we don't need to worry about them not existing.

gmethvin · 2017-12-03T01:32:33Z

framework/src/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

+ * CaffeineCache components for compile time injection
+ */
+trait CaffeineCacheComponents {
+  def environment: Environment


It doesn't look like you need environment or applicationLifecycle in here at all.

Yup, it's removed :)

gmethvin · 2017-12-03T01:37:50Z

framework/src/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

+  /**
+   * Use this to create with the given name.
+   */
+  def cacheApi(name: String, create: Boolean = true): AsyncCacheApi = {


Is the create needed here? We don't need to worry about having the exact same API as EhCacheComponents.

It's not used and since we dont have to keep the same API it is removed

gmethvin · 2017-12-03T01:46:05Z

...ework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineConfigConstants.java

+ */
+package play.cache.caffeine;
+
+public class CaffeineConfigConstants {


Reading your code I actually don't find it useful to have these in global string constants. Generally most of these need to be referenced in exactly one place besides the configuration.

I agree with you on this one, I was planning to remove them. I usually don't like having string literals in the code but they seem to be better for readability when it comes to config keys...

gmethvin · 2017-12-03T01:49:50Z

framework/src/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

+    new CaffeineCacheApi(NamedCaffeineCacheProvider.getNamedCache(name, caffeineCacheManager, configuration))(ec)
+  }
+
+  lazy val defaultCacheApi: AsyncCacheApi = cacheApi(PLAY_CACHE_CAFFEINE_DEFAULT_NAME)


I think this should use the default cache name, i.e. the value of the play.cache.defaultCache configuration.

Yes, that is a good catch, interestingly this was always passing "play" in the EhCache implementation

gmethvin · 2017-12-03T01:53:09Z

...ework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineCacheComponents.java

+ * }
+ * </pre>
+ */
+public interface CaffeineCacheComponents extends ConfigurationComponents, AkkaComponents {


Same comments as in the scala components.

Both are done

ben-manes · 2017-12-06T06:34:32Z

I think so. There are a few similar features, like removal listener, that could go into a future PR when users actually request them. Minimal seems wise for now.

…

On Tue, Dec 5, 2017 at 10:31 PM dusanstanojeviccs ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineParser.java <#8025 (comment)> : > + private CaffeineParser(Config config) { + this.cacheBuilder = Caffeine.newBuilder(); + this.config = Objects.requireNonNull(config); + } + + /** Returns a configured ***@***.*** Caffeine} cache builder. */ + public static Caffeine<Object, Object> from(Config config) { + CaffeineParser parser = new CaffeineParser(config); + + config.entrySet().stream().map(Map.Entry::getKey).forEach(parser::parse); + + return parser.cacheBuilder; + } + + @SuppressWarnings("fallthrough") + private void parse(String key) { Got it, so I've removed maximum-weight, is that the right thing to do for now? If the feature becomes exposed we can always add it to a future PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8025 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXG9oeed-OEucbBjYCk0kx4I7Z4Grs0ks5s9jTJgaJpZM4Qlawz> .

gmethvin · 2017-12-06T07:03:48Z

framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineParser.java

+    }
+
+    @SuppressWarnings("fallthrough")
+    private void parse(String key) {


I'm not sure I buy that fallthrough is the right way to do this. Even if you don't want to use the scala Configuration wrapper, you can use:

private void nullable(String key, Consumer<String> ifNotNull) { if (!config.getIsNull(key)) { ifNotNull(key); } }

then getting nullable keys is as easy as:

nullable("initial-capacity", key -> cacheBuilder.initialCapacity(config.getInt(key))) nullable("maximum-size", key -> cacheBuilder.maximumSize(config.getLong(key)))

If we have default values in reference.conf we don't need to worry about the case where the key does not exist.

And I don't think this code here works for null values.

gmethvin · 2017-12-06T07:22:48Z

framework/src/play-caffeine-cache/src/main/resources/reference.conf

+  cache {
+    # Data that should be used to configure the cache
+    caffeine {
+      defaults {}


Can you add the default values to reference.conf? It should look something like:

defaults { initialCapacity = null maximumSize = null weakKeys = false softValues = false recordStats = false }

Even if the value is null meaning the caffeine default, it's useful to have the settings documented here.

I was also thinking we should use camelCase here since that's consistent with what caffeine uses and camelCase is used in other places in this configuration. What do you think?

According to the HOCON spec,

Config keys are encouraged to be hyphen-separated rather than camelCase.

https://github.com/lightbend/config/blob/master/HOCON.md#hyphen-separated-vs-camelcase

I did this with hyphen separated key names:

caffeine { defaults { initial-capacity = null weak-keys = null weak-keys = false soft-values = false record-stats = false } caches {} }

gmethvin · 2017-12-06T20:21:00Z

For stats, I think the most basic implementation would be for the user to inject the cache instance and call stats(). Eventually I think we'd want to let the user bind their own instance of StatsCollector but that requires a bit more thinking so it's probably best to do that in another PR.

PromanSEW · 2018-02-12T09:38:51Z

documentation/manual/working/scalaGuide/main/cache/ScalaCache.md

@@ -7,32 +7,51 @@ Caching data is a typical optimization in modern applications, and so Play provi

 For any data stored in the cache, a regeneration strategy needs to be put in place in case the data goes missing. This philosophy is one of the fundamentals behind Play, and is different from Java EE, where the session is expected to retain values throughout its lifetime.

-The default implementation of the Cache API uses [Ehcache](http://ehcache.org/).
+Implmentations of both [Caffeine](https://github.com/ben-manes/caffeine/) and [EhCache](http://www.ehcache.org) are provided, using the Caffeine implementation is recommended.


Typo in the first word: Implementation

gmethvin · 2018-02-12T09:32:52Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md

 To add only the API, add `cacheApi` to your dependencies list.

 @[cache-sbt-dependencies](code/cache.sbt)

-The API dependency is useful if you'd like to define your own bindings for the `Cached` helper and `AsyncCacheApi`, etc., without having to depend on Ehcache. If you're writing a custom cache module you should use this.
+The API dependency is useful if you'd like to define your own bindings for the `Cached` helper and `AsyncCacheApi`, etc., without having to depend on Caffeine Cache. If you're writing a custom cache module you should use this.


instead of "caffeine cache" I'd just say "any specific cache impementation"

gmethvin · 2018-02-12T10:03:08Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md


 For any data stored in the cache, a regeneration strategy needs to be put in place in case the data goes missing. This philosophy is one of the fundamentals behind Play, and is different from Java EE, where the session is expected to retain values throughout its lifetime.

-The default implementation of the cache API uses [Ehcache](http://www.ehcache.org/).
+Implmentations of both [Caffeine](https://github.com/ben-manes/caffeine/) and [EhCache](http://www.ehcache.org) are provided, using the Caffeine implementation is recommended.


I think we can change the wording slightly here to suggest that the Ehcache 2.x implementation is a "legacy" implementation. Something like this:

"Play provides a CacheApi implementation based on Caffeine and a legacy implementation based on Ehcache 2.x. For in-process caching Caffeine is typically the best choice. If you need distributed caching, there are third-party plugins for memcached and redis."

It seems useful to mention the redis and memcached plugins because Play doesn't provide any guidance on how to do distributed caching with ehcache, and it usually makes more sense to use something else.

/cc @marcospereira: I'm assuming the plan is to introduce the new caffeine implementation and later deprecate and remove the ehcache impl completely.

gmethvin · 2018-02-12T10:08:52Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md

-Play provides both an API and an default Ehcache implementation of that API. To get the full Ehcache implementation, add `ehcache` to your dependencies list:
+### Caffeine
+
+Play provides both the API and the default Caffeine implementation of that API. To get the Caffeine implementation, add `caffeine` to your dependencies list:


The first sentence here "Play provides both the API and ..." (in both sections) should be moved to the top of the "Importing the Cache API" section and be something like "Play provides separate dependencies for the Cache API and for the Caffeine and Ehcache implementations."

gmethvin · 2018-02-12T10:18:35Z

framework/project/Dependencies.scala

  val playAhcWsDeps = Seq(
    "com.typesafe.play" %% "play-ahc-ws-standalone" % playWsStandaloneVersion,
    "com.typesafe.play" % "shaded-asynchttpclient" % playWsStandaloneVersion,
    "com.typesafe.play" % "shaded-oauth" % playWsStandaloneVersion,
-    "com.github.ben-manes.caffeine" % "jcache" % caffeineVersion % Test
+    "com.github.ben-manes.caffeine" % "jcache" % caffeineVersion % Test,
+    "net.sf.ehcache" % "ehcache" % ehcacheVersion,


Why did you need to add the ehcache dependency here?

Hey, thanks for pointing this out, the file OptionalAhcHttpCacheProviderSpec.scala has a test written for using ehcache with jcache, I've mirrored this into one more test like this:

val settings = Map( "play.ws.cache.enabled" -> "true", "play.ws.cache.cachingProviderName" -> classOf[CaffeineCachingProvider].getName, "play.ws.cache.cacheManagerResource" -> "caffeine.conf" )

So the whole ahc is now dependent on both caffeine and ehcache for this test to run. I am not sure if we want to change the ahc in this PR, if we want to do that let me know in which way and I'll adjust it.

If you just need it for tests then it should be:

"net.sf.ehcache" % "ehcache" % ehcacheVersion % Test

test dependencies are generally fine, but I'd prefer not to add runtime dependencies to play-ahc-ws.

Oh got it, that makes sense, thanks for pointing that out.

gmethvin · 2018-02-12T11:13:22Z

framework/src/play-caffeine-cache/src/main/resources/reference.conf

+
+  cache {
+    # Data that should be used to configure the cache
+    caffeine {


We need tests for custom configurations.

Written a new test for this, the only way I could see for testing this is to make sure that the proper cache builder is set up depending on the cache name (because the name is passed to it from the CaffeineCacheManager.getCache).

gmethvin · 2018-02-12T11:34:05Z

documentation/manual/working/scalaGuide/main/cache/ScalaCache.md


 ## Setting the execution context

-By default, all Ehcache operations are blocking, and async implementations will block threads in the default execution context.
+By default, all Caffeine operations are blocking, and async implementations will block threads in the default execution context.


Caffeine and Ehcache

gmethvin · 2018-02-12T11:40:05Z

...rk/src/play-caffeine-cache/src/test/scala/play/api/cache/caffeine/CaffeineCacheApiSpec.scala

+      _.overrides(
+        bind[CaffeineCacheManager].toProvider[CustomCacheManagerProvider]
+      ).configure(
+          "play.cache.createBoundCaches" -> false,


As far as I can tell this doesn't do anything for the caffeine implementation. This is meant to create a cache that's already in ehcache.xml, so the whole test is probably useless. Instead we should include some tests exercising some caffeine cache specific configuration.

gmethvin · 2018-02-12T12:00:15Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md

 Usually this is okay if you are using Play's default configuration, which only stores elements in memory since reads should be relatively fast.
-However, depending on how EhCache was configured and [where the data is stored](http://www.ehcache.org/generated/2.10.4/html/ehc-all/#page/Ehcache_Documentation_Set%2Fco-store_storage_tiers.html), this blocking I/O might be too costly.
-For such a case you can configure a different [Akka dispatcher](http://doc.akka.io/docs/akka/current/scala/dispatchers.html#looking-up-a-dispatcher) and set it via `play.cache.dispatcher` so the EhCache plugin makes use of it:
+However, depending on how Caffeine Cache was configured, this blocking I/O might be too costly.


I think this feature is really a lot more useful for Ehcache since it has slower storage tiers like caching to disk, so maybe worth keeping that part? /cc @mkurz.

gmethvin · 2018-02-12T12:00:53Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md


-If you want to access multiple different ehcache caches, then you'll need to tell Play to bind them in `application.conf`, like so:
+If you want to access multiple different caffeine cache caches, then you'll need to tell Play to bind them in `application.conf`, like so:


s/caffeine cache caches/named caches/

gmethvin · 2018-02-12T21:30:58Z

documentation/manual/working/scalaGuide/main/cache/ScalaCache.md


 @[jcache-sbt-dependencies](code/cache.sbt)

 If you are using Guice, you can add the following for Java annotations:

 @[jcache-guice-annotation-sbt-dependencies](code/cache.sbt)

+### JCache support with Caffeine
+
+Caffeine Cache does not natively implement the [JSR 107](https://github.com/jsr107/jsr107spec) specification, also known as JCache. If you want to use JCache with Caffeine you can use [caffeine-jcache](https://github.com/ben-manes/caffeine/wiki/JCache) dependency.


So actually I think the jcache module we need is already there in playCaffeineDeps so it shouldn't need to be added, right? And that documentation page just talks about configuring the JCache support for Caffeine, not how to add the dependency.

There is another confusing thing here: as far as I can tell, the JCache support for Caffeine would create a separate cache based on a separate configuration (which also uses typesafe config). @ben-manes what do you think is the best way to unify these two so we can have a common configuration?

I don't think unifying is a great option. All JCache implementations need to provide their own configuration support and each do it differently. Most appear to prefer XML, it seems. It would seem that you might want a generic integration strategy (e.g. Play JCache adapter) that fetches from the CacheManager than to couple to an implementation.

Caffeine's JCache will likely be written in v3 to simplify it. The reason is that variable expiration was a late feature, since doing it correctly is hard, and most JCache implementations use lazy expiration by requiring a maximum size. This behavior was emulated in the JCache layer, and later a timerwheel-based approach was implemented in the core library. This will result in a simpler config and implementation, so I'd like the option to not tie it deeply with Play's.

Also, why does Play appear to recommend JCache? If you've looked into it, its actually an awful specification. Many of its features are incompatible with each other, it was rejected by JEE multiple times (most recently by JEE-8), and has a poor user experience. It is useful for integrations, like Hibernate, but otherwise I consider it to be a step backwards. I certainly wouldn't use it in any of my code.

Perhaps this documentation needs to be reworded since we should definitely not be recommending JCache.

I'm not actually sure why we introduced JCache support in the first place. Probably to allow integration with something else. @wsargent can you provide any background? I think it was used in the WS API to provide a common API for caching requests.

Maybe the rewording should be something like "We do not recommend using JCache in your own code, but it may be useful for integration with other libraries. Both Caffeine and Ehcache provide JCache implementations."

@gmethvin I have added those sentences to the "JCache Support" header.

I'm not actually sure why we introduced JCache support in the first place. Probably to allow integration with something else. @wsargent can you provide any background? I think it was used in the WS API to provide a common API for caching requests.

The theory was that we may add Caffeine, but people would still be using Ehcache (given how long its stuck around) and so JCache is the lowest common denominator API between them, same as reactive streams is the base API for akka streams. The purpose of JCache is as an integration layer, so by using that we wouldn't have to go through API rework depending on which library we picked later on.

@wsargent You are absolutely right. From what I've seen JCache API is used for ws caching only. Regular caching is implemented directly on top of plays CacheApi (lowest denominator) interface so it has no dependencies on JCache. I have removed the JCache section from the caching page but have not touched the ws documentation, ws still uses JCache but does a really good job of explaining how to use it so I figured better not touch those docs.

moved time-set Typo in the first word: Implementation suggest that the Ehcache 2.x implementation is a "legacy" Importing the Cache API section change Caffeine and Ehcache

gmethvin · 2018-02-15T07:22:11Z

documentation/manual/releases/release27/Highlights27.md

+
+## Support for Caffeine
+
+Play now offeres an implementation of caching with Caffeine. Caffeine is the recomended cache implementation for Play users.


"Play now offers a CacheApi implementation based on Caffeine." (with the link)

gmethvin · 2018-02-15T07:22:13Z

documentation/manual/releases/release27/Highlights27.md

+
+Play now offeres an implementation of caching with Caffeine. Caffeine is the recomended cache implementation for Play users.
+
+To migrate from EhCache to Caffeine you will have to remove `ehcache` from your dependencies and replace it with `caffeine` and you will have to change your caching configuration.


Run-on sentence. Could be rewritten as: "To migrate from EhCache to Caffeine you will have to remove ehcache from your dependencies and replace it with caffeine. To customize the settings from the defaults you will also need to update the configuration in application.conf as explained in the documentation."

ben-manes · 2018-02-21T09:05:46Z

Thanks @dusanstanojeviccs!

@gmethvin are there remaining items for review?

gmethvin · 2018-03-01T06:33:33Z

framework/src/play-caffeine-cache/src/test/scala/play/api/cache/JavaCacheApiSpec.scala

+      val cacheApi = app.injector.instanceOf[JavaAsyncCacheApi]
+      Await.result(cacheApi.set("foo", "bar", 1 /* second */ ).toScala, 1.second)
+
+      Thread.sleep(2.seconds.toMillis)


Do we need these Thread.sleep calls?

I think that particular test sets expiration to 1 second, waits 2 seconds and check if the value really got cleared from the cache, that is why Thread.sleep is necessary.

We already have some of these in our specs. It could be good to move them later to use something like specs2 eventually.

But this may be out of the scope of this PR.

gmethvin · 2018-03-01T06:39:33Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md


 For any data stored in the cache, a regeneration strategy needs to be put in place in case the data goes missing. This philosophy is one of the fundamentals behind Play, and is different from Java EE, where the session is expected to retain values throughout its lifetime.

-The default implementation of the cache API uses [Ehcache](http://www.ehcache.org/).
+Play provides a CacheApi implementation based on [Caffeine](https://github.com/ben-manes/caffeine/) and a legacy implementation based on [Ehcache 2.x](http://www.ehcache.org). For in-process caching Caffeine is typically the best choice. If you need distributed caching, there are third-party plugins for memcached and redis.


You might want to link to https://github.com/mumoshu/play2-memcached and https://github.com/KarelCemus/play-redis.

Or maybe create a "Cache" section on ModuleDirectory.md and link to there.

marcospereira

Made a bunch of smaller comments/questions/suggestions.

Thanks for the amazing work and patience here, @dusanstanojeviccs.

marcospereira · 2018-03-01T17:04:22Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md

-Caching data is a typical optimization in modern applications, and so Play provides a global cache. An important point about the cache is that it behaves just like a cache should: the data you just stored may just go missing.
+Caching data is a typical optimization in modern applications, and so Play provides a global cache.
+
+> An important point about the cache is that it behaves just like a cache should: the data you just stored may just go missing.


Nitpick of docs formatting:

> **Note:** An important point ...

marcospereira · 2018-03-01T17:12:32Z

documentation/manual/working/javaGuide/main/cache/JavaCache.md


-@[jcache-guice-annotation-sbt-dependencies](code/cache.sbt)
+The API dependency is useful if you'd like to define your own bindings for the `Cached` helper and `AsyncCacheApi`, etc., without having to depend on any specific cache implementation. If you're writing a custom cache module you should use this.


Add links to Cached and AsyncCacheApi:

... your own bindings for the [`Cached`](api/java/play/cache/Cached.html) helper and [`AsyncCacheApi`](api/java/play/cache/AsyncCacheApi.html), etc., without ...

Also, for Java Cached is an annotation that does not have/need bindings.

marcospereira · 2018-03-01T17:20:49Z

documentation/manual/working/javaGuide/main/cache/code/ehcache.sbt

+)
+//#ehcache-sbt-dependencies
+
+//#jcache-sbt-dependencies


I think the reference for this snippet section was removed, just like jcache-guice-annotation-sbt-dependencies a few lines below. The same for documentation/manual/working/javaGuide/main/cache/code/cache.sbt.

marcospereira · 2018-03-01T17:24:00Z

documentation/manual/working/scalaGuide/main/cache/ScalaCache.md


-@[jcache-guice-annotation-sbt-dependencies](code/cache.sbt)
+The API dependency is useful if you'd like to define your own bindings for the `Cached` helper and `AsyncCacheApi`, etc., without having to depend on any specific cache implementation. If you're writing a custom cache module you should use this.


Add links to Cached and AsyncCacheApi:

... bindings for the [`Cached`](api/scala/play/api/cache/Cached.html) helper and [`AsyncCacheApi`](api/scala/play/api/cache/AsyncCacheApi.html), etc., without ...

marcospereira · 2018-03-01T17:31:43Z

documentation/manual/working/scalaGuide/main/cache/code/ehcache.sbt

+)
+//#ehcache-sbt-dependencies
+
+//#jcache-sbt-dependencies


Same for Java examples, is this still being used?

marcospereira · 2018-03-01T17:32:23Z

framework/project/Dependencies.scala

  val ehcacheVersion = "2.10.4"
  val playEhcacheDeps = Seq(
    "net.sf.ehcache" % "ehcache" % ehcacheVersion,
    "org.ehcache" % "jcache" % "1.0.1"
  ) ++ jcacheApi

-  val caffeineVersion = "2.5.6"
+  val caffeineVersion = "2.6.0"


Use caffeine 2.6.2.

marcospereira · 2018-03-01T18:00:49Z

framework/src/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineDefaultExpiry.java

+
+import com.github.benmanes.caffeine.cache.Expiry;
+
+import javax.annotation.Nonnull;


Nice. We may want to start using these annotations more widely. :-)

marcospereira · 2018-03-01T18:24:01Z

framework/src/play-caffeine-cache/src/test/scala/play/api/cache/JavaCacheApiSpec.scala

+      val cacheApi = app.injector.instanceOf[JavaAsyncCacheApi]
+      Await.result(cacheApi.set("foo", "bar", 1 /* second */ ).toScala, 1.second)
+
+      Thread.sleep(2.seconds.toMillis)


We already have some of these in our specs. It could be good to move them later to use something like specs2 eventually.

But this may be out of the scope of this PR.

marcospereira · 2018-03-01T18:28:16Z

framework/src/play-caffeine-cache/src/test/scala/play/api/cache/JavaCacheApiSpec.scala

+  "Java AsyncCacheApi" should {
+    "set cache values" in new WithApplication {
+      val cacheApi = app.injector.instanceOf[JavaAsyncCacheApi]
+      Await.result(cacheApi.set("foo", "bar").toScala, 1.second)


You can just use await(cacheApi("foo", "bar").toScala) instead, which already has a default timeout and is slightly less verbose. (await is part of PlaySpecification, see play.api.test.FutureAwaits).

marcospereira · 2018-03-01T18:39:34Z

framework/build.sbt

@@ -324,14 +325,24 @@ lazy val PlayEhcacheProject = PlayCrossBuiltProject("Play-Ehcache", "play-ehcach
      PlaySpecs2Project % "test"
    )

+lazy val PlayCaffeineCacheProject = PlayCrossBuiltProject("Play-Caffeine-Cache", "play-caffeine-cache")
+    .settings(
+      libraryDependencies ++= playCaffeineDeps


Also add the following setting here:

mimaPreviousArtifacts := Set.empty,

PromanSEW · 2018-03-02T04:38:57Z

documentation/manual/releases/release27/Highlights27.md

+
+## Support for Caffeine
+
+Play now offers a CacheApi implementation based on [Caffeine](https://github.com/ben-manes/caffeine/). Caffeine is the recomended cache implementation for Play users.


Typo: recommended

marcospereira

LGTM.

Thank you so much, @dusanstanojeviccs. This is a fantastic contribution.

mkurz · 2020-06-19T15:09:50Z

framework/src/play-caffeine-cache/src/main/resources/reference.conf

+      defaults {
+        initial-capacity = null
+        weak-keys = null
+        weak-keys = false


@dusanstanojeviccs I think you made a mistake here? weak-keys twice? Shouldn't be one of both configs be weak-values? See

playframework/cache/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineParser.java

Lines 68 to 70 in bc1badd

case "weak-values":

conditionally(key, cacheBuilder::weakValues);

break;

Also the config maximum-size = null is missing:

playframework/cache/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineParser.java

Lines 60 to 64 in bc1badd

case "maximum-size":

if (!config.getIsNull(key)) {

cacheBuilder.maximumSize(config.getLong(key));

}

break;

Fix can be found here: #10398

dusanstanojeviccs added 4 commits November 19, 2017 01:21

Created Play-Caffeine-Cache module

1a78650

Reading cache properties from conf file

6dbcbb0

put instead of setExpiresAfter

a97b4d2

Documentation changes for Caffeine Cache

ef3011d

marcospereira suggested changes Nov 21, 2017

View reviewed changes

ben-manes reviewed Nov 21, 2017

View reviewed changes

PR changes

38349b4

1. Removed testing loggers 2. removed not needed code 3. added support for named cache configs

ben-manes reviewed Nov 22, 2017

View reviewed changes

dusanstanojeviccs added 2 commits November 21, 2017 20:01

whether fix

5ec44df

removed spec from the config name

ee2183d

gmethvin reviewed Nov 22, 2017

View reviewed changes

dusanstanojeviccs added 3 commits November 21, 2017 22:43

build fix for test dependency

07a6587

constants cleanup + config fix

bdd9ead

removed loggers

47fe5ae

gmethvin reviewed Dec 3, 2017

View reviewed changes

dusanstanojeviccs added 3 commits December 5, 2017 22:03

Made default fallthrough + dropped constants

2036277

removed logger

69c56d5

removed maximum weight

d2d89bc

gmethvin reviewed Dec 6, 2017

View reviewed changes

PromanSEW reviewed Feb 12, 2018

View reviewed changes

gmethvin requested changes Feb 12, 2018

View reviewed changes

dusanstanojeviccs added 9 commits February 12, 2018 20:14

Progress

a757503

moved time-set Typo in the first word: Implementation suggest that the Ehcache 2.x implementation is a "legacy" Importing the Cache API section change Caffeine and Ehcache

improved Setting the execution context

3202b03

Removed useless test

f9fa499

New test

888f806

dependencies fix

981a7fd

Tests

c17aae7

Configuring named caches with EhCache

0647296

Recommend not using JCache

243270c

Renamed text

4720f25

gmethvin reviewed Feb 15, 2018

View reviewed changes

Highlights27 - rephrasing

91a2024

Dropped JCache from the Cache documentation

a5ab287

gmethvin approved these changes Mar 1, 2018

View reviewed changes

Added links for memcache and redis

344ab70

marcospereira reviewed Mar 1, 2018

View reviewed changes

dusanstanojeviccs added 2 commits March 1, 2018 20:13

Merge branch 'master' into master

95fa096

Updated tests and docs

7f93cae

PromanSEW reviewed Mar 2, 2018

View reviewed changes

dusanstanojeviccs added 3 commits March 1, 2018 22:00

cleanup

71fa2f8

Fixed scala 2.11 compilation issue + spelling

5e8b799

fixed missing headers

965ee4e

marcospereira approved these changes Mar 2, 2018

View reviewed changes

marcospereira merged commit 536378b into playframework:master Mar 2, 2018

marcospereira mentioned this pull request Sep 12, 2018

Cache API getOrElseUpdate should be atomic. #8605

Closed

TimMoore added this to the Play 2.7.0 milestone Dec 10, 2018

mkurz reviewed Jun 19, 2020

View reviewed changes


		play.cache.createBoundCaches = false
		play.cache.caffeine.spec = {}


		If you want to access multiple different ehcache caches, then you'll need to tell Play to bind them in `application.conf`, like so:
		If you want to access multiple different caffeine cache caches, then you'll need to tell Play to bind them in `application.conf`, like so:


		## Support for Caffeine

		Play now offeres an implementation of caching with Caffeine. Caffeine is the recomended cache implementation for Play users.


		Play now offeres an implementation of caching with Caffeine. Caffeine is the recomended cache implementation for Play users.

		To migrate from EhCache to Caffeine you will have to remove `ehcache` from your dependencies and replace it with `caffeine` and you will have to change your caching configuration.


		@[jcache-guice-annotation-sbt-dependencies](code/cache.sbt)
		The API dependency is useful if you'd like to define your own bindings for the `Cached` helper and `AsyncCacheApi`, etc., without having to depend on any specific cache implementation. If you're writing a custom cache module you should use this.


		import com.github.benmanes.caffeine.cache.Expiry;

		import javax.annotation.Nonnull;


		## Support for Caffeine

		Play now offers a CacheApi implementation based on [Caffeine](https://github.com/ben-manes/caffeine/). Caffeine is the recomended cache implementation for Play users.

	case "weak-values":
	conditionally(key, cacheBuilder::weakValues);
	break;

	case "maximum-size":
	if (!config.getIsNull(key)) {
	cacheBuilder.maximumSize(config.getLong(key));
	}
	break;

Added Caffeine Cache module and made it the default Cache #8025

Added Caffeine Cache module and made it the default Cache #8025

Conversation

dusanstanojeviccs commented Nov 21, 2017

Pull Request Checklist

Helpful things

Fixes

Purpose

Background Context

References

marcospereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusanstanojeviccs Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusanstanojeviccs Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

ben-manes Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-manes commented Nov 21, 2017

gmethvin commented Nov 21, 2017 • edited Loading

dusanstanojeviccs commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusanstanojeviccs commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmethvin Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-manes commented Dec 6, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusanstanojeviccs Dec 6, 2017 • edited Loading

dusanstanojeviccs Nov 22, 2017 •

edited

Loading

gmethvin Nov 22, 2017 •

edited

Loading

dusanstanojeviccs Nov 22, 2017 •

edited

Loading

ben-manes Nov 22, 2017 •

edited

Loading

gmethvin Dec 3, 2017 •

edited

Loading

gmethvin commented Nov 21, 2017 •

edited

Loading

gmethvin Dec 3, 2017 •

edited

Loading

gmethvin Dec 6, 2017 •

edited

Loading

dusanstanojeviccs Dec 6, 2017 •

edited

Loading

PromanSEW Feb 12, 2018 •

edited

Loading