=act Make Behavior.start tailrec.

[He-Pin]

Make it a tailrec and using a depth limit to it.

[He-Pin]

just hard code this for safety

[akka-ci]

Test PASSed.

[akka-ci]

Test PASSed.

[akka-ci]

Test PASSed.

[johanandren]

Not really a big improvement replacing the thread stack with our own, with another limit, is it?

[He-Pin]

I think we can adjust the max depth and it's more consistent when error occur, isn't it?
Different environments have different Xss configs.

[johanandren]

I don't expect that we will ever see a stack overflow here even with small stacks, if the behavior stack grows so much that it does hit the max stack size it will be because of a bug, for example when each interpreted message adds a new interceptor somehow.

I think the comment that it should/could be tailrec was perhaps misleading

[He-Pin]

This number can be adjusted.

[octonato]

But how is 8192 chosen? And how would that be adjusted?

I don't think we should have a fixed magical number. We never know in which JVM this will run. And having it adjustable by the user seems to me a leaked abstraction.

[octonato]

ah ok, I should have search before commenting. 8192 is the deault StackOverflow size.

Not true what I wrote here. My question still then relevant. Why 8192? The issue is memory, not amount of nesting.

[He-Pin]

@renatocaval For myself,I want to remove the limit of depth too, but as a PR, I think it would be great to simulate it's previous behavior.
I just pick up 8192 randomly because we do not know how many frames can be there before it throws a StackOverflowError.

// note that this can't be @tailrec, but normal stack of interceptors and similar shouldn't be
// that deep, and if they are it's most likely a bug which will be seen as StackOverflowError

[He-Pin]

I bring this up for discussion, thanks for your inputs.

[He-Pin]

I have updated this and removed the maxDepth as it can only be a bug when OOM.

[akka-ci]

Test PASSed.

[patriknw]

Isn't this similar to the attempt in #27364 ?
It's replacing the stack with a collection. I'm not sure there are any advantages of doing that.

[He-Pin]

I have forgotten that. I came up with this once I read in detail of the Behavior code. With this change, it will work more concisely, at least it doesn't harm, isn't it? Different deploy environments XSS configs various too.

[patriknw]

I ran a temporary benchmark comparing the two and this is about 30% slower.

BEFORE:

[info] Benchmark                                               (batchSize)    (dispatcher)                                         (mailbox)  (tpt)   Mode  Cnt        Score        Error   Units
[info] TypedActorBenchmark.start                                        50  fjp-dispatcher  akka.dispatch.SingleConsumerOnlyUnboundedMailbox     50  thrpt    6  8048211,084 ± 156294,353   ops/s

AFTER:

[info] Benchmark                                               (batchSize)    (dispatcher)                                         (mailbox)  (tpt)   Mode  Cnt        Score        Error   Units
[info] TypedActorBenchmark.start                                        50  fjp-dispatcher  akka.dispatch.SingleConsumerOnlyUnboundedMailbox     50  thrpt    6  5772894,593 ± 257261,651   ops/s

Used the following in TypedActorBenchmark:

  private val behv: Behavior[Start] =
    Behaviors
      .supervise[Start] {
        Behaviors.setup[Start] { _ =>
          Behaviors.withMdc(Map.empty[String, String]) {
            Behaviors.receiveMessage[Start](_ => Behaviors.same)
          }
        }
      }
      .onFailure[Exception](SupervisorStrategy.restart)

  def startSupervisor(iteration: Int): Behavior[Start] =
    Behaviors.receive { (ctx, msg) =>
      msg match {
        case Start(respondTo) =>
          val startNanoTime = System.nanoTime()

          var i = 0
          while (i < iteration) {
            Behavior.start(behv, ctx)
            i += 1
          }

          respondTo ! Completed(startNanoTime)
          Behaviors.same
      }
    }

  @Benchmark
  @OperationsPerInvocation(iterations)
  def start(): Unit = {
    Await.result(system.ask(Start), timeout)
  }

[He-Pin]

Then it's not worth it.

[akka-ci]

Test PASSed.

[patriknw]

Closing this because I don't think it is an improvement over status quo.

[patriknw]

but thanks anyway

[He-Pin]

The performance is always critical for Akka, let's keep this in mind once we do encounter any problem later.