-
Notifications
You must be signed in to change notification settings - Fork 40
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
Improve Hash#hex performance. #15
Conversation
Thank you for the contribution! I'll try to take a look at the code this weekend. Before I do, though, one question: Was this a hotspot you found in your app, or were you benchmarking the lib in isolation when you found this? |
I found this issue when I was stress testing our web application that generates hash(output for hex) per request. At that time I noticed Hash#hex method(uses String#format derived from Regex) consumes a lot of cpu time. |
@@ -27,8 +27,17 @@ case class Hash ( val bytes: Array[Byte] ) extends Equals { | |||
def this ( hex: String ) = | |||
this( hex.grouped(2).map( Integer.parseInt(_, 16).byteValue ).toArray ) | |||
|
|||
private[this] val HexChars = "0123456789abcdef".toCharArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a class property. It belongs up on the Hash
object above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply review result.
8ff1055
/** Converts this hash to a hex encoded string */ | ||
lazy val hex: String = bytes.map( "%02x".format(_) ).mkString("") | ||
lazy val hex: String = { | ||
val buffer = new StringBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to initialize this with a specific length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply review result.
8ff1055
bytes.foreach { byte => | ||
buffer.append(HexChars((byte & 0xF0) >> 4)) | ||
buffer.append(HexChars(byte & 0x0F)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested to see if this function is faster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see below comment.
#15 (comment)
Thank you for taking your time for the review. I fixed my codes.
Here is another benchmark result. My implementation is 1.5 times faster. My implementation #2(review has been applied)
DatatypeConverter version. Implementation is below.
and a benchmark result.
|
Hello.
Current implementation of Hash#hex is very slow.
I improved that performance.
Here is a jmh benchmark results.
Before fix.
After fix.