-
Notifications
You must be signed in to change notification settings - Fork 195
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
Go: ergonomics improvement for Variants type #624
Comments
I think a sealed interface as suggested by @lann in #612 (comment) is my preferred solution to variants. If possible, I'd suggest avoiding setters in favor of concrete assignment (this is similar to how the Go protobuf generation has getters but no setters on oneof types). |
#612 (comment) by @qmuntal points out a potential performance optimization allowed by struct wrappers. It would be worthwhile to (micro-)benchmark the two approaches to get an idea of the potential performance deltas. |
For reference, the new log/slog package code using the struct wrapper kind approach is here: https://cs.opensource.google/go/go/+/refs/tags/go1.21rc3:src/log/slog/value.go;l=21. |
Disclaimer: please do your own benchmarks, I'm not a users of this project and I can be biased towards a niche use-case. I did run some benchmarks:
Results: goos: windows
goarch: amd64
pkg: gotest
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
│ c2.txt │ c1.txt │
│ sec/op │ sec/op vs base │
AppendInt32-12 213.50n ± 2% 20.64n ± 4% -90.33% (p=0.000 n=10)
AppendInt64-12 261.00n ± 1% 19.14n ± 1% -92.66% (p=0.000 n=10)
AppendString-12 606.15n ± 13% 42.88n ± 8% -92.93% (p=0.000 n=10)
AppendMix-12 1163.50n ± 7% 65.78n ± 13% -94.35% (p=0.000 n=10)
Access-12 1.292n ± 11% 2.623n ± 9% +102.98% (p=0.000 n=10)
│ c2.txt │ c1.txt │
│ B/op │ B/op vs base │
AppendInt32-12 80.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
AppendInt64-12 160.0 ± 0% 0.0 ± 0% -100.00% (p=0.000 n=10)
AppendString-12 320.0 ± 0% 0.0 ± 0% -100.00% (p=0.000 n=10)
AppendMix-12 640.0 ± 0% 0.0 ± 0% -100.00% (p=0.000 n=10)
Access-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10)
│ c2.txt │ c1.txt │
│ allocs/op │ allocs/op vs base │
AppendInt32-12 20.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
AppendInt64-12 20.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
AppendString-12 20.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
AppendMix-12 60.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
Access-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) Highlights:
Although the sealed interface approach is faster in terms of type-switching, the absolute difference is so small (1n vs 2n) that I don't expect it to compensate the increased GC pressure. |
In fairness, we probably should be doing these benchmarks on WebAssembly, not amd64 😅. I also want to caution against being guided too much by benchmarks in designing an ergonomic interface. I am still a fan of the sealed interface, perhaps we could implement both and then perform benchmarks? Ideally with real world examples, too. |
Okay so to summarize the discussion so far. We will want to implement both the sealed interface and sealed struct + integer optaimization methods and comapre their real world benchmarks on Wasm. |
Turns out that using a sealed struct works fine, and is ABI compatible (at least on TinyGo, and eventually on Here is an example of various I modeled the API after the Go enum pattern, with constructors and getters, but not setters (per @johanbrandhorst’s observation). Variant values are pass-by-value (for ABI compatibility), but can implement a common interface. Ignoring that the type ( // NewTimestamp represents the variant "wasi:filesystem/types.new-timestamp".
//
// When setting a timestamp, this gives the value to set it to.
type NewTimestamp struct {
v cm.Variant[uint8, DateTime, struct{}]
}
// NewTimestampNoChange returns a NewTimestamp with variant case "no-change".
func NewTimestampNoChange() NewTimestamp {
var result NewTimestamp
cm.Set(&result.v, 0, struct{}{})
return result
}
// NoChange represents variant case "no-change".
//
// Leave the timestamp set to its previous value.
func (self *NewTimestamp) NoChange() bool {
return self.v.Is(0)
}
// NewTimestampNow returns a NewTimestamp with variant case "now".
func NewTimestampNow() NewTimestamp {
var result NewTimestamp
cm.Set(&result.v, 1, struct{}{})
return result
}
// Now represents variant case "now".
//
// Leave the timestamp set to its previous value.
func (self *NewTimestamp) Now() bool {
return self.v.Is(1)
}
// NewTimestampTimestamp returns a NewTimestamp with variant case "timestamp(datetime)".
func NewTimestampTimestamp(v DateTime) NewTimestamp {
var result NewTimestamp
cm.Set(&result.v, 2, v)
return result
}
// Timestamp represents variant case "timestamp(datetime)".
//
// Set the timestamp to the given value.
func (self *NewTimestamp) Timestamp() (DateTime, bool) {
return cm.Get[DateTime](&self.v, 2)
} |
This issue is created for a discussion on improving the
variants
type in the Go bindgen.To see how the variants type is generated, please read the head of this #612 (comment).
A summary of what's being proposed: #612 (comment)
The text was updated successfully, but these errors were encountered: