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

runtime: remove github.com/cloudwego/frugal from linkname comment #69222

Closed
xiaost opened this issue Sep 3, 2024 · 8 comments
Closed

runtime: remove github.com/cloudwego/frugal from linkname comment #69222

xiaost opened this issue Sep 3, 2024 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@xiaost
Copy link
Contributor

xiaost commented Sep 3, 2024

Dear Go Team,

I'm one of the maintainers of github.com/cloudwego/frugal

Recently, we have become aware of issues related to the misuse of linknames in the Go runtime (#67401).
Previously frugal is a JIT solution for decoding apache thrift protocol which widely used in our company,
but it involved binding to various components of the Go runtime, and it has made the project difficult to maintain as Go continues to evolve.

To address this, we have implemented a new reflection-based version of frugal that no longer relies on numerous runtime functions and structures. We would greatly appreciate it if Go could consider removing frugal from the "hall of shame" comment in the runtime package.

Starting from Go 1.24, we will fully deprecate the JIT implementation,
and we have already added build tag to facilitate this transition.

However, we regret to inform you that we still need to rely on runtime.mallocgc for memory allocation when decoding different types. This is the last remaining linkname dependency in frugal.
There are two purposes of using runtime.mallocgc:

  • copy after runtime.mallocgc with needzero=false for better performance.
  • alternative to reflect.MakeSlice or reflect.New when handling unsafe.Pointer

We understand that relying on any linkname function is not an ideal long-term solution,
and we are committed to adopting a better and more compatible approach provided by Go in the future.


attached with the full list that can be removed.

src/runtime/symtab.go:470://   - github.com/cloudwego/frugal
src/runtime/symtab.go:589://   - github.com/cloudwego/frugal
src/runtime/symtab.go:865://   - github.com/cloudwego/frugal
src/runtime/symtab.go:1203://   - github.com/cloudwego/frugal
src/runtime/symtab.go:1241://   - github.com/cloudwego/frugal
src/runtime/symtab.go:1295://   - github.com/cloudwego/frugal
src/runtime/map_fast32_noswiss.go:108://   - github.com/cloudwego/frugal
src/runtime/alg.go:100://   - github.com/cloudwego/frugal
src/runtime/map_faststr_noswiss.go:218://   - github.com/cloudwego/frugal
src/runtime/type.go:113://   - github.com/cloudwego/frugal
src/runtime/type.go:156://   - github.com/cloudwego/frugal
src/runtime/map_noswiss.go:315://   - github.com/cloudwego/frugal
src/runtime/map_noswiss.go:610://   - github.com/cloudwego/frugal
src/runtime/map_noswiss.go:870://   - github.com/cloudwego/frugal
src/runtime/map_noswiss.go:931://   - github.com/cloudwego/frugal
src/runtime/map_noswiss.go:1073://   - github.com/cloudwego/frugal
src/runtime/stubs.go:100://   - github.com/cloudwego/frugal
src/runtime/stubs.go:135://   - github.com/cloudwego/frugal
src/runtime/stubs.go:371://   - github.com/cloudwego/frugal
src/runtime/stubs.go:468://   - github.com/cloudwego/frugal
src/runtime/map_fast64_noswiss.go:108://   - github.com/cloudwego/frugal
src/runtime/map_fast64_noswiss.go:209://   - github.com/cloudwego/frugal
src/runtime/runtime1.go:683://   - github.com/cloudwego/frugal
src/runtime/mgc.go:223://   - github.com/cloudwego/frugal
src/runtime/string.go:85://   - github.com/cloudwego/frugal
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 3, 2024
@gophun
Copy link

gophun commented Sep 3, 2024

However, the code of people using one of the older versions of frugal must be kept running. Therefore, it doesn’t matter if newer versions no longer use the linknames.

@seankhliao
Copy link
Member

Not necessarily, all the dependencies that didn't make the cut to be special cased cannot be built with the most recent release. This won't be any different, except there's an upgrade path that will resolve the issue.

@xiaost
Copy link
Contributor Author

xiaost commented Sep 3, 2024

the code of people using one of the older versions of frugal must be kept running

@gophun Yes, but people who using the old versions can only compile with the old Go version which may already deprecated by the Go team, and users should not report any issues to Go or frugal team when they're using a legacy Go version.
This case also exists in github.com/phuslu/log and github.com/opencontainers/runc which removed from the linkname list recently.

@seankhliao not sure if I understand what you mean.
I was referring to the fact that we used to have dependencies on linkname, but now we don't
(the deadline to our users is go1.24, see frugal_jit_go124.go)
Therefore, I shared here that can remove the related comments. I can raise a CR if needed.

@gophun
Copy link

gophun commented Sep 3, 2024

@xiaost

Yes, but people who using the old versions can only compile with the old Go version

Why? You can expect old programs with old dependency versions to compile and run with the latest Go version.

@gophun
Copy link

gophun commented Sep 3, 2024

Not necessarily, all the dependencies that didn't make the cut to be special cased cannot be built with the most recent release.

Yes, but frugal made the cut to be special cased, because too many projects depend on the problematic versions directly or indirectly. These projects can't be expected to upgrade all of a sudden just because there is a new version of frugal. The special cases shouldn't be removed unless there is evidence that usage of old frugal versions has decreased enough to ensure that the Go ecosystem won't break significantly with a current Go version.

@xiaost
Copy link
Contributor Author

xiaost commented Sep 3, 2024

@gophun Why? You can expect old programs with old dependency versions to compile and run with the latest Go version.

Coz our package can only works with latest Go version after we support it. like this change. It's very tricky, I know 😟 .

@ianlancetaylor
Copy link
Contributor

@xiaost Thanks for working to remove the linkname references. I would encourage you to send a patch to the main Go repo removing frugal from the lists it should no longer be on. See https://go.dev/doc/contribute. Thanks.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/609918 mentions this issue: runtime: remove cloudwego/frugal unused linkname from comment

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. labels Sep 4, 2024
@dmitshur dmitshur added this to the Go1.24 milestone Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests

6 participants