Performance regression in 0.23.12

[bastewart]

We suspect there is a performance regression in v0.23.12. This was initially noticed by updating http4s-jetty to 0.23.12 which brings servlet 0.23.13 in transitively.

We've observed a ~15-20% performance hit in a production service (increased CPU to handle the same load). Sorry, this is both pretty unscientific in benchmarking terms and we haven't yet isolated the cause (an exact commit). We are almost entirely certain on the release though.

We've isolated this to the 0.23.12 release of this library by manually bumping the servlet version (and keeping all other dependencies constant). 0.23.11 appears fine, 0.23.12 and 0.23.13 show the reduced performance.

In addition it does not appear that http4s-jetty (or jetty itself) are the cause. Running http4s-jetty 0.23.12 alongside servlet 0.23.13 shows the same performance as http4s-jetty 0.23.11 and servlet 0.23.13.

@TimWSpence is currently looking to isolate the commit!

[rossabaker]

It would make me very happy to find out it's not #50, but I'd look there first.

[bastewart]

That's where our suspicion fell internal as well! 😬

[rossabaker]

Make sure it's cats-effect-3.4. Transitively, it should be.

We could play with the type of queue. I didn't rigorously performance test, and apparently should have. I think this implementation is much nicer, so I'd hate to have to revert, but we could if we can't fix this.

[armanbilge]

I was chatting with @TimWSpence about this, their current suspicion is that a large number of fiber allocations are at play.

If you are using a "parallel" Dispatcher, then each of these unsafeRunAndForget will start a new Fiber.

http4s-servlet/servlet/src/main/scala/org/http4s/servlet/ServletIo.scala

Lines 133 to 142 in cdff010

	def onAllDataRead(): Unit =
	unsafeRunAndForget(q.offer(End))
	def onError(t: Throwable): Unit =
	unsafeRunAndForget(q.offer(Error(t)))
	def unsafeRunAndForget[A](fa: F[A]): Unit =
	dispatcher.unsafeRunAndForget(
	fa.onError { case t => F.delay(logger.error(t)("Error in servlet read listener")) }
	)

However, if you create a "sequential" Dispatcher, then all those effects will be run sequentially on the same fiber. In this case you would want the Dispatcher to be local to the request.

[bastewart]

Also just going to confirm here (for the record) that @TimWSpence did isolate this to #50. He confirmed this on discord though, hence no message here. Everyone in this thread is aware (I think!) though.

[rossabaker]

A request-scoped, sequential dispatcher sounds reasonable to me. I took a quick look and it's going to be a moderate pain to fix in a binary compatible fashion, but I think it can be done.

[rossabaker]

It's late and I'm getting stupid, but, uh, who dispatches the dispatcher?

  override def service(
      servletRequest: HttpServletRequest,
      servletResponse: HttpServletResponse,
  ): Unit =
    Dispatcher.sequential[F].use { dispatcher =>
       ???
    }

I think the servlet needs a parallel one, and each request can get a local, sequential one?

[armanbilge]

Heh, that sounds about right. Each request would get its own fiber (from the parallel dispatcher) on which it starts a single-fiber sequential dispatcher.

[TimWSpence]

It would make me very happy to find out it's not #50, but I'd look there first.

Sorry Ross 😛 I just opened #171. Hope both the description and the changes make sense!