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.

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

Offer migration path from Spray Caching #213

Closed
ktoso opened this Issue Sep 9, 2016 · 23 comments

Comments

Projects
None yet
4 participants
@ktoso
Member

ktoso commented Sep 9, 2016

Issue by ktoso
Wednesday Jun 01, 2016 at 09:15 GMT
Originally opened as akka/akka#20679


The question keeps coming up, and it's only one simple file https://github.com/spray/spray/tree/master/spray-caching/src/main/scala/spray/caching

We could provide it as separate mini project, declare it community lead.
Please ping me if you'd like to contribute this port so I can create a repo for it.

@ktoso ktoso added this to the http-backlog milestone Sep 9, 2016

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by zhxiaogg
Wednesday Jun 01, 2016 at 12:57 GMT


@ktoso It's about migrating the spray cache directive to Akka HTTP server DSL, right? If so, I would like to give it a try.

Member

ktoso commented Sep 9, 2016

Comment by zhxiaogg
Wednesday Jun 01, 2016 at 12:57 GMT


@ktoso It's about migrating the spray cache directive to Akka HTTP server DSL, right? If so, I would like to give it a try.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by ktoso
Wednesday Jun 01, 2016 at 12:59 GMT


Yes exactly - shouldn't be hard I think, and I think it should live outside of the akka/akka repo, but we can host it under the akka/ organisation perhaps as akka/akka-http-caching. I would like to see it be a community by-demand driven project, I don't think we (core team) will spend time on it, and at the same time we do want to allow the HTTP ecosystem to flourish a bit more.

Member

ktoso commented Sep 9, 2016

Comment by ktoso
Wednesday Jun 01, 2016 at 12:59 GMT


Yes exactly - shouldn't be hard I think, and I think it should live outside of the akka/akka repo, but we can host it under the akka/ organisation perhaps as akka/akka-http-caching. I would like to see it be a community by-demand driven project, I don't think we (core team) will spend time on it, and at the same time we do want to allow the HTTP ecosystem to flourish a bit more.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by zhxiaogg
Wednesday Jun 01, 2016 at 12:59 GMT


BTW, why a new project? Is it a planned repo to hold all extra directives?

Member

ktoso commented Sep 9, 2016

Comment by zhxiaogg
Wednesday Jun 01, 2016 at 12:59 GMT


BTW, why a new project? Is it a planned repo to hold all extra directives?

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by ktoso
Wednesday Jun 01, 2016 at 13:01 GMT


It does not quite fit nor would it be "driven by" lightbend, so a separate repo makes most sense.
Yes, I think we'll want to have a repo to encourage more HTTP community participation, I hope to have a detailed plan for that laid out in a few weeks.

Member

ktoso commented Sep 9, 2016

Comment by ktoso
Wednesday Jun 01, 2016 at 13:01 GMT


It does not quite fit nor would it be "driven by" lightbend, so a separate repo makes most sense.
Yes, I think we'll want to have a repo to encourage more HTTP community participation, I hope to have a detailed plan for that laid out in a few weeks.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by zhxiaogg
Wednesday Jun 01, 2016 at 13:09 GMT


Great, looking forward to the plan.

Member

ktoso commented Sep 9, 2016

Comment by zhxiaogg
Wednesday Jun 01, 2016 at 13:09 GMT


Great, looking forward to the plan.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by hepin1989
Wednesday Jun 01, 2016 at 15:13 GMT


https://github.com/ben-manes/caffeine/
We could try to make use of this:)

Member

ktoso commented Sep 9, 2016

Comment by hepin1989
Wednesday Jun 01, 2016 at 15:13 GMT


https://github.com/ben-manes/caffeine/
We could try to make use of this:)

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by ktoso
Wednesday Jun 01, 2016 at 15:17 GMT

Member

ktoso commented Sep 9, 2016

Comment by ktoso
Wednesday Jun 01, 2016 at 15:17 GMT

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 9, 2016

Member

Comment by ben-manes
Wednesday Jun 01, 2016 at 17:13 GMT


Thanks @hepin1989.

Please do prefer Caffeine. I no longer actively develop ConcurrentLinkedHashMap (used by Spray Caching) or Guava's cache. If you use the jdk8-scala adapters you might prefer to use an AsyncLoadingCache for future handling. Also note that ascendingKeys would use cache.policy to obtain the eviction order, though I'm surprised that's being used (intended for warm restart).

Member

ktoso commented Sep 9, 2016

Comment by ben-manes
Wednesday Jun 01, 2016 at 17:13 GMT


Thanks @hepin1989.

Please do prefer Caffeine. I no longer actively develop ConcurrentLinkedHashMap (used by Spray Caching) or Guava's cache. If you use the jdk8-scala adapters you might prefer to use an AsyncLoadingCache for future handling. Also note that ascendingKeys would use cache.policy to obtain the eviction order, though I'm surprised that's being used (intended for warm restart).

@ktoso ktoso added the 1 - triaged label Sep 9, 2016

@ianclegg

This comment has been minimized.

Show comment
Hide comment
@ianclegg

ianclegg Jan 5, 2017

Contributor

@zhxiaogg I assume you were not able to pick this up?

Contributor

ianclegg commented Jan 5, 2017

@zhxiaogg I assume you were not able to pick this up?

@ianclegg

This comment has been minimized.

Show comment
Hide comment
@ianclegg

ianclegg Jan 5, 2017

Contributor

@ktoso, Had a quick look at this, Caffeine is impressive exceptional. I followed up @ben-manes suggestion, but sadly AsyncLoadingCache is not a good fit. It's designed to use a single value loader implementation across all keys. Contrast that with Spray Caching, where callers can pass a unique, key specific value loader when fetching/putting into cache and the problem is apparent.

It's not a big deal - Caffeine's BoundedLocalCache looks like the way to go.

Contributor

ianclegg commented Jan 5, 2017

@ktoso, Had a quick look at this, Caffeine is impressive exceptional. I followed up @ben-manes suggestion, but sadly AsyncLoadingCache is not a good fit. It's designed to use a single value loader implementation across all keys. Contrast that with Spray Caching, where callers can pass a unique, key specific value loader when fetching/putting into cache and the problem is apparent.

It's not a big deal - Caffeine's BoundedLocalCache looks like the way to go.

@ben-manes

This comment has been minimized.

Show comment
Hide comment
@ben-manes

ben-manes Jan 5, 2017

@ianclegg can you use Cache.get(key, mappingFunction)?

ben-manes commented Jan 5, 2017

@ianclegg can you use Cache.get(key, mappingFunction)?

@ben-manes

This comment has been minimized.

Show comment
Hide comment
@ben-manes

ben-manes Jan 5, 2017

Because there hasn't been a justification for AsyncCache, the quick hack is to use a dummy loader (e.g. key -> null). Then you can use AsyncLoadingCache,get(key, mappingFunction) too. The manual loading methods are on the interface to make it easy for us to add AsyncCache later if there's is enough demand.

ben-manes commented Jan 5, 2017

Because there hasn't been a justification for AsyncCache, the quick hack is to use a dummy loader (e.g. key -> null). Then you can use AsyncLoadingCache,get(key, mappingFunction) too. The manual loading methods are on the interface to make it easy for us to add AsyncCache later if there's is enough demand.

@ianclegg

This comment has been minimized.

Show comment
Hide comment
@ianclegg

ianclegg Jan 5, 2017

Contributor

@ben-manes Perfect 👍

Contributor

ianclegg commented Jan 5, 2017

@ben-manes Perfect 👍

@ben-manes

This comment has been minimized.

Show comment
Hide comment
@ben-manes

ben-manes Jan 5, 2017

You may also prefer to use the Scala wrapper - Scaffeine, which provides a nice idiomatic view.

ben-manes commented Jan 5, 2017

You may also prefer to use the Scala wrapper - Scaffeine, which provides a nice idiomatic view.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jan 6, 2017

Member

Hi there! Thanks for chiming in @ben-manes :-)

I think we'd actually prefer to depend on the raw java library - makes it easier to cross publish for different scala versions as we're not dependent on a 3rd party library then. Akka is kind of at the root of the tree of many dependencies so we try to publish for stable scala versions ASAP, so while it won't matter for a year or so it'll be nicer to depend on the raw java lib (also, since we hide the API behind directives anyway, users won't see much of the Caffeine API I think).

Sounds like you're interested in hacking on this @ianclegg? That would be really great! Let us know if you need any hints etc.

Member

ktoso commented Jan 6, 2017

Hi there! Thanks for chiming in @ben-manes :-)

