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

Simplify no-copy conversion of []byte to string #587

Closed
wants to merge 1 commit into from

Conversation

YenForYang
Copy link
Contributor

@YenForYang YenForYang commented Sep 10, 2021

This is a pretty well-tested method currently in use by strings.Builder. It's a one-liner so there's no need to worry about the GC interfering. Messing with *reflect.StringHeader is often error-prone and might require a runtime.KeepAlive in some cases. Honestly pleased me to see that this project even attempts to make these kinds of optimizations 😄

This is a pretty well-tested method currently in use by `strings.Builder`. It's a one-liner so there's no need to worry about the GC interfering. Messing with (*reflect.StringHeader) is often error-prone and might require a `runtime.KeepAlive` in some cases.
@anacrolix
Copy link
Owner

I literally just changed this to adhere to the recommendation in the link that is removed. I'm pretty sure the code was correct as I had it, but please reference anything that suggests otherwise.

From memory, the unsafe code here is very performance critical, that's why it's been in for so long.

@YenForYang
Copy link
Contributor Author

YenForYang commented Sep 12, 2021

I've always seen golang/go#25484 as the best reference for this particular conversion -- mainly because it gets opinions from multiple individuals. I tend to trust what the folks over at golang/go do (maybe a bit too much). In this case I it was golang/go#25484 (comment). Here's the strings.Builder part mentioned: https://github.com/golang/go/blob/0d8a4bfc962a606584be0a76ed708f86b44164c7/src/strings/builder.go#L46-L49

golang/go#40701 pretty much supports my reasoning on why reflect.StringHeader should be avoided in this case.

I won't argue if you insist on using StringHeader -- I have way less experience than you in Go at the end of the day. Honestly I'm pretty happy that any of my pulls get in lol.

@anacrolix
Copy link
Owner

Let's revisit this when unsafe.Slice is added, or performance issues come up with the way it currently is. My change was to work around the warning that go vet gave me, and the current code is what was suggested (and it removes the warning).

@anacrolix anacrolix closed this Sep 12, 2021
@YenForYang
Copy link
Contributor Author

Aww but unsafe.Slice is added in Go 1.17. It also can't be used for byte to string conversions -- and for string to byte conversions, it slower than a cast to pointer to array and slicing.

All jokes aside, I guess I can wait for your opinion to change lol. This is actually a really good change -- honestly. I don't see any disadvantages compared to the current method, only advantages. Closing this was a bit hasty.

@anacrolix
Copy link
Owner

anacrolix commented Sep 12, 2021

Maybe I missed something, the commit I refer to is 5cb4702.

@YenForYang
Copy link
Contributor Author

Yeah I saw. To be honest I only made this pull request because I noticed the commit you made. I kinda leapt at the opportunity -- probably should have waited a bit, I know. But I still think closing this request is a bit premature. golang/go#40701 is the issue I think you are talking about.

@anacrolix
Copy link
Owner

Thanks for looking in to this. I'm happy to take simpler code if it's correct, and different code if it's demonstrably faster.

@YenForYang
Copy link
Contributor Author

Great! I'll make a new pull request.

YenForYang added a commit to YenForYang/torrent that referenced this pull request Sep 13, 2021
anacrolix pushed a commit that referenced this pull request Sep 14, 2021
attilabuti pushed a commit to attilabuti/bencode that referenced this pull request Dec 1, 2022
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