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

bug: Avoid the overhead cost of WriteString in write function in compiler.go #188

Closed
Mido-sys opened this issue Aug 6, 2024 · 9 comments

Comments

@Mido-sys
Copy link
Contributor

Mido-sys commented Aug 6, 2024

Description

The WriteString() causes an overhead as the underlying bytes change. We know that the string value for each WriteString line of code won't change so we can include use unsafe. pointer which will result in zero allocation.

To Reproduce

No response

Additional Context

Details

Paste the output of `buffalo info` here!
@Mido-sys
Copy link
Contributor Author

Mido-sys commented Aug 6, 2024

Hello @paganotoni , I pushed a fix for the above.

@paganotoni
Copy link
Member

paganotoni commented Aug 6, 2024

Thanks @Mido-sys, Is there a way to benchmark this one? I would love to understand the impact of it better. A test (and/or benchmark) would be appreciated.

@Mido-sys
Copy link
Contributor Author

Mido-sys commented Aug 6, 2024

Before the fix, this was result from pprof
Screenshot from 2024-08-06 06-08-40

After applying the fix, plushRender didn't appear

@Mido-sys
Copy link
Contributor Author

Mido-sys commented Aug 6, 2024

Also, due to go.mod is 1.13 we couldn't use the recommended way of using unsafe which was introduced in 1.17
unsafe.Slice(unsafe.StringData(s), len(s))

So I got the function from here https://stackoverflow.com/a/59210739. The original author of this function is Ian Lance Taylor

@paganotoni
Copy link
Member

ok. Good. Its time to cut a v5 for plush, with some breaking changes. one of those is reduce the dependencies that the helpers bring in. Once we do that we can change it to the recommended way of doing it. Thanks @Mido-sys !

@Mido-sys
Copy link
Contributor Author

Mido-sys commented Aug 6, 2024

Also with Go1.22 we can use this []byte("STRING") with zero overhead cost.

@paganotoni
Copy link
Member

@Mido-sys I just tagged v5 with Go 1.21.

@Mido-sys
Copy link
Contributor Author

Mido-sys commented Aug 6, 2024

@paganotoni I upgraded the function.

@ouamer-dahmani
Copy link

@Mido-sys
See this comment: kubernetes/kubernetes#124656 (comment)

String-to-byte conversion should still trigger allocations. I believe you are victim of a bad benchmark.

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

No branches or pull requests

3 participants