I think we'd actually prefer to depend on the raw java library - makes it easier to cross publish for different scala versions as we're not dependent on a 3rd party library then. Akka is kind of at the root of the tree of many dependencies so we try to publish for stable scala versions ASAP, so while it won't matter for a year or so it'll be nicer to depend on the raw java lib (also, since we hide the API behind directives anyway, users won't see much of the Caffeine API I think).

Sounds like you're interested in hacking on this @ianclegg? That would be really great! Let us know if you need any hints etc.

@ianclegg

This comment has been minimized.

Show comment
Hide comment
@ianclegg

ianclegg Jan 10, 2017

Contributor

Sure, I have all the key pieces - i'll put together a PR - perhaps this evening - otherwise towards the end of the week and we can refine.

Contributor

ianclegg commented Jan 10, 2017

Sure, I have all the key pieces - i'll put together a PR - perhaps this evening - otherwise towards the end of the week and we can refine.

@ianclegg

This comment has been minimized.

Show comment
Hide comment
@ianclegg

ianclegg Jan 10, 2017

Contributor

@ktoso Earlier in this issue you mentioned creating a new repo for caching - whats your current thoughts on this? I noticed the unusual version number bump for akka-http 😏 should the cache/PR now sit in this repo.

We'll have around 3 or so source files consisting of the cache directive, the cache trait and the cache implementations to find a home for.

Contributor

ianclegg commented Jan 10, 2017

@ktoso Earlier in this issue you mentioned creating a new repo for caching - whats your current thoughts on this? I noticed the unusual version number bump for akka-http 😏 should the cache/PR now sit in this repo.

We'll have around 3 or so source files consisting of the cache directive, the cache trait and the cache implementations to find a home for.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jan 11, 2017

Member

Thea earlier note on separate repo was indeed before we worked out the new versioning and split of akka-http into its own repo. Now it should be fine to include here, as separate project but in the same repo. We'd mark it as an experimental module.

Thanks in advance :)

Member

ktoso commented Jan 11, 2017

Thea earlier note on separate repo was indeed before we worked out the new versioning and split of akka-http into its own repo. Now it should be fine to include here, as separate project but in the same repo. We'd mark it as an experimental module.

Thanks in advance :)

@ianclegg

This comment has been minimized.

Show comment
Hide comment
@ianclegg

ianclegg Jan 11, 2017

Contributor

@ben-manes Spray caching also wrapped the size(), keySet(), remove(key) methods on ConcurrentLinkedHashMap. It looks like the equivalent api's for Caffeine Cache can be reached via the LoadingCache from the synchronous() method.

Off the top of your head, is going via synchronous() a significant performance/memory hit over the equivalent calls for ConcurrentLinkedHashMap. That is to say is calling size(), keySet(), remove(key) better optimised in ConcurrentLinkedHashMap. I know thats a poor question, but whats you hunch on it 😏

To give you the background, although Spray Caching wraps them, they are not actually required by the Caching Directive, so there may be an option to simply drop them from the wrapper if the synchronous() route is discouraged

gist: https://gist.github.com/ianclegg/dc1254a77f815df64ffe2636696df4f2

Contributor

ianclegg commented Jan 11, 2017

@ben-manes Spray caching also wrapped the size(), keySet(), remove(key) methods on ConcurrentLinkedHashMap. It looks like the equivalent api's for Caffeine Cache can be reached via the LoadingCache from the synchronous() method.

Off the top of your head, is going via synchronous() a significant performance/memory hit over the equivalent calls for ConcurrentLinkedHashMap. That is to say is calling size(), keySet(), remove(key) better optimised in ConcurrentLinkedHashMap. I know thats a poor question, but whats you hunch on it 😏

To give you the background, although Spray Caching wraps them, they are not actually required by the Caching Directive, so there may be an option to simply drop them from the wrapper if the synchronous() route is discouraged

gist: https://gist.github.com/ianclegg/dc1254a77f815df64ffe2636696df4f2

@ben-manes

This comment has been minimized.

Show comment
Hide comment
@ben-manes

ben-manes Jan 11, 2017

The synchronous() and asMap() views are effectively free. For an asynchronous cache it can be more expensive if it requires blocking, e.g. to return the result. So a cache.synchronous().invalidate(key) would be cheaper than cache.synchronous().asMap().remove(key) since the latter requires returning the non-future value. Otherwise the actual removal from the hash map is similar performance to the underlying ConcurrentHashMap, with a little more work to maintain the other data structures.

