Parallel performance improvement by manufacturist · Pull Request #2159 · typelevel/cats-effect

manufacturist · 2021-07-26T14:38:45Z

First PR. Be gentle 🙏 🎉 PR covering #2155:

~~mapBoth~~
~~map2 in terms of mapBoth~~
~~GenConcurrent.mapBoth override~~
map2 & map2Eval overrides (with mapBoth solution)
~~unit tests~~ (No longer needed due to overriding map2 & map2Eval)
benchmarks + profiling results

Delegate the implementation to both in GenSpawn, but then override that implementation in GenConcurrent with something implemented in terms of Fiber and Deferred. The results should be a noticeable improvement in performance of the par operators.

In abstract, I need to ensure that two effects run in parallel, obtain their results and map over them.

Qs:

The cancelled *> never trick ensures that never will never run, therefore it is only used throughout the cats-effect code to help with matching the type signature where needed. Is this correct?
When joining a fiber in a construct such as outcome <- poll(workFiber.join), is there any need for calling onCancel or guarantee? Is there the possibility of having the main fiber cancelled while the workFiber hasn't joined yet? And if yes, then the mentioned constructs guards us against such scenarios?
Why would the solution need to be implemented in terms of fibers AND Deferred? Why isn't fibers enough, e.g.

  override def mapBoth[A, B, C](fa: F[A], fb: F[B])(f: (A, B) => F[C]): F[C] = {
    implicit val F: GenConcurrent[F, E] = this

    uncancelable { poll =>
      for {
        fiberA <- start(fa)
        fiberB <- start(fb)

        outcomeA <- poll(fiberA.join).guarantee(fiberA.cancel)
        outcomeB <- poll(fiberB.join).guarantee(fiberB.cancel)

        resultC <- (outcomeA, outcomeB) match {
          case (Outcome.Succeeded(fa), Outcome.Succeeded(fb)) =>
            fa.product(fb).flatMap { case (a, b) => f(a, b) }

          case (Outcome.Errored(e), _) => raiseError(e)
          case (_, Outcome.Errored(e)) => raiseError(e)
          case (Outcome.Canceled(), _) => F.canceled *> never
          case (_, Outcome.Canceled()) => F.canceled *> never
        }
      } yield resultC
     }
    }

djspiewak

This is great! Just a couple adjustments. Also, @vasilmkd do we have a parTraverse benchmark already? We should add one and check the difference.

vasilmkd · 2021-07-26T15:13:32Z

Unfortunately I don't think we have a parTraverse benchmark. It would be great to have a suite of benchmarks for traverse and parTraverse, maybe it can be done in a sepate PR so that we can test before and after. Sorry about this.

djspiewak · 2021-07-26T15:19:22Z

Unfortunately I don't think we have a parTraverse benchmark. It would be great to have a suite of benchmarks for traverse and parTraverse, maybe it can be done in a sepate PR so that we can test before and after. Sorry about this.

No worries! I was thinking of just doing it as part of this PR actually. @manufacturist are you comfortable working with JMH? Would you like to give it a try? Otherwise I (or Vasil) can get to it and push here.

vasilmkd · 2021-07-26T15:24:58Z

Feel free to ask and to look into the benchmarks sources and copy/paste liberally.

djspiewak · 2021-07-26T16:56:31Z

Missed the questions! Answers incoming…

The cancelled *> never trick ensures that never will never run, therefore it is only used throughout the cats-effect code to help with matching the type signature where needed. Is this correct?

The problem here is a little more existential. If it was just a bit of type juggling I would be a lot more worried about it! Here's the issue:

F.uncancelable(_ => F.both(F.canceled.as(42), F.unit))

What should this produce?

The problem here is the type signature of both is too limited to properly represent this scenario. We can't produce a value of type (Int, Unit), and we can't cancel ourselves (because of uncancelable), so we have to either error or spin forever. The error approach doesn't work either though because it might be caught within the uncancelable block, etc etc.

When joining a fiber in a construct such as outcome <- poll(workFiber.join), is there any need for calling onCancel or guarantee? Is there the possibility of having the main fiber cancelled while the workFiber hasn't joined yet? And if yes, then the mentioned constructs guards us against such scenarios?

Precisely as you said. :-) Also I'm realizing that there's actually a leak here: it's possible that the parent fiber is canceled prior to the first poll, in which case it will show up there but only one of the child fibers will be canceled! We need something a little trickier using guaranteeCase to ensure that both fibers get canceled if either of them error or cancel.

Why would the solution need to be implemented in terms of fibers AND Deferred? Why isn't fibers enough, e.g.

I believe you're correct! I'm pretty sure we don't need Deferred at all here.

manufacturist · 2021-07-26T18:34:04Z

Unfortunately I don't think we have a parTraverse benchmark. It would be great to have a suite of benchmarks for traverse and parTraverse, maybe it can be done in a sepate PR so that we can test before and after. Sorry about this.

No worries! I was thinking of just doing it as part of this PR actually. @manufacturist are you comfortable working with JMH? Would you like to give it a try? Otherwise I (or Vasil) can get to it and push here.

I'll give it a try 😄

djspiewak · 2021-07-26T20:23:51Z

@manufacturist If it helps, I literally always copy/paste from something pre-existing for JMH. I recommend just copying something like the BothBenchmark, removing all the cases, and writing a new one for parTraverse. Call it ParallelBenchmark or something like that. The parTraverse benchmark can probably be pretty simple, just like traverse over a list of size n (where n: Int is a @Param and you can set it to like, "10000" or something). Anyway, just create a list of size n and then parTraverse over it to an IO which uses BlackHole.consumeCPU() with some small (but not totally trivial number). It might be useful to also add an identical traverse benchmark in the same file. We would expect that parTraverse would be k times faster than n, where k is the number of physical threads on the underlying machine.

vasilmkd · 2021-07-26T20:29:18Z

Maintainers caught admitting copy/pasting code all over the place.

djspiewak · 2021-07-26T20:31:16Z

Maintainers caught admitting copy/pasting code all over the place.

My favorite Sublime key combo is Cmd-Shift-D: it duplicates the current line. :-P

manufacturist · 2021-07-27T10:24:25Z

Updated the mapBoth override to drop Deffered from the solution & removed overrides for mapBoth in instances. Added the benchmarks for parTraverse and traverse only with val cpuTimeTokens = 10000.

Executing Machine

Processor Name:         Quad-Core Intel Core i7
Processor Speed:        2,2 GHz
Number of Processors:   1
Total Number of Cores:  4

All benchmarks executed with variations of jmh:run -i X -wi Y -f 2 -t 4 cats.effect.benchmarks.ParallelBenchmark

CPU Time Tokens = 200

[info] Benchmark                                  (size)   Mode  Cnt      Score     Error  Units
[info] ParallelBenchmark.traverse100               10000  thrpt   15  37910.533 ± 398.023  ops/s
[info] ParallelBenchmark.traverse1000              10000  thrpt   15   4742.708 ± 207.744  ops/s
[info] ParallelBenchmark.traverse10000             10000  thrpt   15    506.790 ±  25.914  ops/s
[info] ParallelBenchmark.parTraverseBoth100        10000  thrpt   15   3632.091 ± 354.683  ops/s
[info] ParallelBenchmark.parTraverseBoth1000       10000  thrpt   15    435.991 ±   8.477  ops/s
[info] ParallelBenchmark.parTraverseBoth10000.     10000  thrpt   15     55.030 ±   0.788  ops/s
[info] ParallelBenchmark.parTraverseMapBoth100     10000  thrpt   10   7824.942 ± 111.829  ops/s
[info] ParallelBenchmark.parTraverseMapBoth1000    10000  thrpt   10   1049.031 ± 151.450  ops/s
[info] ParallelBenchmark.parTraverseMapBoth10000   10000  thrpt   10    108.418 ±  13.729  ops/s

CPU Time Tokens = 2000 - `mapBoth` approach

[info] Benchmark                           (size)   Mode  Cnt     Score     Error  Units
[info] ParallelBenchmark.parTraverse100     10000  thrpt    5  7291.586 ± 621.810  ops/s
[info] ParallelBenchmark.parTraverse1000    10000  thrpt    5   729.836 ± 143.587  ops/s
[info] ParallelBenchmark.parTraverse10000   10000  thrpt    5    62.743 ±   3.068  ops/s
[info] ParallelBenchmark.traverse100        10000  thrpt    5  6977.727 ± 218.018  ops/s
[info] ParallelBenchmark.traverse1000       10000  thrpt    5   755.024 ±  26.890  ops/s
[info] ParallelBenchmark.traverse10000      10000  thrpt    5    72.412 ±  10.234  ops/s

