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

Go: ergonomics improvement for Variants type #624

Open
Tracked by #499
Mossaka opened this issue Jul 20, 2023 · 7 comments
Open
Tracked by #499

Go: ergonomics improvement for Variants type #624

Mossaka opened this issue Jul 20, 2023 · 7 comments
Labels
gen-tinygo Related to the TinyGo bindings generator

Comments

@Mossaka
Copy link
Member

Mossaka commented Jul 20, 2023

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)

@Mossaka Mossaka added the gen-tinygo Related to the TinyGo bindings generator label Jul 20, 2023
@johanbrandhorst
Copy link

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).

@lann
Copy link
Contributor

lann commented Jul 24, 2023

#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.

@johanbrandhorst
Copy link

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.

@qmuntal
Copy link

qmuntal commented Jul 25, 2023

It would be worthwhile to (micro-)benchmark the two approaches to get an idea of the potential performance deltas.

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:
(c1 == sealed struct, c2 == sealed interface)

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:

  • Sealed struct wins:
    • No allocations
    • 10 times faster to append to an slice
  • Sealed interface wins:
    • 2 times faster to access a value via type-switch

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.

@johanbrandhorst
Copy link

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.

@Mossaka
Copy link
Member Author

Mossaka commented Aug 2, 2023

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.

@ydnar
Copy link

ydnar commented Jan 25, 2024

Turns out that using a sealed struct works fine, and is ABI compatible (at least on TinyGo, and eventually on GOARCH=wasm32) with the Canonical ABI.

Here is an example of various variant and result types in-progress targeting WASI Preview 2: https://github.com/ydnar/wasm-tools-go/tree/main/wasi

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 (new-timestamp) starts with New when translated to Go…

// 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)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gen-tinygo Related to the TinyGo bindings generator
Projects
None yet
Development

No branches or pull requests

5 participants