[proposal] zero-allocation Set #4142

MrAlias · 2023-05-26T21:02:04Z

This proposes to separate the data a Set represents from the Set itself. Doing so allows for ...

zero(-ish) allocations to the heap and little computation overhead during set construction
reduction in size of data when passing Sets (i.e. the metric pipeline)

It also means that while Sets remain comparable, their equivalents can no longer be tested with ==. The existing Distinct type or the Equals method need to be used to test equivalence.

Design

Add a registry to globally hold all Set data.
- Sets are constructed by sorting and de-duplicating their data then passing that data to the registry for a unique ID.
- The data in the registry uses reference counting to know when it can be removed from the registry
Redefine a Set to hold pointer to a unique ID of the data (note: this is not the same reference as other sets for the same data)
- When Sets are constructed their IDs have a finalizer set. When they become unreachable the reference they hold to the data will be removed from the registry.
Redefine a Destinct to hold the unique ID of the Set data the Destinct was made from
Use pools for all set data and IDs to amortize heap allocations
A zero allocation implementation of the FNV-1a hash algorithm is added to compute unique IDs for the Set data

Zero Allocations

By owning all of the data and controlling its life-cycle, we are able to effectively use pools for any data that needs to be allocated to the head. This means that all allocations can be amortized and effectively creating Sets will require zero allocations to the heap.

Reduced size of `Set`

Defining a Set with a single *uint64 field means the size of the Set is now 8 bytes. Contrast this to the prior implementation (an interface{} (2 uintptr) referencing an array of N KeyValue) which was (2 * sizeOf(uintptr)) + (N * sizeOf(KeyValue)) (on a 32-bit system with no data this would be 8 bytes).

This reduction in Set data size means that whenever a set is passed as an argument, the sized copied on the stack will be much smaller, but also consistent. This means the Set will always be able to be in a small size stack frame.

Trade Offs

Equivalence testing

The Set is still comparable. However, comparison of Sets created with the same KeyValues will evaluate to false when compared in a map or with ==. To test equivalence the Equals method needs to be used, and a map should be defined over the Distinct type instead.

This incompatibility does not break the API, but it does alter released behavior. Even though the Set is defined with an Equals method and the Distinct type is explicitly declared to be used as a map key instead, this will likely break user (and our metric pipeline) code. Careful consideration of if this is acceptable needs to be made.

It might be possible to resolve this, but I have not found a way. More investigation might be beneficial.

Testing

Moderate test coverage is included. It is not release-ready level of testing.
The BenchmarkNewSet benchmark is added to show the performance improvement of the changes.

$ go test -run='^$' -bench=BenchmarkNewSet -count=20 > out.txt
$ benchstat out.txt
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
         │   out.txt    │
         │    sec/op    │
NewSet-8   2.327µ ± 30%

         │   out.txt   │
         │    B/op     │
NewSet-8   19.50 ± 18%

         │  out.txt   │
         │ allocs/op  │
NewSet-8   0.000 ± 0%

This is a failed experiment that tried to hold set data in its own registry and use a uint64 reference number to act as a fast map key and underlie a set. It failed because a Set is returned from all the NewSet* functions and since it is not allocated to the heap there is no way to correctly set a finalizer for the object.

This is still a failed solution. It converts a pointer to a uintptr via unsafe.Pointer, but it also then converts it back. This is going to cause issues because the original pointer address is not guaranteed to remain the same and casting back could end up pointing at invalid memory.

MrAlias · 2023-06-01T17:33:19Z

An additional branch from this that would show the SDK changes needed to support this would be helpful.

pellared · 2023-06-06T19:14:16Z

Even though the Set is defined with an Equals method and the Distinct type is explicitly declared to be used as a map key instead, this will likely break user (and our metric pipeline) code.

In my opinion, this is acceptable. This is not an ABI breaking change. Moreover, we always have the option to revert the change.

MrAlias · 2023-06-07T18:51:59Z

Using a weak reference for the pointer held by the Set was explored. Similar to this comment and noted in this issue, there is no way to return the same strong pointer from the weak reference without abusing unsafe.Pointer. If Go ever introduces a moving garbage collector the implementation would break.

tigrannajaryan

Sorry, I only had a chance to do a cursory look, so mostly superficial comments.

tigrannajaryan · 2023-06-14T21:04:50Z

attribute/set.go

-var (
-	// keyValueType is used in computeDistinctReflect.
-	keyValueType = reflect.TypeOf(KeyValue{})
+var slicePool = sync.Pool{New: func() any { return new([]KeyValue) }}


I am curious how much this pool helps with performance. How much worse it is without this pool and with just allocating slices as needed?

tigrannajaryan · 2023-06-14T21:05:02Z

attribute/set.go

+func getSlice(length, capacity int) *[]KeyValue {
+	v := slicePool.Get().(*[]KeyValue)
+	if cap(*v) < capacity {
+		*v = make([]KeyValue, length, capacity)


Suggested change

*v = make([]KeyValue, length, capacity)

return make([]KeyValue, length, capacity)

Also can return v to the pool since we didn't use it.

tigrannajaryan · 2023-06-14T21:06:53Z

attribute/set.go

+	return Set{id: id}
+}
+
+var idPool = sync.Pool{New: func() any { return new(uint64) }}


Can you clarify why we need a pool of ids? Can we always allocate instead?

tigrannajaryan · 2023-06-14T21:13:34Z

attribute/set_test.go

+		attribute.Float64Slice("[]float64", []float64{10.23, 941.1, 184e9, -2.3}),
+		attribute.StringSlice("[]string", []string{"", "one", "two"}),
+	}
+	// Pre-sort to remove from first iteration results.


Doesn't this defeat the purpose of the benchmark? Aren't we interested in finding out the performance of a typical case where the attributes are not necessarily sorted? Or is being sorted more typical?

tigrannajaryan · 2023-06-14T21:22:16Z

attribute/set.go

+var sets = newSetRegistry(-1)
+
+func newSet(data *[]KeyValue) Set {
+	id := getID()


I am not quite sure but is it necessary to allocate (or get from a pool) a pointer to an id? The id is already stored in the setData.key. Can Set.id point to setData.key instead?

tigrannajaryan · 2023-06-14T21:24:38Z

attribute/set.go

+		//
+		// A pointer is used so the finalizer can handle reference-counting for
+		// the sets registry while still being optimized as a map key.
+		id *uint64


Would it be possible to make this work by using a pointer to setData instead of a pointer to a separate uint64?

MrAlias · 2024-01-23T18:14:03Z

The use of finalizers here does not seem like a production ready solution. Closing.

MrAlias added the proposal label May 26, 2023

MrAlias added 14 commits May 26, 2023 14:22

Use a stored uniq value to set finalizer on

23568b8

Rename reg.go to registry.go

a8fc5ff

Add header to registry.go

eb3c708

Solve by changing comparison of two equal sets failing

e6cb9cd

Zero allocation set!

a9d9680

Use reference counting for set data

d49ba40

Fix tests

7883e0f

Test comparability of Distinct

18ba702

Refactor and cleanup

46178d9

Rename ref count methods and document

df83b49

Ensure iterator remains non-comparable

5520519

Handle INVALID in hashKV

0bdfa6a

MrAlias force-pushed the set-registry branch from a127ab4 to 0bdfa6a Compare May 26, 2023 21:22

Fix lint

0d11429

This comment was marked as outdated.

Sign in to view

tigrannajaryan reviewed Jun 14, 2023

View reviewed changes

MrAlias closed this Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[proposal] zero-allocation Set #4142

[proposal] zero-allocation Set #4142

MrAlias commented May 26, 2023

MrAlias commented Jun 1, 2023

This comment was marked as outdated.

pellared commented Jun 6, 2023 •

edited

Loading

MrAlias commented Jun 7, 2023 •

edited

Loading

tigrannajaryan left a comment

tigrannajaryan Jun 14, 2023

tigrannajaryan Jun 14, 2023

tigrannajaryan Jun 14, 2023

tigrannajaryan Jun 14, 2023

tigrannajaryan Jun 14, 2023

tigrannajaryan Jun 14, 2023

tigrannajaryan Jun 14, 2023

MrAlias commented Jan 23, 2024

	*v = make([]KeyValue, length, capacity)
	return make([]KeyValue, length, capacity)

[proposal] zero-allocation Set #4142

[proposal] zero-allocation Set #4142

Conversation

MrAlias commented May 26, 2023

Design

Zero Allocations

Reduced size of Set

Trade Offs

Equivalence testing

Testing

MrAlias commented Jun 1, 2023

This comment was marked as outdated.

pellared commented Jun 6, 2023 • edited Loading

MrAlias commented Jun 7, 2023 • edited Loading

tigrannajaryan left a comment

Choose a reason for hiding this comment

tigrannajaryan Jun 14, 2023

Choose a reason for hiding this comment

tigrannajaryan Jun 14, 2023

Choose a reason for hiding this comment

tigrannajaryan Jun 14, 2023

Choose a reason for hiding this comment

tigrannajaryan Jun 14, 2023

Choose a reason for hiding this comment

tigrannajaryan Jun 14, 2023

Choose a reason for hiding this comment

tigrannajaryan Jun 14, 2023

Choose a reason for hiding this comment

tigrannajaryan Jun 14, 2023

Choose a reason for hiding this comment

MrAlias commented Jan 23, 2024

Reduced size of `Set`

pellared commented Jun 6, 2023 •

edited

Loading

MrAlias commented Jun 7, 2023 •

edited

Loading