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

Use ByteVector#fromHex in CSRF Middleware #7002

Merged
merged 2 commits into from
Feb 26, 2023

Conversation

danicheg
Copy link
Member

An attendant PR to the merged #6984.
As it was previously, the method from scodec needs some performance tweaks.
It'd be lovely if @mpilquist could do a miracle once again 🙏🏻
But, this time the difference isn't that dramatic for real-world usage.

[info] DecodeHexBench.decodeHexHttp4s    1000  thrpt    5  154.654 ± 11.577  ops/ms
[info] DecodeHexBench.decodeHexHttp4s   10000  thrpt    5   13.714 ±  0.116  ops/ms
[info] DecodeHexBench.decodeHexHttp4s  100000  thrpt    5    1.524 ±  0.507  ops/ms
[info] DecodeHexBench.decodeHexScodec    1000  thrpt    5   81.527 ±  4.917  ops/ms
[info] DecodeHexBench.decodeHexScodec   10000  thrpt    5    3.923 ±  0.063  ops/ms
[info] DecodeHexBench.decodeHexScodec  100000  thrpt    5    0.425 ±  0.024  ops/ms

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

I'm just happy to see internal vanishing before my eyes!

Hard to reason about performance here, since Hmac cryptography is involved here, that might be the bottleneck anyway. Not sure tho, maybe that's fast 😇

@mpilquist
Copy link
Contributor

I did some quick benchmarking of scodec's fromHex and it turns out there's a bunch of integer boxing as a result of using a pattern match on a Char. Working on a PR now.

@mpilquist
Copy link
Contributor

Ignore that - the boxing was artifact of the profiler.

@mpilquist
Copy link
Contributor

WIP here: scodec/scodec-bits#422

@danicheg
Copy link
Member Author

scodec/scodec-bits#422 is merged, so we are unlocked here. Steward will bring us a new scodec version when it is published.

@danicheg danicheg merged commit f852eb0 into http4s:series/0.23 Feb 26, 2023
@danicheg danicheg deleted the decode-hex branch February 26, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants