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.

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.

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.

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.

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.

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.

whether

yup

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

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!

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.

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

Yup, it's removed :)

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

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...

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

Same comments as in the scala components.

Both are done

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.

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 {}
    }

Typo in the first word: Implementation

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

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.

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."

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.