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

refactor sets use generic #112377

Merged
merged 1 commit into from Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
136 changes: 26 additions & 110 deletions staging/src/k8s.io/apimachinery/pkg/util/sets/byte.go
@@ -1,5 +1,5 @@
/*
Copyright The Kubernetes Authors.
Copyright 2022 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -14,86 +14,55 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// Code generated by set-gen. DO NOT EDIT.

package sets

import (
"reflect"
"sort"
)

// sets.Byte is a set of bytes, implemented via map[byte]struct{} for minimal memory consumption.
// Byte is a set of bytes, implemented via map[byte]struct{} for minimal memory consumption.
//
// Deprecated: use generic Set instead.
// new ways:
// s1 := Set[byte]{}
// s2 := New[byte]()
type Byte map[byte]Empty

// NewByte creates a Byte from a list of values.
func NewByte(items ...byte) Byte {
ss := make(Byte, len(items))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change these generated sets, then we won't be able to run a benchmark? I suspect the generic code may actually be slower, we should confirm how much.

The cast trick makes it easy to turn one of these types into a generic set if needed, without modifying the generated implementation anyway, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will not able to run the benchmark of both. I should just expose the cast trick and keep generated implementation the old way.

User can cast generated set to generic set on demand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add benchmarks in the initial commit of this PR, run them (go test -count=10 ... >/tmp/before.log), then change the implementation and run them again, then compare (benchstat /tmp/before.log /tmp/after.log).

User can cast generated set to generic set on demand.

Why would they do that? IMHO one of the goals for this PR should be to simplify the implementation by getting rid of the code generator and instead use Go generics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add benchmarks in the initial commit of this PR, run them (go test -count=10 ... >/tmp/before.log), then change the implementation and run them again, then compare (benchstat /tmp/before.log /tmp/after.log).

I'm not sure if I understand you correctly. Are you referring to me doing benchmarks of different implementations in each of the two commits and then comparing the results?

But we ultimately need somewhere to record the results of the comparison. Maybe we can record the result directly in this pr conversation?

IMHO one of the goals for this PR should be to simplify the implementation by getting rid of the code generator and instead use Go generics.

Goals the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to me doing benchmarks of different implementations in each of the two commits and then comparing the results?

Yes.

Maybe we can record the result directly in this pr conversation?

Yes. That's where we decide to go ahead with the refactoring or stick with the current code if we see a performance regression that is too high.

ss.Insert(items...)
return ss
return Byte(New[byte](items...))
}

// ByteKeySet creates a Byte from a keys of a map[byte](? extends interface{}).
// If the value passed in is not actually a map, this will panic.
func ByteKeySet(theMap interface{}) Byte {
v := reflect.ValueOf(theMap)
ret := Byte{}

for _, keyValue := range v.MapKeys() {
ret.Insert(keyValue.Interface().(byte))
}
return ret
func ByteKeySet[T any](theMap map[byte]T) Byte {
return Byte(KeySet(theMap))
}

// Insert adds items to the set.
func (s Byte) Insert(items ...byte) Byte {
for _, item := range items {
s[item] = Empty{}
}
return s
return Byte(cast(s).Insert(items...))
}

// Delete removes all items from the set.
func (s Byte) Delete(items ...byte) Byte {
for _, item := range items {
delete(s, item)
}
return s
return Byte(cast(s).Delete(items...))
}

// Has returns true if and only if item is contained in the set.
func (s Byte) Has(item byte) bool {
_, contained := s[item]
return contained
return cast(s).Has(item)
}

// HasAll returns true if and only if all items are contained in the set.
func (s Byte) HasAll(items ...byte) bool {
for _, item := range items {
if !s.Has(item) {
return false
}
}
return true
return cast(s).HasAll(items...)
}

// HasAny returns true if any items are contained in the set.
func (s Byte) HasAny(items ...byte) bool {
for _, item := range items {
if s.Has(item) {
return true
}
}
return false
return cast(s).HasAny(items...)
}

// Clone returns a new set which is a copy of the current set.
func (s Byte) Clone() Byte {
result := make(Byte, len(s))
for key := range s {
result.Insert(key)
}
return result
return Byte(cast(s).Clone())
}

// Difference returns a set of objects that are not in s2.
Expand All @@ -103,13 +72,7 @@ func (s Byte) Clone() Byte {
// s1.Difference(s2) = {a3}
// s2.Difference(s1) = {a4, a5}
func (s1 Byte) Difference(s2 Byte) Byte {
result := NewByte()
for key := range s1 {
if !s2.Has(key) {
result.Insert(key)
}
}
return result
return Byte(cast(s1).Difference(cast(s2)))
}

// SymmetricDifference returns a set of elements which are in either of the sets, but not in their intersection.
Expand All @@ -119,7 +82,7 @@ func (s1 Byte) Difference(s2 Byte) Byte {
// s1.SymmetricDifference(s2) = {a3, a4, a5}
// s2.SymmetricDifference(s1) = {a3, a4, a5}
func (s1 Byte) SymmetricDifference(s2 Byte) Byte {
return s1.Difference(s2).Union(s2.Difference(s1))
return Byte(cast(s1).SymmetricDifference(cast(s2)))
}

// Union returns a new set which includes items in either s1 or s2.
Expand All @@ -129,11 +92,7 @@ func (s1 Byte) SymmetricDifference(s2 Byte) Byte {
// s1.Union(s2) = {a1, a2, a3, a4}
// s2.Union(s1) = {a1, a2, a3, a4}
func (s1 Byte) Union(s2 Byte) Byte {
result := s1.Clone()
for key := range s2 {
result.Insert(key)
}
return result
return Byte(cast(s1).Union(cast(s2)))
}

// Intersection returns a new set which includes the item in BOTH s1 and s2
Expand All @@ -142,80 +101,37 @@ func (s1 Byte) Union(s2 Byte) Byte {
// s2 = {a2, a3}
// s1.Intersection(s2) = {a2}
func (s1 Byte) Intersection(s2 Byte) Byte {
var walk, other Byte
result := NewByte()
if s1.Len() < s2.Len() {
walk = s1
other = s2
} else {
walk = s2
other = s1
}
for key := range walk {
if other.Has(key) {
result.Insert(key)
}
}
return result
return Byte(cast(s1).Intersection(cast(s2)))
}

// IsSuperset returns true if and only if s1 is a superset of s2.
func (s1 Byte) IsSuperset(s2 Byte) bool {
for item := range s2 {
if !s1.Has(item) {
return false
}
}
return true
return cast(s1).IsSuperset(cast(s2))
}

// Equal returns true if and only if s1 is equal (as a set) to s2.
// Two sets are equal if their membership is identical.
// (In practice, this means same elements, order doesn't matter)
func (s1 Byte) Equal(s2 Byte) bool {
return len(s1) == len(s2) && s1.IsSuperset(s2)
return cast(s1).Equal(cast(s2))
}

type sortableSliceOfByte []byte

func (s sortableSliceOfByte) Len() int { return len(s) }
func (s sortableSliceOfByte) Less(i, j int) bool { return lessByte(s[i], s[j]) }
func (s sortableSliceOfByte) Swap(i, j int) { s[i], s[j] = s[j], s[i] }

// List returns the contents as a sorted byte slice.
func (s Byte) List() []byte {
res := make(sortableSliceOfByte, 0, len(s))
for key := range s {
res = append(res, key)
}
sort.Sort(res)
return []byte(res)
return List(cast(s))
}

// UnsortedList returns the slice with contents in random order.
func (s Byte) UnsortedList() []byte {
res := make([]byte, 0, len(s))
for key := range s {
res = append(res, key)
}
return res
return cast(s).UnsortedList()
}

// Returns a single element from the set.
// PopAny returns a single element from the set.
func (s Byte) PopAny() (byte, bool) {
for key := range s {
s.Delete(key)
return key, true
}
var zeroValue byte
return zeroValue, false
return cast(s).PopAny()
}

// Len returns the size of the set.
func (s Byte) Len() int {
return len(s)
}

func lessByte(lhs, rhs byte) bool {
return lhs < rhs
}
7 changes: 3 additions & 4 deletions staging/src/k8s.io/apimachinery/pkg/util/sets/doc.go
@@ -1,5 +1,5 @@
/*
Copyright The Kubernetes Authors.
Copyright 2022 The Kubernetes Authors.
weilaaa marked this conversation as resolved.
Show resolved Hide resolved

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// Code generated by set-gen. DO NOT EDIT.

// Package sets has auto-generated set types.
// Package sets has generic set and specified sets. Generic set will
// replace specified ones over time. And specific ones are deprecated.
package sets
4 changes: 1 addition & 3 deletions staging/src/k8s.io/apimachinery/pkg/util/sets/empty.go
@@ -1,5 +1,5 @@
/*
Copyright The Kubernetes Authors.
Copyright 2022 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -14,8 +14,6 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// Code generated by set-gen. DO NOT EDIT.

package sets

// Empty is public since it is used by some internal API objects for conversions between external
Expand Down