Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small refactor to DefaultClient to remove unsafe calls and repeated code #7417

Merged

Conversation

Adam-McDevitt
Copy link
Contributor

Saw some unsafe calls:

  • NonEmptyList.fromListUnsafe
  • m.head where m is a List, not a NonEmptyList

that could be easily refactored to a safe call, and an opportunity to move some repeated code into a private function.

@mergify mergify bot added series/0.23 PRs targeting 0.23.x module:client labels Mar 28, 2024
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Thanks!

@@ -88,14 +87,13 @@ private[http4s] abstract class DefaultClient[F[_]](implicit F: MonadCancelThrow[
def streaming[A](req: F[Request[F]])(f: Response[F] => Stream[F, A]): Stream[F, A] =
Stream.eval(req).flatMap(stream).flatMap(f)

private def reqWithMediaRangeAndQValue[A](req: Request[F], d: EntityDecoder[F, A]) =
d.consumes.toList.toNel.fold(req)(m => req.addHeader(Accept(m.map(MediaRangeAndQValue(_)))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a few femtoseconds slower than the original, but since we're about to make an HTTP request, it is probably completely imperceptible. And I like getting rid of the unsafe methods. And if someone wants to reoptimize this, now it's in one place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.toList.toNel

In the capacity of a grumpy micro-benchmarker of certain parts of http4s, this will also consume a bit more heap and stress out the GC (in terms of femtoseconds). Unsafe API here is a reasonable choice since we have checks on non-emptiness.

But, I really like this refactoring into the dedicated def 👍🏻

Copy link
Contributor Author

@Adam-McDevitt Adam-McDevitt Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I like getting rid of the unsafe methods. And if someone wants to reoptimize this, now it's in one place.

Unsafe API here is a reasonable choice since we have checks on non-emptiness.

I'm happy to rework to go with whatever maintainer preference is here. I think the .toNel.fold implementation nicer and more pleasing, but if the performance decrease is deemed important I could push up changes to have the old implementation pulled out into the same helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part is it that's expected to add a bit more heap / GC / execution time? Would this be an improvement?

  private def reqWithMediaRangeAndQValue[A](req: Request[F], d: EntityDecoder[F, A]) =
    d.consumes.toList match {
      case head :: next =>
        req.addHeader(Accept(NonEmptyList(head, next).map(MediaRangeAndQValue(_))))
      case Nil => req
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part is it that's expected to add a bit more heap / GC / execution time? Would this be an improvement?

  private def reqWithMediaRangeAndQValue[A](req: Request[F], d: EntityDecoder[F, A]) =
    d.consumes.toList match {
      case head :: next =>
        req.addHeader(Accept(NonEmptyList(head, next).map(MediaRangeAndQValue(_))))
      case Nil => req
    }

I think this would be an improvement. It avoids allocating the extra Option that using toNel had.
The original implementations avoiding calling toList if the Set was empty, but doing that is just going to return Nil if the set is empty anyways. It's fine.

@Adam-McDevitt, if you're still willing, yes I think your suggestion here would be an improvement.

@danicheg
Copy link
Member

danicheg commented Apr 6, 2024

Since Ross approved this, it should be enough to move on. I don't really want to be a party pooper. But maybe some third folk will glance at this to have a quorum in opinions.

Copy link
Member

@valencik valencik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @Adam-McDevitt, it's a nice clean up.

I do think your latest suggestion avoiding toNel is nice and hope you are able to push a commit with that change. 😺

Approving to not block.

@@ -88,14 +87,13 @@ private[http4s] abstract class DefaultClient[F[_]](implicit F: MonadCancelThrow[
def streaming[A](req: F[Request[F]])(f: Response[F] => Stream[F, A]): Stream[F, A] =
Stream.eval(req).flatMap(stream).flatMap(f)

private def reqWithMediaRangeAndQValue[A](req: Request[F], d: EntityDecoder[F, A]) =
d.consumes.toList.toNel.fold(req)(m => req.addHeader(Accept(m.map(MediaRangeAndQValue(_)))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part is it that's expected to add a bit more heap / GC / execution time? Would this be an improvement?

  private def reqWithMediaRangeAndQValue[A](req: Request[F], d: EntityDecoder[F, A]) =
    d.consumes.toList match {
      case head :: next =>
        req.addHeader(Accept(NonEmptyList(head, next).map(MediaRangeAndQValue(_))))
      case Nil => req
    }

I think this would be an improvement. It avoids allocating the extra Option that using toNel had.
The original implementations avoiding calling toList if the Set was empty, but doing that is just going to return Nil if the set is empty anyways. It's fine.

@Adam-McDevitt, if you're still willing, yes I think your suggestion here would be an improvement.

@Adam-McDevitt
Copy link
Contributor Author

Adam-McDevitt commented Apr 11, 2024

Thanks all - I'll push up a change probably today with my latest suggestion.

@Adam-McDevitt
Copy link
Contributor Author

Updated! :)

Copy link
Contributor

@hamnis hamnis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a sbt scalafixAll

@danicheg
Copy link
Member

Thank you for iterating over this!

@danicheg danicheg merged commit 74edf0f into http4s:series/0.23 Apr 12, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:client series/0.23 PRs targeting 0.23.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants