Incorporate MapRef into Std #2464

ChristopherDavenport · 2021-10-25T17:04:22Z

Integrates MapRef.

Features: ShardedMapRef, ConcurrentHashMap, and Scala Concurrent Map Implementations, and Defaulted Ref/MapRef.

Why Care? This enables keyed concurrent state machines that don't space leak. It basically allows almost any state machine with a default state to be lifted to a keyed version of the same state machine that is removed entirely when it enters some zero state.

tests/jvm/src/test/scala/cats/effect/std/MapRefTrieSpec.scala

ChristopherDavenport · 2021-10-25T17:08:05Z

std/shared/src/main/scala/cats/effect/std/MapRef.scala

+   * Also useful for anytime a shared storage location is used for a ref, i.e. DB or Redis to not waste
+   * space.
+   **/
+  def defaultedRef[F[_]: Functor, A: Eq](ref: Ref[F, Option[A]], default: A): Ref[F, A] = 


This makes sense here as this is where it necessary. But might be useful elsewhere. We could also rely on global equality if we like being risky or offer a global equality version.

armanbilge · 2021-11-01T22:39:59Z

tests/jvm/src/test/scala/cats/effect/std/MapRefJVMSpec.scala

@@ -169,4 +169,40 @@ class MapRefTrieSpec extends BaseSpec {

  }

+  "MapRef" should { // Requires unsafeRunSync so doesn't work with JS


Interesting note, it's not just that JS doesn't have unsafeRunSync so this test doesn't work, it's actually that this test is meaningless on JS anyway because concurrent modifications are impossible :)

armanbilge · 2021-11-01T22:55:53Z

Just a thought, could we abstract the current RefSpec in tests and reuse it for the various MapRef implementations?

https://github.com/typelevel/cats-effect/blob/series/3.x/tests/shared/src/test/scala/cats/effect/kernel/RefSpec.scala

djspiewak

I really like this as an addition to the stdlib. My concerns basically boil down to the following:

Confusing and IMO unnecessary of/in constructor duplication
Inaccurate typeclass constraints faked by using unit.map(_ => ...) rather than delay(...)
Syntax requires unnecessary imports
Minor bikeshedding on test structure
Lots of unnecessary and likely-inefficient use of lazy val rather than def

djspiewak · 2021-11-07T18:26:03Z

std/jvm/src/main/scala/cats/effect/std/MapRefCompanionPlatform.scala

+    Concurrent[F]
+      .unit
+      .map(_ => scala.collection.concurrent.TrieMap.empty[K, V])


Very leery of something like this. :-)

djspiewak · 2021-11-07T18:27:04Z

std/shared/src/main/scala/cats/effect/std/MapRef.scala

+    class HandleRef(k: K) extends Ref[F, Option[V]] {
+
+      def access: F[(Option[V], Option[V] => F[Boolean])] = for {
+        hasBeenCalled <- Applicative[F].unit.map(_ => new AtomicBoolean(false))


…and here too. Why not simply embrace the fact that this is really forcing Sync[F]?

I'm going to open an issue for what I'm about to say, and I also want to give a general review of MapRef (tl;dr mostly amazing, some api bikeshedding), but I'm going to drop it here in case I forget: we should relax the semantics on access to not need hasBeenCalled. It was in "broken" form in fs2 for ages, the only reason this was introduced was to conform to the docs, and it's a pain in the arse for things that want to implement Ref, like SignallingRef or MapRef.

djspiewak · 2021-11-07T18:28:25Z

std/shared/src/main/scala/cats/effect/std/MapRef.scala

+   * equally distributed Hash, the contention should allow for interaction like a general
+   * datastructure. Created in G, operates in F.
+   */
+  def inShardedImmutableMap[G[_]: Sync, F[_]: Async, K, V](


If we more fully embrace Sync, then we don't need to have the of/in split in the constructors.

djspiewak · 2021-11-07T18:29:45Z

std/shared/src/main/scala/cats/effect/std/syntax/MapRefSyntax.scala

+import cats.syntax.all._
+
+trait MapRefSyntax {
+  implicit def mapRefOptionSyntax[F[_], K, V](


You can actually stick this into the MapRef companion and it won't require the import.

djspiewak · 2021-11-07T18:30:55Z

tests/shared/src/test/scala/cats/effect/std/MapRefSpec.scala

+
+  "MapRef" should {
+
+    "MapRef.ofSingleImmutableMapRef - concurrent modifications" in real {


Uh… we're in Specs2, so this kind of test name is pretty confusing. :-) I would prefer if we did "ofSingleImmutableMapRef" >> { ... } and then inside that block have "concurrent modifications" >> real { etc etc.

durban · 2021-12-12T21:44:27Z

This seems pretty useful. What I find very strange is that it uses universal equality/hash for the keys, while there is a Hash typeclass in Cats.

ChristopherDavenport · 2022-03-22T20:55:42Z

Proposal:

Remove the access requirement on replay so concurrent constraint can get met by Ref implementation. (or allow the concurrent.map unit case for this single location)
Make a single default implementation that matches on the effect type like the Queue implementation does that delegates to CHM on Async and Ref on Concurrent
All the current constructors live on in MapRef library but the trait and default lives here. That way people can still leverage the interface, but the advanced uses still have the library for those more advanced use cases that would bulk up the binary interface in cats-effect too much.

Thoughts?

Questions with this: Def we want configurable CHM initialCapacity, loadFactor and concurrencyLevel? For The Ref of Map Implementation should there be a default striping we leverage?

SystemFw · 2022-03-22T21:11:50Z

I'm in favour of the proposal above, and I think it would be good to get this into CE :)

durban · 2022-03-23T00:16:05Z

I would recommend basing the default implementation on TrieMap instead, since that can support using the Hash typeclass for the keys.

Incorporate MapRef

b27a156

ChristopherDavenport commented Oct 25, 2021

View reviewed changes

tests/jvm/src/test/scala/cats/effect/std/MapRefTrieSpec.scala Outdated Show resolved Hide resolved

ChristopherDavenport commented Oct 25, 2021

View reviewed changes

ChristopherDavenport added 3 commits October 25, 2021 10:09

scalafmt

42bc059

Reinstate Commented Tests

cd58294

Move unsafeRunSync based tests to JVM tests

c44c0ff

armanbilge reviewed Nov 1, 2021

View reviewed changes

djspiewak requested changes Nov 7, 2021

View reviewed changes

SystemFw self-requested a review December 12, 2021 21:03

djspiewak added the ❗ source breaking ❗ label Jan 1, 2022

djspiewak added 🍄 enhancement and removed ❗ source breaking ❗ labels Jan 16, 2022

Fix Tests, Syntax, lazy val

13f6d78

armanbilge mentioned this pull request Mar 22, 2022

typeclass-based collections implementations typelevel/cats#4147

Open

ChristopherDavenport added 5 commits March 22, 2022 12:47

Make Concurrent Version Pure by remove unsafe atomic for access

53dd685

Remove Syntax

92bc745

Merge remote-tracking branch 'upstream/series/3.x' into mapRef

c78a986

Header fix

81df999

scalafmt

3decbac

ChristopherDavenport added 3 commits March 22, 2022 13:56

Lingering Syntax References

2232f91

Remove Test For Dual Access Call

489ac59

scalafix

5f3a975

armanbilge added this to the v3.4.0 milestone Mar 22, 2022

armanbilge mentioned this pull request Mar 22, 2022

Relax Ref#access semantics if set more than once #2901

Merged

ChristopherDavenport requested a review from djspiewak March 23, 2022 22:04

djspiewak approved these changes Jun 19, 2022

View reviewed changes

djspiewak merged commit 8f905b8 into typelevel:series/3.x Jun 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporate MapRef into Std #2464

Incorporate MapRef into Std #2464

ChristopherDavenport commented Oct 25, 2021 •

edited

Loading

ChristopherDavenport Oct 25, 2021

armanbilge Nov 1, 2021

armanbilge commented Nov 1, 2021 •

edited

Loading

djspiewak left a comment

djspiewak Nov 7, 2021

djspiewak Nov 7, 2021

SystemFw Dec 12, 2021 •

edited

Loading

djspiewak Nov 7, 2021

djspiewak Nov 7, 2021

djspiewak Nov 7, 2021

durban commented Dec 12, 2021

ChristopherDavenport commented Mar 22, 2022 •

edited

Loading

SystemFw commented Mar 22, 2022

durban commented Mar 23, 2022

		@@ -169,4 +169,40 @@ class MapRefTrieSpec extends BaseSpec {

		}

		"MapRef" should { // Requires unsafeRunSync so doesn't work with JS


		"MapRef" should {

		"MapRef.ofSingleImmutableMapRef - concurrent modifications" in real {

Incorporate MapRef into Std #2464

Incorporate MapRef into Std #2464

Conversation

ChristopherDavenport commented Oct 25, 2021 • edited Loading

ChristopherDavenport Oct 25, 2021

Choose a reason for hiding this comment

armanbilge Nov 1, 2021

Choose a reason for hiding this comment

armanbilge commented Nov 1, 2021 • edited Loading

djspiewak left a comment

Choose a reason for hiding this comment

djspiewak Nov 7, 2021

Choose a reason for hiding this comment

djspiewak Nov 7, 2021

Choose a reason for hiding this comment

SystemFw Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

djspiewak Nov 7, 2021

Choose a reason for hiding this comment

djspiewak Nov 7, 2021

Choose a reason for hiding this comment

djspiewak Nov 7, 2021

Choose a reason for hiding this comment

durban commented Dec 12, 2021

ChristopherDavenport commented Mar 22, 2022 • edited Loading

SystemFw commented Mar 22, 2022

durban commented Mar 23, 2022

ChristopherDavenport commented Oct 25, 2021 •

edited

Loading

armanbilge commented Nov 1, 2021 •

edited

Loading

SystemFw Dec 12, 2021 •

edited

Loading

ChristopherDavenport commented Mar 22, 2022 •

edited

Loading