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

skylark: don't access runtime.stringHash #16

Open
alandonovan opened this issue Oct 22, 2018 · 4 comments
Open

skylark: don't access runtime.stringHash #16

alandonovan opened this issue Oct 22, 2018 · 4 comments

Comments

@alandonovan
Copy link
Contributor

Russ Cox points out that CL google/skylark@f94b021 makes Skylark depend on the internals of one particular Go implementation. In its defense, stringHash is one of the most stable runtime functions, its disappearance would be immediately detected by build/test using a new Go release, its assembly implementation is not a trivial piece of code, and a pure-software workaround is readily available. That all said, it does cut off use of skylark from GopherJS, for example, and possibly also gccgo.

The task of this issue is to make this dependency cleaner, either by duplicating the AESENC assembly from the Go runtime, or by build-tagging the non-portable code so that the runtime internals are only used when known to be available.

See also golang/go#28322

@alandonovan alandonovan transferred this issue from google/skylark Nov 21, 2018
@AlekSi
Copy link
Contributor

AlekSi commented Feb 1, 2020

hash/maphash landed in tip: https://tip.golang.org/pkg/hash/maphash/

@alandonovan
Copy link
Contributor Author

Thanks. We could start using it in a tagged file, but we can't stop using the old hacky mechanism until two releases have passed.

@alandonovan
Copy link
Contributor Author

We can't fix this yet. The overheads of the Go 1.14's hash/maphash package make it slower than even a software implementation for strings up to 128 bytes, and even for 1024 byte strings it is 6x slower than the Go runtime's private implementation. See golang/go#42710 (comment)

@zikaeroh
Copy link

zikaeroh commented Jan 20, 2021

Also note that due to golang/go#40067, using hash/maphash in an existing/required package (even behind a build tag) means that no user can use this one without upgrading to a version of Go with hash/maphash (or face the wrath of go mod tidy never working). Perhaps that's not such a big issue now that every "supported" version of Go contains this package, but this module does declare 1.13+ (and I bet someone's using it like that).

EDIT: golang/go#44557 is now fixed and backported to 1.16 and 1.15, so the dependency can be added once patch adoption is reasonable.

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

No branches or pull requests

3 participants