CPU Time Tokens = 5000 - `mapBoth` approach

[info] ParallelBenchmark.parTraverse100     10000  thrpt    6  4741.184 ± 173.918  ops/s
[info] ParallelBenchmark.parTraverse1000    10000  thrpt    6   476.457 ±  74.728  ops/s
[info] ParallelBenchmark.parTraverse10000   10000  thrpt    6    38.966 ±   2.960  ops/s
[info] ParallelBenchmark.traverse100        10000  thrpt    6  2945.512 ±  55.656  ops/s
[info] ParallelBenchmark.traverse1000       10000  thrpt    6   298.424 ±  25.320  ops/s
[info] ParallelBenchmark.traverse10000      10000  thrpt    6    31.545 ±   0.906  ops/s

CPU Time Tokens = 10000 - `mapBoth` approach

[info] ParallelBenchmark.parTraverse100     10000  thrpt    6  2726.039 ±  19.494  ops/s
[info] ParallelBenchmark.parTraverse1000    10000  thrpt    6   283.107 ±   2.026  ops/s
[info] ParallelBenchmark.parTraverse10000   10000  thrpt    6    25.774 ±   4.185  ops/s
[info] ParallelBenchmark.traverse100        10000  thrpt    6  1496.449 ± 147.759  ops/s
[info] ParallelBenchmark.traverse1000       10000  thrpt    6   151.205 ±   3.623  ops/s
[info] ParallelBenchmark.traverse10000      10000  thrpt    6    15.209 ±   1.546  ops/s

How I interpret the benchmarks:

Non-CPU intensive tasks result in bad parTraverse performance due to an occuring performance cost from internal CE operating mechanisms (it's a black box for me 😅)
CPU intensive tasks are an excellent candidate for parTraverse
The mapBoth solution provides a better performance than the old one using both (GenConcurrent.mapBoth not overriden)

vasilmkd · 2021-07-27T11:11:09Z

+
+  override def mapBoth[A, B, C](fa: F[A], fb: F[B])(f: (A, B) => F[C]): F[C] = {
+    implicit val F: GenConcurrent[F, E] = this
+
+    uncancelable { poll =>
+      for {
+        fiberA <- start(fa)
+        fiberB <- start(fb)
+
+        outcomeA <- poll(fiberA.join).onCancel(fiberB.cancel)
+        outcomeB <- poll(fiberB.join)
+
+        resultC <- (outcomeA, outcomeB) match {
+          case (Outcome.Succeeded(fa), Outcome.Succeeded(fb)) =>
+            fa.flatMap(a => fb.flatMap(b => f(a, b)))
+
+          case (Outcome.Errored(e), _) => raiseError(e)
+          case (_, Outcome.Errored(e)) => raiseError(e)
+          case (Outcome.Canceled(), _) => F.canceled *> never
+          case (_, Outcome.Canceled()) => F.canceled *> never
+        }
+      } yield resultC
+    }
+  }


Now that there are no GenConcurrent constructs used, we should try to move this code in GenSpawn. This was an observation by @TimWSpence, thank you!

I feel this block of code needs a good review from more pairs of eyes.

That being said, if the logic does end up changing, this code may indeed need to live in GenConcurrent.

vasilmkd · 2021-07-27T11:19:53Z

@manufacturist Are the benchmark results you posted only for the current branch which includes the changes in this PR?

manufacturist · 2021-07-27T11:24:20Z

@manufacturist Are the benchmark results you posted only for the current branch which includes the changes in this PR?

They are. The first batch (CPU Time Tokens = 200) also includes results for the old implementation of map2.

vasilmkd · 2021-07-27T11:28:36Z

I would like to split the benchmark code out of this PR, open it as a separate PR and then when we merge it, rebase this PR and do a proper comparison for every case between series/3.x and this branch.

vasilmkd · 2021-07-27T12:48:12Z

@manufacturist The original issue has been changed in light of new information. I suggest that we split out the benchmarks into a separate PR and push that through, and then we can work together on tackling this problem again and be a bit more focused this time round.

djspiewak · 2021-07-27T14:44:57Z

I think it's okay to keep updating this PR in light of the revised requirements. It's good history. :-) Agree that splitting the benchmarks out into a separate PR (#2167) is a good move.

vasilmkd · 2021-07-27T15:52:06Z

Can you drop the commit that adds the ParallelBenchmark?

manufacturist · 2021-07-27T17:35:13Z

Can you drop the commit that adds the ParallelBenchmark?

Will do so in ~12 hours.

djspiewak · 2021-07-27T19:53:43Z

@manufacturist New plan! Let's skip the mapBoth in GenSpawn and instead just put that same implementation into map2 for commutativeApplicativeForParallelF. In fact, if you copy/paste the implementation into map2Eval in the same class, it should also fix #2095. This is great. :-D

@vasilmkd gets all the credit for this realization btw.

vasilmkd · 2021-07-27T20:08:15Z

Plan of attack that @manufacturist can follow tomorrow:

Drop the ParallelBenchmark commit and rebase on series/3.x, you'll get it from there.
Reimplement map2 in commutativeApplicativeForParallelF.
You can attempt to override map2Eval in commutativeApplicativeForParallelF too, and then I will push the unit tests that we have prepared for that particular bug, or you can choose to ignore this, your call, we can also add that code to the already open PR

…allelF

manufacturist · 2021-07-29T07:59:57Z

Will rerun the benchmarks later today to be able to compare them with the ones from #2167.

manufacturist · 2021-07-29T19:34:00Z

Finished running the benchmarks. They can be compared with the ones from the other PR:

[info] Benchmark                      (cpuTokens)  (size)   Mode  Cnt      Score     Error  Units
[info] ParallelBenchmark.parTraverse          100     100  thrpt   20   9944.533 ± 211.074  ops/s
[info] ParallelBenchmark.parTraverse          100    1000  thrpt   20   1428.851 ±   9.781  ops/s
[info] ParallelBenchmark.parTraverse          100   10000  thrpt   20    139.946 ±   1.383  ops/s
[info] ParallelBenchmark.parTraverse         1000     100  thrpt   20   8184.030 ±  39.369  ops/s
[info] ParallelBenchmark.parTraverse         1000    1000  thrpt   20   1008.832 ±   2.568  ops/s
[info] ParallelBenchmark.parTraverse         1000   10000  thrpt   20     98.693 ±   0.729  ops/s
[info] ParallelBenchmark.parTraverse        10000     100  thrpt   20   2526.056 ±  29.223  ops/s
[info] ParallelBenchmark.parTraverse        10000    1000  thrpt   20    257.168 ±   2.535  ops/s
[info] ParallelBenchmark.parTraverse        10000   10000  thrpt   20     25.164 ±   0.491  ops/s
[info] ParallelBenchmark.parTraverse       100000     100  thrpt   20    305.078 ±   1.061  ops/s
[info] ParallelBenchmark.parTraverse       100000    1000  thrpt   20     30.497 ±   0.589  ops/s
[info] ParallelBenchmark.parTraverse       100000   10000  thrpt   20      3.006 ±   0.084  ops/s
[info] ParallelBenchmark.parTraverse      1000000     100  thrpt   20     31.570 ±   0.133  ops/s
[info] ParallelBenchmark.parTraverse      1000000    1000  thrpt   20      3.164 ±   0.017  ops/s
[info] ParallelBenchmark.parTraverse      1000000   10000  thrpt   20      0.330 ±   0.034  ops/s
[info] ParallelBenchmark.traverse             100     100  thrpt   20  61728.601 ± 694.063  ops/s
[info] ParallelBenchmark.traverse             100    1000  thrpt   20   7589.424 ±  84.756  ops/s
[info] ParallelBenchmark.traverse             100   10000  thrpt   20    873.365 ±   2.606  ops/s
[info] ParallelBenchmark.traverse            1000     100  thrpt   20  15077.902 ±  89.639  ops/s
[info] ParallelBenchmark.traverse            1000    1000  thrpt   20   1622.568 ±  10.988  ops/s
[info] ParallelBenchmark.traverse            1000   10000  thrpt   20    165.695 ±   1.015  ops/s
[info] ParallelBenchmark.traverse           10000     100  thrpt   20   1770.700 ±   0.867  ops/s
[info] ParallelBenchmark.traverse           10000    1000  thrpt   20    177.990 ±   0.240  ops/s
[info] ParallelBenchmark.traverse           10000   10000  thrpt   20     17.709 ±   0.125  ops/s
[info] ParallelBenchmark.traverse          100000     100  thrpt   20    179.605 ±   0.153  ops/s
[info] ParallelBenchmark.traverse          100000    1000  thrpt   20     17.886 ±   0.097  ops/s
[info] ParallelBenchmark.traverse          100000   10000  thrpt   20      1.737 ±   0.067  ops/s
[info] ParallelBenchmark.traverse         1000000     100  thrpt   20     17.896 ±   0.069  ops/s
[info] ParallelBenchmark.traverse         1000000    1000  thrpt   20      1.729 ±   0.075  ops/s
[info] ParallelBenchmark.traverse         1000000   10000  thrpt   20      0.176 ±   0.008  ops/s

The traverse ones (cpuTokens = 100) are slightly worse possibly due to the fact that I ran the benchmarks immediately after a work day, whereas for the other ones I restarted my machine beforehand (The other run took 4h, this one took 4h20m).

For my machine, the performance gain for parTraverse is visible up until the 10k cpuTokens / 10k size benchmark. Afterwards it kinda syncs with the other batch of results. I would've been happy (and curious) to provide some benchmarks with 8 threads, however my machine is limited in this situation 🤷

vasilmkd · 2021-07-29T22:07:01Z

Here are the runs from a 16 core cloud instance:

series/3.x:

Benchmark                      (cpuTokens)  (size)   Mode  Cnt     Score    Error  Units
ParallelBenchmark.parTraverse          100     100  thrpt   10  1022.928 ± 97.351  ops/s
ParallelBenchmark.parTraverse          100    1000  thrpt   10   590.857 ± 17.301  ops/s
ParallelBenchmark.parTraverse          100   10000  thrpt   10   114.312 ±  1.927  ops/s
ParallelBenchmark.parTraverse         1000     100  thrpt   10   920.307 ± 61.221  ops/s
ParallelBenchmark.parTraverse         1000    1000  thrpt   10   541.315 ±  8.762  ops/s
ParallelBenchmark.parTraverse         1000   10000  thrpt   10    91.450 ±  9.160  ops/s
ParallelBenchmark.parTraverse        10000     100  thrpt   10   627.563 ± 11.182  ops/s
ParallelBenchmark.parTraverse        10000    1000  thrpt   10   282.210 ±  4.189  ops/s
ParallelBenchmark.parTraverse        10000   10000  thrpt   10    36.335 ±  0.490  ops/s
ParallelBenchmark.parTraverse       100000     100  thrpt   10   364.892 ±  5.622  ops/s
ParallelBenchmark.parTraverse       100000    1000  thrpt   10    47.056 ±  0.736  ops/s
ParallelBenchmark.parTraverse       100000   10000  thrpt   10     4.995 ±  0.053  ops/s
ParallelBenchmark.parTraverse      1000000     100  thrpt   10    46.086 ±  0.445  ops/s
ParallelBenchmark.parTraverse      1000000    1000  thrpt   10     5.107 ±  0.044  ops/s
ParallelBenchmark.parTraverse      1000000   10000  thrpt   10     0.524 ±  0.005  ops/s

This PR:

Benchmark                      (cpuTokens)  (size)   Mode  Cnt     Score    Error  Units
ParallelBenchmark.parTraverse          100     100  thrpt   10  1923.358 ± 94.031  ops/s
ParallelBenchmark.parTraverse          100    1000  thrpt   10   802.741 ± 20.411  ops/s
ParallelBenchmark.parTraverse          100   10000  thrpt   10   213.009 ±  5.766  ops/s
ParallelBenchmark.parTraverse         1000     100  thrpt   10  1452.882 ± 55.123  ops/s
ParallelBenchmark.parTraverse         1000    1000  thrpt   10   735.565 ± 22.182  ops/s
ParallelBenchmark.parTraverse         1000   10000  thrpt   10   156.585 ±  5.453  ops/s
ParallelBenchmark.parTraverse        10000     100  thrpt   10   976.165 ± 59.770  ops/s
ParallelBenchmark.parTraverse        10000    1000  thrpt   10   316.931 ± 31.421  ops/s
ParallelBenchmark.parTraverse        10000   10000  thrpt   10    41.550 ±  2.564  ops/s
ParallelBenchmark.parTraverse       100000     100  thrpt   10   396.673 ±  6.149  ops/s
ParallelBenchmark.parTraverse       100000    1000  thrpt   10    48.691 ±  0.602  ops/s
ParallelBenchmark.parTraverse       100000   10000  thrpt   10     5.116 ±  0.065  ops/s
ParallelBenchmark.parTraverse      1000000     100  thrpt   10    46.660 ±  0.361  ops/s
ParallelBenchmark.parTraverse      1000000    1000  thrpt   10     5.093 ±  0.089  ops/s
ParallelBenchmark.parTraverse      1000000   10000  thrpt   10     0.528 ±  0.004  ops/s

I think this is a spectacular improvement and well worth the code duplication. Amazing work @manufacturist! You should be proud.

vasilmkd · 2021-07-29T22:08:15Z

I will do another pass over the code and give my final approval later.

vasilmkd

@djspiewak Please take a close look when reviewing the changes. I'd love an explanation on a couple of confusion points I encountered in the review. Thanks.

vasilmkd · 2021-07-29T23:22:32Z

+              fiberA <- F.start(ParallelF.value(fa))
+              fiberB <- F.start(ParallelF.value(fb))
+
+              a <- F.onCancel(poll(fiberA.join), fiberB.cancel).flatMap[A] {


Here's a scenario, we get canceled while trying to join the fiberA. However, this does not cancel fiberA.

Is this a valid concern or am I missing something?

This is a valid concern! We need to cancel both of the fibers if either join is canceled. And in fact, we need to ensure that this cancelation is itself run in parallel, so the correct finalizer here would be F.onCancel(poll(fiberA.join), F.both(fiberB.cancel, fiberA.cancel)).

vasilmkd · 2021-07-29T23:24:42Z

+              a <- F.onCancel(poll(fiberA.join), fiberB.cancel).flatMap[A] {
+                case Outcome.Succeeded(fa) => fa
+                case Outcome.Errored(e) => fiberB.cancel *> F.raiseError(e)
+                case Outcome.Canceled() => fiberB.cancel *> F.never


I don't quite understand this interaction. I fear this will enter F.never here.

Should this line be: fiberB.cancel *> F.canceled *> F.never instead? I also don't quite understand how this all interacts with the outer uncancelable.

This should indeed be poll(fiberB.cancel *> F.canceled *> F.never) (and analogously in the b case)

djspiewak

Just the changes noted and confirmed in @vasilmkd's review. Otherwise, this looks good! Very impressive work.

djspiewak

Excellent work!

vasilmkd

Very impactful change @manufacturist! Amazing contribution. Thank you for your patience while we figured things out along the way. It's usually how great PRs come together. 😄

johnynek · 2021-07-30T23:24:57Z

@@ -53,7 +53,53 @@ trait GenSpawnInstances {
      final override def map2[A, B, Z](fa: ParallelF[F, A], fb: ParallelF[F, B])(


this code is exactly the same as below, except for the Eval wrapping, no?

Why not have one private method that returns Now[ParallelF[F, Z]] and in the map2 case pass fa, Now(fb)? I think the extra now wrap unwrap is likely trivial.

I think the extra now wrap unwrap is likely trivial.

The unwrap is pretty trivial but the allocation is… less so. Particularly since you pay that penalty on a per-item basis.

manufacturist added 3 commits July 26, 2021 16:21

Add GenSpawn.mapBoth & instances

0cd2ceb

Implement CommutativeApplicative.map2 in terms of mapBoth

7859ba3

Override GenConcurrent.mapBoth

f33d6bc

manufacturist changed the title ~~#2155 mapBoth implementation~~ mapBoth implementation Jul 26, 2021

djspiewak reviewed Jul 26, 2021

View reviewed changes

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala Outdated

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/GenConcurrent.scala Outdated

manufacturist added 2 commits July 27, 2021 13:15

Drop Deferred usage from mapBoth override in GenConcurrent

09e53c2

Remove mapBoth override from GenSpawn instances

2413e27

vasilmkd requested changes Jul 27, 2021

View reviewed changes

manufacturist force-pushed the 2155-map-both-implementation branch from 79838aa to c599b30 Compare July 27, 2021 15:43

manufacturist added 3 commits July 29, 2021 10:46

Move mapBoth to GenSpawn & remove overrides in instances

6d68a0c

Fix 'safety: lhs error during rhs interruptible region' ResourceSpec

9fd83cf

Add onCancel finalizer

006a0e4

manufacturist force-pushed the 2155-map-both-implementation branch from c599b30 to 006a0e4 Compare July 29, 2021 07:48

manufacturist added 2 commits July 29, 2021 10:51

Drop mapBoth; override map2, map2Eval in commutativeApplicativeForPar…

ae121bd

…allelF

Merge branch 'series/3.x' into 2155-map-both-implementation

d5f4f69

manufacturist changed the title ~~mapBoth implementation~~ Parallel performance improvement Jul 29, 2021

manufacturist and others added 3 commits July 29, 2021 12:17

Nitpick variable name (from old mapBoth implementation)

2b8a670

Add a Kleisli parTraverse stack overflow unit test

f068de8

Use Ref instead of capturing side effects

aac0738

vasilmkd mentioned this pull request Jul 29, 2021

Kleisli parTraverse #2156

Closed

This was linked to issues Jul 29, 2021

[CE3] Kleisli Stack safety #2095

Closed

Improve performance of Parallel by overriding map2 to avoid tupling #2155

Closed

vasilmkd reviewed Jul 29, 2021

View reviewed changes

vasilmkd requested a review from djspiewak July 29, 2021 23:27

djspiewak requested changes Jul 30, 2021

View reviewed changes

Make map2 & map2Eval cancellation safe

0f3400f

manufacturist requested review from djspiewak and vasilmkd July 30, 2021 06:34

vasilmkd reviewed Jul 30, 2021

View reviewed changes

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/instances/GenSpawnInstances.scala Outdated

Poll after fiber cancel

8b19b6b

djspiewak approved these changes Jul 30, 2021

View reviewed changes

vasilmkd reviewed Jul 30, 2021

View reviewed changes

Comment thread kernel/shared/src/main/scala/cats/effect/kernel/instances/GenSpawnInstances.scala

vasilmkd approved these changes Jul 30, 2021

View reviewed changes

vasilmkd merged commit 93d150c into typelevel:series/3.x Jul 30, 2021

manufacturist deleted the 2155-map-both-implementation branch July 30, 2021 19:18

johnynek reviewed Jul 30, 2021

View reviewed changes

armanbilge mentioned this pull request Nov 26, 2022

"Only Once" example hangs #3283

Closed

durban mentioned this pull request Feb 20, 2023

Parallel map2 optimization #3428

Merged

		@@ -53,7 +53,53 @@ trait GenSpawnInstances {
		final override def map2[A, B, Z](fa: ParallelF[F, A], fb: ParallelF[F, B])(

Uh oh!

Uh oh!

Conversation

manufacturist commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djspiewak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vasilmkd commented Jul 26, 2021

Uh oh!

djspiewak commented Jul 26, 2021

Uh oh!

vasilmkd commented Jul 26, 2021

Uh oh!

djspiewak commented Jul 26, 2021

Uh oh!

manufacturist commented Jul 26, 2021

Uh oh!

djspiewak commented Jul 26, 2021

Uh oh!

vasilmkd commented Jul 26, 2021

Uh oh!

djspiewak commented Jul 26, 2021

Uh oh!

manufacturist commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Executing Machine

CPU Time Tokens = 200

CPU Time Tokens = 2000 - mapBoth approach

CPU Time Tokens = 5000 - mapBoth approach

CPU Time Tokens = 10000 - mapBoth approach

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasilmkd commented Jul 27, 2021

Uh oh!

manufacturist commented Jul 27, 2021

Uh oh!

vasilmkd commented Jul 27, 2021

Uh oh!

vasilmkd commented Jul 27, 2021

Uh oh!

djspiewak commented Jul 27, 2021

Uh oh!

vasilmkd commented Jul 27, 2021

Uh oh!

manufacturist commented Jul 27, 2021

Uh oh!

djspiewak commented Jul 27, 2021

Uh oh!

vasilmkd commented Jul 27, 2021

Uh oh!

manufacturist commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manufacturist commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasilmkd commented Jul 29, 2021

Uh oh!

vasilmkd commented Jul 29, 2021

Uh oh!

vasilmkd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manufacturist commented Jul 26, 2021 •

edited

Loading

manufacturist commented Jul 27, 2021 •

edited

Loading

CPU Time Tokens = 2000 - `mapBoth` approach

CPU Time Tokens = 5000 - `mapBoth` approach

CPU Time Tokens = 10000 - `mapBoth` approach

manufacturist commented Jul 29, 2021 •

edited

Loading

manufacturist commented Jul 29, 2021 •

edited

Loading