search

google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.32k stars 212 forks source link

skylark: don't access runtime.stringHash #16

Open alandonovan opened 5 years ago

alandonovan commented 5 years ago

Russ Cox points out that CL https://github.com/google/skylark/commit/f94b021a287bdc82ccd4d1dbbd1cbf0c174161c9 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 https://github.com/golang/go/issues/28322

AlekSi commented 4 years ago

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

alandonovan commented 4 years ago

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 commented 3 years ago

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 https://github.com/golang/go/issues/42710#issuecomment-763950234

zikaeroh commented 3 years ago

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: https://github.com/golang/go/issues/44557 is now fixed and backported to 1.16 and 1.15, so the dependency can be added once patch adoption is reasonable.