The write performance actually increased in Caffeine compared to CLHM, which can be a bit surprising. CLHM relies on spin loops to make the operations atomic whereas Caffeine synchronizes on the entry. But I wouldn't expect the benchmarked difference to be observable in real applications.

Caffeine uses JDK8's rewritten CHM which is a big improvement. CLHM adopted a backport of that, but is loosely coupled so it worked with any ConcurrentMap.

I don't think there's any degradation except for the remove(key): Option[Future[V]] method. Since there is no asynchronous asMap() view it would become either a Unit or V result, the latter being blocking. That would require forgoing the async view and uses a Cache[K, Future[V]] instead.

We could make async caches more flexible, as I error'd on simplicity to observe usages.

ben-manes commented Jan 11, 2017

The synchronous() and asMap() views are effectively free. For an asynchronous cache it can be more expensive if it requires blocking, e.g. to return the result. So a cache.synchronous().invalidate(key) would be cheaper than cache.synchronous().asMap().remove(key) since the latter requires returning the non-future value. Otherwise the actual removal from the hash map is similar performance to the underlying ConcurrentHashMap, with a little more work to maintain the other data structures.

The write performance actually increased in Caffeine compared to CLHM, which can be a bit surprising. CLHM relies on spin loops to make the operations atomic whereas Caffeine synchronizes on the entry. But I wouldn't expect the benchmarked difference to be observable in real applications.

Caffeine uses JDK8's rewritten CHM which is a big improvement. CLHM adopted a backport of that, but is loosely coupled so it worked with any ConcurrentMap.

I don't think there's any degradation except for the remove(key): Option[Future[V]] method. Since there is no asynchronous asMap() view it would become either a Unit or V result, the latter being blocking. That would require forgoing the async view and uses a Cache[K, Future[V]] instead.

We could make async caches more flexible, as I error'd on simplicity to observe usages.

@ianclegg

This comment has been minimized.

Show comment
Hide comment
@ianclegg

ianclegg Jan 11, 2017

Contributor

Sounds like we shouldn't have a problem to continue supporting the spray Cache trait using those wrappers then. As I mentioned the remove method won't be used by the Cache Directive anyway, I think it was just there to enable the cache to be used in applications outside of Spray. Thanks again!

Contributor

ianclegg commented Jan 11, 2017

Sounds like we shouldn't have a problem to continue supporting the spray Cache trait using those wrappers then. As I mentioned the remove method won't be used by the Cache Directive anyway, I think it was just there to enable the cache to be used in applications outside of Spray. Thanks again!

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Feb 28, 2017

Member

Hi there - are you still working on this @ianclegg?

Member

ktoso commented Feb 28, 2017

Hi there - are you still working on this @ianclegg?

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Mar 20, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Mar 21, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Mar 21, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Mar 30, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Apr 3, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue May 9, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Jun 7, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Jul 1, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Aug 13, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Aug 14, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Aug 14, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Sep 23, 2017

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Oct 21, 2017

Migrate Spray caching as akka-http-caching akka#213
Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

tomrf1 added a commit to tomrf1/akka-http that referenced this issue Oct 29, 2017

Migrate Spray caching as akka-http-caching akka#213
Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

ktoso added a commit to ktoso/akka-http that referenced this issue Oct 31, 2017

Migrate Spray caching as akka-http-caching akka#213
Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

@ktoso ktoso modified the milestones: backlog, 10.0.11 Nov 15, 2017

raboof added a commit that referenced this issue Nov 21, 2017

Migrate Spray caching as akka-http-caching #213
Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

raboof added a commit that referenced this issue Nov 21, 2017

Migrate Spray caching as akka-http-caching #213
Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

Increase delay in cache test

ApiMayChange annotation

Remove test that sometimes fails due to race condition

Exclude akka-http-caching from MiMa previous artifacts

Remove the need for a loader on LfuCache creation

Simplify CachingDirectives - remove CacheSpecMagnet

MiMa ignoredModules

Refactor LfuCache - replace subclasses with store param

raboof added a commit that referenced this issue Nov 21, 2017

Migrate Spray caching as akka-http-caching #213
Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

Increase delay in cache test

@ApiMayChange annotations

Remove test that sometimes fails due to race condition

Exclude akka-http-caching from MiMa previous artifacts

Remove the need for a loader on LfuCache creation

Simplify CachingDirectives - remove CacheSpecMagnet

MiMa ignoredModules

Refactor LfuCache - replace subclasses with store param

Docs and example code for caching directives

Move LfuCache params to LfuCacheSettings to support binary compatibility

Replace Keyed and ValueMagnet classes with overloaded apply and get methods

Remove default value for keyer function

Remove build.sbt

Update docs

Make cache key type generic

Fix Cache.remove comment

WIP towards JavaDSL for akka-http-caching

Use JavaMapping

formatting

copyright, docs, annotations

Make CacheJavaMapping internal

Java API markers

getStrict -> getOrCreateStrict

Remove simpleKeyer

Use LfuCacheSettings in routeCache

Example keyer for docs

Fix for sbt 1.0

Make Java DSL implement proper CachingSettings companion

=cache make settings reference.conf and mark things as internal

ktoso added a commit that referenced this issue Nov 21, 2017

New module: Akka-HTTP-Caching (#1539) #213
* Migrate Spray caching as akka-http-caching #213

Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

Increase delay in cache test

@ApiMayChange annotations

Remove test that sometimes fails due to race condition

Exclude akka-http-caching from MiMa previous artifacts

Remove the need for a loader on LfuCache creation

Simplify CachingDirectives - remove CacheSpecMagnet

MiMa ignoredModules

Refactor LfuCache - replace subclasses with store param

Docs and example code for caching directives

Move LfuCache params to LfuCacheSettings to support binary compatibility

Replace Keyed and ValueMagnet classes with overloaded apply and get methods

Remove default value for keyer function

Remove build.sbt

Update docs

Make cache key type generic

Fix Cache.remove comment

WIP towards JavaDSL for akka-http-caching

Use JavaMapping

formatting

copyright, docs, annotations

Make CacheJavaMapping internal

Java API markers

getStrict -> getOrCreateStrict

Remove simpleKeyer

Use LfuCacheSettings in routeCache

Example keyer for docs

Fix for sbt 1.0

Make Java DSL implement proper CachingSettings companion

=cache make settings reference.conf and mark things as internal

* Documentation, tests and various cleanups for caching

=doc Add server-side overview page (#1371)

Revert formatting changes back to how Scalariform formats

Mark more settings constructors private

Use self reference to implement with... methods

Override productPrefix in the setting implementations

Fix implementation of withLfuCacheSettings

Implement withLfuCacheSettings in the Java DSL and provide a
Scala-specific implementation as well.

Fix compilation error in the caching directive example

Fix places where the Java DSL leaks into the Scala DSL

For example apply methods should always take the Scala DSL instance
where as create methods should take the Java DSL instances.

Caching docs (#6)

* Fix use of fragments and IDs in directives

* Add Java caching directives examples

* Document Java caching directives

* Document the caching design and implementation

Port spray-caching documentation and update to the new caching API.

* Document the akka-http-caching reference config

* Fix reference to Java DSL cache

* Fix another Cache reference

* Reword docs related to the LFU cache

* cleaning up paradox links

ktoso added a commit that referenced this issue Nov 21, 2017

Migrate Spray caching as akka-http-caching #213
Rename to Lfu

Make remove return Unit

Move caffeine dependency to Dependencies.scala

Fix CachingDirectives package name

Increase delay in cache test

@ApiMayChange annotations

Remove test that sometimes fails due to race condition

Exclude akka-http-caching from MiMa previous artifacts

Remove the need for a loader on LfuCache creation

Simplify CachingDirectives - remove CacheSpecMagnet

MiMa ignoredModules

Refactor LfuCache - replace subclasses with store param

Docs and example code for caching directives

Move LfuCache params to LfuCacheSettings to support binary compatibility

Replace Keyed and ValueMagnet classes with overloaded apply and get methods

Remove default value for keyer function

Remove build.sbt

Update docs

Make cache key type generic

Fix Cache.remove comment

WIP towards JavaDSL for akka-http-caching

Use JavaMapping

formatting

copyright, docs, annotations

Make CacheJavaMapping internal

Java API markers

getStrict -> getOrCreateStrict

Remove simpleKeyer

Use LfuCacheSettings in routeCache

Example keyer for docs

Fix for sbt 1.0

Make Java DSL implement proper CachingSettings companion

=cache make settings reference.conf and mark things as internal
@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 30, 2017

Member

Was added with #1503 / #1539.

Member

jrudolph commented Nov 30, 2017

Was added with #1503 / #1539.

@jrudolph jrudolph closed this Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment