search

gobuffalo / plush

The powerful template system that Go needs
MIT License
895 stars 57 forks source link

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

Closed Mido-sys closed 1 month ago

Mido-sys commented 1 month ago

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 commented 1 month ago

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

paganotoni commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

paganotoni commented 1 month ago

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

Mido-sys commented 1 month ago

@paganotoni I upgraded the function.

ouamer-dahmani commented 2 weeks ago

@Mido-sys See this comment: https://github.com/kubernetes/kubernetes/issues/124656#issuecomment-2327677824

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