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

Optimize mutable iterations while encoding data #2597

Merged
merged 3 commits into from May 29, 2019

Conversation

@ayushhooda
Copy link
Contributor

commented May 26, 2019

Filename: package.scala
Action Items:

  1. Logic to remove mutable iteration for encoding the hexadecimal data.

@ayushhooda ayushhooda force-pushed the ayushhooda:AbstractMethod branch from 7f65234 to 5b27ef3 May 26, 2019

@ayushhooda ayushhooda force-pushed the ayushhooda:AbstractMethod branch from 5b27ef3 to db31143 May 26, 2019

@ayushhooda ayushhooda changed the title Optimize match case using single abstract method Optimize mutable iterations while encoding and decoding data May 26, 2019

@ayushhooda ayushhooda changed the title Optimize mutable iterations while encoding and decoding data Optimize mutable iterations while encoding data May 26, 2019

@ayushhooda ayushhooda force-pushed the ayushhooda:AbstractMethod branch from 5f9e34b to a139161 May 26, 2019

@rossabaker
Copy link
Member

left a comment

Thanks. Do we have any idea if this is a performance hit? I'd prioritize fast over beautiful for innards like this, but if it's about the same, I of course prefer to eliminate mutation.

@ayushhooda

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Use of tail recursion won't compromise on performance issue but also avoid mutations

@rossabaker
Copy link
Member

left a comment

I added a crude benchmark to convince myself. Would be better if it used different sizes, but I was in a hurry. But it looks like you're right: cleaner and a little faster. Nice work.

Before:

[info] EncodeHexBench.encodeHex  avgt   20  11507.665 ± 145.184  ns/op

After:

[info] EncodeHexBench.encodeHex  avgt   20  10986.248 ± 175.252  ns/op
@ayushhooda

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@rossabaker Thanks for the appreciation. The benchmark results are motivating. But the build seems to be failing by addition of your file.

@rossabaker

This comment has been minimized.

Copy link
Member

commented May 28, 2019

It's one of those intermittent blaze failures that only happens on Travis CI that we haven't gotten to the bottom of yet. 😦

@ayushhooda

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Ohh Now it seems to have passed all checks.

@ayushhooda

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@rossabaker Are we good to go with this patch?

@rossabaker

This comment has been minimized.

Copy link
Member

commented May 29, 2019

We usually wait for a second reviewer to sign off, and then it's ready.

@ayushhooda

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Sounds good. @aeons @cquiroz @diesalbla please have a look.

@cquiroz
Copy link
Member

left a comment

LGTM

@ayushhooda

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Thanks @cquiroz.
I guess, now we're good to go @rossabaker.
What do you suggest?

@rossabaker rossabaker merged commit f437e4a into http4s:master May 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rossabaker

This comment has been minimized.

Copy link
Member

commented May 29, 2019

If you have an urgent need for the extra performance, I can cherry-pick this back to series/0.20. There's also a bugfix PR open, so we'll be doing a maintenance release soon. If it's just nice to have in the long run, it'll be released in the 0.21 series, and can be tried now with 0.21.0-SNAPSHOT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.