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

Decode: use unsafe []byte->string conversion for discarded value #4

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

odeke-em
Copy link
Contributor

For Decode's MLI4I, we invoke strconv.Atoi after string(*b)
which incurs a []byte->string allocation. The converted []byte->string
is discarded so anyways it is okay to use unsafe, as the string is
never used again. You can see the results at https://dashboard.github.orijtech.com/benchmark/dbb0fca65f644d95bcf8d0c8caafc601

Results:

  • time/op (ns/op)
    Encoding/Decoding_2I-8 7.5ns ± 1% 6.7ns ± 0% -10.36% (p=0.000 n=10+9)
    Encoding/Decoding_4E-8 8.8ns ± 0% 7.9ns ± 0% -10.35% (p=0.000 n=10+10)
    Encoding/Decoding_A4E-8 38ns ± 1% 17ns ± 0% -54.84% (p=0.000 n=10+10)

  • allocs/op (B/op)
    Encoding/Decoding_A4E-8 4.0B ± 0% 0 -100.00% (p=0.000 n=10+10)

  • allocs/op (N/op)
    Encoding/Decoding_A4E-8 1.0 ± 0% 0 -100.00% (p=0.000 n=10+10)

Or visually
Screen Shot 2021-09-25 at 12 58 38 AM

Problem Statement

What is the current behavior? Why and how does it need to change?

string(*b) incurs a []byte->string allocation yet the value will be discarded.

Description of Change

Please include a summary of the change and, if applicable, tag related issues, add code snippets or logs.

This change removes a []byte->string allocation

Breaking Change

Is this a breaking change?

No.

Caveats

Please list any caveats or special considerations for this change.

It uses unsafe, but the usage is isolated as we directly insert the converted string into strconv.Atoi, so this isn't a caveat really. /cc @kirbyquerby @cuonglm @madflojo

For Decode's MLI4I, we invoke strconv.Atoi after string(*b)
which incurs a []byte->string allocation. The converted []byte->string
is discarded so anyways it is okay to use unsafe, as the string is
never used again.

Results:

* time/op (ns/op)
Encoding/Decoding_2I-8	7.5ns ± 1%  6.7ns ± 0%  -10.36%	(p=0.000 n=10+9)
Encoding/Decoding_4E-8	8.8ns ± 0%  7.9ns ± 0%  -10.35%	(p=0.000 n=10+10)
Encoding/Decoding_A4E-8	38ns ± 1%   17ns ± 0%   -54.84%	(p=0.000 n=10+10)

* allocs/op (B/op)
Encoding/Decoding_A4E-8	4.0B ± 0%   0	-100.00% (p=0.000 n=10+10)

* allocs/op (N/op)
Encoding/Decoding_A4E-8	1.0 ± 0%    0	-100.00% (p=0.000 n=10+10)
@madflojo
Copy link
Member

madflojo commented Oct 2, 2021

@odeke-em thanks for opening this, sorry for the delay in comments. The change looks pretty clear and I see what you mean on the use of unsafe being pretty isolated.

I’ve not explored unsafe very much so let me do a bit of research on any downsides of using unsafe.

Thanks again for opening this.

@odeke-em
Copy link
Contributor Author

odeke-em commented Oct 2, 2021

@odeke-em thanks for opening this, sorry for the delay in comments. The change looks pretty clear and I see what you mean on the use of unsafe being pretty isolated.

I’ve not explored unsafe very much so let me do a bit of research on any downsides of using unsafe.

Thanks again for opening this.

No rush at all @madflojo, please take your time! I put it up so you can indeed explore trade-offs and benefits.

@madflojo
Copy link
Member

madflojo commented Dec 2, 2021

After going through it, I think I'm good with this change. Thanks @odeke-em!

@madflojo madflojo merged commit 6499601 into americanexpress:main Dec 2, 2021
@odeke-em odeke-em deleted the unsafe-conversions branch December 3, 2021 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants