Deprecate parTraverseN/parSequenceN with semaphore limit as Long

[satyamagarwal]

Issue

324

Status

READY

Description

The current apis parTraverseN and parSequenceN use semaphore limit as Long type, and convert to Int which can lead to overflow issue.

This pr deprecates these methods, sets a validation check to fail the api if limit is more than Int range, and introduces new apis that takes limit as Int type.

Todos

• Tests
• Documentation

Deploy Notes

Would like help on if I need to do something about deploying @nomisRev

Related PRs

• None

[nomisRev]

Couple of small nits. Great catch @satyamagarwal! Thanks for keeping such a close eye on this!! 👏 👏 👏

[satyamagarwal]

I don't know what previous build integration does, but it is failing everytime it runs.

[nomisRev]

Not sure how the build step managed to pass. The integration steps builds this project together with master of Arrow Core
There was a mistake in the tests, I tested this locally with the following stress test.

I also saw that we can add inline to the parTraverse.kt implementations, it requires the lambdas to be defined as crossinline f: suspend () -> Ato work properly.

checkAll(5000, Arb.constant(Unit)) { _ ->
  val ref = Atomic(0)
  (0 until 10_000)
    .map { suspend { ref.update { it + 1 } } }
    .parSequenceN(5)
  ref.get() shouldBe 10_00
}

Thanks again for the contribution @satyamagarwal! 👏 👏

[satyamagarwal]

"parSequenceN can traverse effect full computations" {
    checkAll(5000, Arb.constant(Unit)) { _ ->
      val ref = Atomic(0)
      (0 until 10_000)
        .map { suspend { ref.update { it + 1 } } }
        .parSequenceN(5)
      ref.get() shouldBe 10_000
    }
  }

this test doesn't finish on my machine.

[satyamagarwal]

I also saw that we can add inline to the parTraverse.kt implementations, it requires the lambdas to be defined as crossinline f: suspend () -> A to work properly.

This should probably be a new PR right ?

I can find time do it in a few days.

@nomisRev

[nomisRev]

@satyamagarwal a new PR for that is fine! Doesn't need to be in this one 👍

[nomisRev]

That stress test runs fine on my machine 🤔 It takes quite some time tho since it's launching 10_000 coroutines 5000 times, and it's doing so by limiting the parallelism to 5. 😀

[satyamagarwal]

this finally completed in 32 minutes. :D

[nomisRev]

Oh we probably don't want to push that test to CI 😅 I just linked it because that's what I tried locally before I realized we were using i from IntRange instead of from update.
It passed in 2-3 minutes locally for me.

[satyamagarwal]

okay. I'll revert it :)

[nomisRev]

Looks perfect! 👏 👏 Thank you so much for taking care of this @satyamagarwal

[raulraja]

Thanks @satyamagarwal !