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

Adopt go 1.18 Generics #36308

Closed
howardjohn opened this issue Nov 30, 2021 · 8 comments
Closed

Adopt go 1.18 Generics #36308

howardjohn opened this issue Nov 30, 2021 · 8 comments
Labels
area/test and release lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed

Comments

@howardjohn
Copy link
Member

Istio T&R has made an intentional decision to not merge any PRs adding generics until at least 3 months after Go 1.18's release, to reduce churn and instability. Everything discussed below is future looking, after this date has passed.

Issues:
TODO

Areas of interest:

  • client-go
  • config store?
  • Delta + SotW sharing code?
@ericvn
Copy link
Contributor

ericvn commented Dec 9, 2021

Blog (https://go.dev/blog/12years):

Generics will be one of our focuses for 2022. The initial release in Go 1.18 is only the beginning.
We need to spend time using generics and learning what works and what doesn’t, so that we can
write best practices and decide what should be added to the standard library and other libraries.
We expect that Go 1.19 (expected in August 2022) and later releases will further refine the 
design and implementation of generics as well as integrating them further into the overall Go
experience.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Mar 10, 2022
@howardjohn howardjohn added lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed and removed lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while labels Mar 10, 2022
@howardjohn
Copy link
Member Author

howardjohn commented Jul 20, 2022

Proposal:

Allow generics in test only code, with production code still not using generics until at least 1.19.

Rationale:

  • Go 1.19 improves generic performance and comes out pretty soon anyways
  • Test code often has a lot of boilerplate, so generics can help here quite a bit

Thoughts? @istio/wg-test-and-release-maintainers

@ldemailly
Copy link
Contributor

I got pretty great reduction in duplicate code using generics in actual code in fortio
fortio/fortio@588306d...1d4dbd8

I left the pre existing tests alone for now actually because I wanted to test that the generic version worked exactly the same

Curious why intend just the tests for now? concern about performance in main code? or that it'll improve/change in 1.19 enough to cause churn?

@sschepens
Copy link
Contributor

I wouldn't probably introduce generics in performance critical paths unless we really test performance or just wait for a few more releases of golang.

This article uncovers a lot of pains with current implementation of genetics in galang. https://planetscale.com/blog/generics-can-make-your-go-code-slower

@ericvn
Copy link
Contributor

ericvn commented Jul 21, 2022

I agree with the slow adoption, test now, starting some in production with 1.19. I'm not sure if there is some verbiage to add with respect to performance critical paths, but I think the community understands the underlying issues with not impacting performance.

@howardjohn
Copy link
Member Author

Curious why intend just the tests for now? concern about performance in main code? or that it'll improve/change in 1.19 enough to cause churn?

It may be a bit conservative, but mostly for hypothetical performance issues, churn is a big one as well. There are also all the unknown-unknowns. Tests only gives us a good way to dip our toes in and see how it goes. 1.19 is only like a month away anyways

@jacob-delgado
Copy link
Contributor

jacob-delgado commented Jul 21, 2022

For 1.15 allowing it in test code is fine. For 1.16 we could start allowing it in. That way if anyone wishes to start working on a change they could have it ready by shortly after the branch cut off happens in mid-August.

It's also fine to say that it's ok for changes in the control plane, but are limited to certain packages for the time being (inclusive or exclusive is fine with me).

@nmittler
Copy link
Contributor

+1

Honestly, I'm not even sure why this would be up for debate. Just do it, since it's only test code. Obviously any decision for production code would need to be scrutiny.

howardjohn added a commit to howardjohn/istio that referenced this issue Jul 22, 2022
istio-testing pushed a commit that referenced this issue Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test and release lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed
Projects
None yet
Development

No branches or pull requests

7 participants