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

Fix s2b #1079

Merged
merged 1 commit into from
Aug 26, 2021
Merged

Fix s2b #1079

merged 1 commit into from
Aug 26, 2021

Conversation

YenForYang
Copy link
Contributor

Ensure len(b) <= cap(b) at all times. Additionally, use runtime.KeepAlive() to prevent s from being GC-ed.

A likely better solution (which requires Go 1.17) would use:

b = unsafe.Slice((*byte)(unsafe.Pointer((*reflect.StringHeader)(unsafe.Pointer(&s)).Data)), len(s))

Ensure `len(b)` <= `cap(b)` at all times. Additionally, use `runtime.KeepAlive()` to prevent `s` from being GC-ed.
@erikdubbelboer erikdubbelboer merged commit c7ce95f into valyala:master Aug 26, 2021
@erikdubbelboer
Copy link
Collaborator

I'm not completely sure if runtime.KeepAlive is really required as s is an argument so it should be kept alive by the caller right?

But it can't hurt either since it's a no-op (only does something compile time).

The len(b) <= cap(b) might not do anything either right now either, does the garbage collector look at this? But it is more correct and might prevent some future bugs.

I'm curious how you came up with these fixes?

@zhangyunhao116
Copy link
Contributor

AFAIK, the GC will recognize the unsafe.Pointer instead of other steps. For example if copy stack happens, the runtime will move the slice to a new stack, because it see the unsafe.Pointer. Looks like the runtime.KeepAlive is meaningless, because this function return immediately.

from doc:
Note: KeepAlive should only be used to prevent finalizers from running prematurely. In particular, when used with unsafe.Pointer, the rules for valid uses of unsafe.Pointer still apply.

ref: https://pkg.go.dev/runtime#KeepAlive

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.

3 participants