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.

Sign up for GitHub

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

Jump to bottom

Reverted: use optimised ByteString.asInputStream #539

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Reverted: use optimised ByteString.asInputStream #539

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d8a133
use optimised ByteString.asInputStream
pjfanning Apr 19, 2024
253e3d8
optimise ByteStringInputStream.scala
pjfanning Apr 23, 2024
58240a2
make byteStringInputStreamMethodTypeOpt a val
pjfanning Apr 23, 2024
b1c606b
refactor
pjfanning Apr 23, 2024
02fea87
avoid getOrElse
pjfanning Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions .../src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/ByteStringInputStream.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
package org.apache.pekko.http.impl.engine.http2.hpack

import java.io.{ ByteArrayInputStream, InputStream }
import java.lang.invoke.{ MethodHandles, MethodType }

import scala.util.Try

import org.apache.pekko
import pekko.annotation.InternalApi
Expand All @@ -24,13 +27,32 @@ import pekko.util.ByteString.ByteString1C
@InternalApi
private[http2] object ByteStringInputStream {

def apply(bs: ByteString): InputStream =
bs match {
case cs: ByteString1C =>
// TODO optimise, ByteString needs to expose InputStream (esp if array backed, nice!)
new ByteArrayInputStream(cs.toArrayUnsafe())
case _ =>
// NOTE: We actually measured recently, and compact + use array was pretty good usually
apply(bs.compact)
}
private val byteStringInputStreamMethodTypeOpt = Try {
val lookup = MethodHandles.publicLookup()
val inputStreamMethodType = MethodType.methodType(classOf[InputStream])
lookup.findVirtual(classOf[ByteString], "asInputStream", inputStreamMethodType)
}.toOption
Copy link
Member

Choose a reason for hiding this comment

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

  1. I think we should drop the lazy and just make it a val.
  2. the toOption will introduce an allocation, use OptionVal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code is only called once in the whole lifetime of the JVM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be lazy because this code may need to run unless it is needed

Copy link
Member

Choose a reason for hiding this comment

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

But the lazy will introduce a volatile iirc, I think this is a trade-off, just not sure if that will cause the problem in the native image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OptionVal does not support changing a Try[A] to an Option[A]. So OptionVal does not look a viable approach here.

Copy link
Member

Choose a reason for hiding this comment

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

The getorelse will introduce another closure creation, think we can just fallback to imperative style for performance reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that the getOrElse has a small cost so I made a change to remove it


def apply(bs: ByteString): InputStream = bs match {
case cs: ByteString1C =>
getInputStreamUnsafe(cs)
case _ =>
if (byteStringInputStreamMethodTypeOpt.isDefined) {
byteStringInputStreamMethodTypeOpt.get.invoke(bs).asInstanceOf[InputStream]
} else {
legacyConvert(bs.compact)
}
}

private def legacyConvert(bs: ByteString): InputStream = bs match {
case cs: ByteString1C =>
getInputStreamUnsafe(cs)
case _ =>
// NOTE: We actually measured recently, and compact + use array was pretty good usually
legacyConvert(bs.compact)
}

private def getInputStreamUnsafe(bs: ByteString): InputStream =
new ByteArrayInputStream(bs.toArrayUnsafe())

}
Loading