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

[Proposal] Use Golang 1.18 for Gitea 1.17 release #19917

Closed
wxiaoguang opened this issue Jun 8, 2022 · 2 comments · Fixed by #19918
Closed

[Proposal] Use Golang 1.18 for Gitea 1.17 release #19917

wxiaoguang opened this issue Jun 8, 2022 · 2 comments · Fixed by #19918
Labels
topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/proposal The new feature has not been accepted yet but needs to be discussed first.
Milestone

Comments

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 8, 2022

Use Golang 1.18 (as minimal requirement) for Gitea 1.17 release, make sure the Golang version is still actively supported during Gitea 1.17 lifecycle.

And Gitea can start using Golang generics from then on.

@wxiaoguang wxiaoguang added type/proposal The new feature has not been accepted yet but needs to be discussed first. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile labels Jun 8, 2022
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Jun 8, 2022
@zeripath
Copy link
Contributor

zeripath commented Jun 8, 2022

Not sure I think that we should immediately jump on to Generics before releasing 1.17 but I agree with making Gitea 1.17 use Golang 1.18.


One thing against moving to generics already is that a lot of the std library remains ungenercised and certainly most of our downstream libraries. This situation will improve over time and whilst I know someone has to start, the wrong steps now could lead to annoying refactors later.

Further:

https://planetscale.com/blog/generics-can-make-your-go-code-slower

Whilst Gitea code is frankly far from optimised enough to make most of these performance losses a serious problem, this comment is worth noticing as it would be relevant to a lot of string manipulations:

The results are not surprising. The function that has been specialized to take a *strings.Builder directly is the fastest, because it allowed the compiler to inline the WriteByte calls inside of it. The generic function is measurably slower than the simplest possible implementation taking an io.ByteWriter interface as an argument. We can see that the impact of the extra load from the generic dictionary is not significant, because both the itab and the Generics dictionary will be very warm in cache in this micro-benchmark (however, do keep reading for an analysis of how cache contention affects Generic code).

This is the first insight we can gather from this analysis: there’s no incentive to convert a pure function that takes an interface to use Generics in 1.18. It’ll only make it slower, as the Go compiler cannot currently generate a function shape where methods are called through a pointer. Instead, it will introduce an interface call with two layers of indirection. This is going in the exact opposite direction of what we’d like, which is de-virtualization and, where possible, inlining.

Although genericising the WriteByte/WriteString to WriteByteSeq() would likely be beneficial.

The Summary here is enlightening:

This was a lot of fun! I hope you had a lot of fun too looking at these assemblies with me. Let’s finish this post with a short list of DOs and DON’Ts when it comes to performance and Generics in Go 1.18:

  • DO try to de-duplicate identical methods that take a string and a []byte using a ByteSeq constraint. The generated shape instantiation is very close to manually writing two almost-identical functions.
  • DO use generics in data structures. This is by far their best use case: Generic data structures that were previously implemented using interface{} are complex and un-ergonomic. Removing the type assertions and storing types unboxed in a type-safe way makes these data structures both easier to use and more performant.
  • DO attempt to parametrize functional helpers by their callback types. In some cases, it may allow the Go compiler to flatten them.
  • DO NOT attempt to use Generics to de-virtualize or inline method calls. It doesn’t work because there’s a single shape for all pointer types that can be passed to the generic function; the associated method information lives in a runtime dictionary.
  • DO NOT pass an interface to a generic function, under any circumstances. Because of the way shape instantiation works for interfaces, instead of de-virtualizing, you’re adding another virtualization layer that involves a global hash table lookup for every method call. When dealing with Generics in a performance-sensitive context, use only pointers instead of interfaces.
  • DO NOT rewrite interface-based APIs to use Generics. Given the current constraints of the implementation, any code that currently uses non-empty interfaces will behave more predictably, and will be simpler, if it continues using interfaces. When it comes to method calls, Generics devolve pointers into twice-indirect interfaces, and interfaces into… well, something quite horrifying, if I’m being honest.
  • DO NOT despair and/or weep profusely, as there is no technical limitation in the language design for Go Generics that prevents an (eventual) implementation that uses monomorphization more aggressively to inline or de-virtualize method calls.

I do think that the go runtime and compiler will likely have further improvements in 1.19 etc with regards to how it handles generics internally. As would the std library and many of our upstream libraries.


Do you have a serious place where generics would be appropriate?


I think instead we should be looking at what's blocking release of 1.17-RC1 - and if there's nothing that we think is really missing or we need in 1.17 we should just cut it and then we can get started on 1.18, allow generics and do more major refactorings.

If we make Gitea 1.17 only support go 1.18 then if there are generics changes that need to be backported we can do so - but I think adding generics at this point is too late. It's just going to further delay 1.17 - which we need to get ready for release.

@wxiaoguang
Copy link
Contributor Author

Do you have a serious place where generics would be appropriate?

Nope at the moment.


Two things here:

  1. Make Gitea use Golang 1.18 for next release
  2. Make Gitea start using Golang Generics

For question 1, I think it's reasonable because Golang's release cycle is very fast, a few month later, Golang 1.19 comes and Golang 1.17 will be unsupported. So, making Gitea use an active Golang 1.18 should be good.

For question 2, I think we can wait and see, some code could benefit from it, for example, container.KeysInt64, maybe more. I also agree that it's not a must, just fine to have. So the decision doesn't need to be made too soon.

@wxiaoguang wxiaoguang linked a pull request Jun 8, 2022 that will close this issue
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants