Replace Ordered with an iterator in export.Labels.

[krnowak]

This is to address the first step in #539. The metric.SDK.Labels() used to store the slice passed to it, so the slice could still be modified after the function returned. Now it still modifies the passed slice in place, but it isn't used after the function is finished. The export.Labels does not expose the labels slice anymore. Instead it has a getter for an iterator, so we are not forced to use a slice, but have some other possibilities of storing the labels.

Currently, for empty labelset, we use a "zero" iterator, for labelsets with less than 11 keys, we use the reflect-based iterator, for larger labelsets we do a one-time allocation at the checkpoint time and use a slice iterator.

Benchmarks before:

BenchmarkLabels_1-4    8917160    140 ns/op     96 B/op   2 allocs/op
BenchmarkLabels_2-4    6682400    178 ns/op    144 B/op   2 allocs/op
BenchmarkLabels_4-4    4760353    244 ns/op    240 B/op   2 allocs/op
BenchmarkLabels_8-4    3176577    390 ns/op    432 B/op   2 allocs/op
BenchmarkLabels_16-4    785216   1465 ns/op   1584 B/op   3 allocs/op

Benchmarks after:

BenchmarkLabels_1-4    8303445    144 ns/op    128 B/op   2 allocs/op
BenchmarkLabels_2-4    5826736    181 ns/op    176 B/op   2 allocs/op
BenchmarkLabels_4-4    4678610    251 ns/op    272 B/op   2 allocs/op
BenchmarkLabels_8-4    2968780    390 ns/op    464 B/op   2 allocs/op
BenchmarkLabels_16-4    807482   1513 ns/op   1616 B/op   3 allocs/op

Iterator benchmarks:

BenchmarkIterator_0-4    313194547    3.84 ns/op   0 B/op   0 allocs/op
BenchmarkIterator_1-4    54833654    20.2  ns/op   0 B/op   0 allocs/op
BenchmarkIterator_2-4    58748677    20.2  ns/op   0 B/op   0 allocs/op
BenchmarkIterator_4-4    54932950    20.5  ns/op   0 B/op   0 allocs/op
BenchmarkIterator_8-4    58982492    20.4  ns/op   0 B/op   0 allocs/op
BenchmarkIterator_16-4   152314864    7.90 ns/op   0 B/op   0 allocs/op

[jmacd]

I apologize in advance, but there are some things I don't like about this approach. Sorry!

The use of an iterator pattern feels not like Go to me, and it requires some additional allocations that don't feel necessary. When the SDK creates its orderedLabels value (i.e., the variable-size array copied from the original slice), the array is immutable at that point and can be used in the export pipeline.

We have a labels struct in the SDK that is already a pointer, so it can be placed in an interface without an allocation. I expect the labels struct to go away, but the record will take its place and store the orderedLabels value. So either the labels or record can implement an interface with two methods, instead of an iterator to address the problem at hand--a NumLabels() int and a GetLabel(int) core.KeyValue perhaps. I think this would be a better option--although it is a slightly less usable interface, it saves on an allocation. It forces the user to allocate an index and step through an array.

Of the three fields in labels today, two can disappear (meter becomes available in the embedded instrument in #560, and slice disappears as a result of this work). The remaining orderedLabels array can move into the record and at that point, the export pipeline will take a *record that implements the two methods, which will be passed into the export.Labels. These methods will use reflection and will not cause an allocation if the address is used, like so:

type ExportLabels interface {
   NumLabels() int
   GetLabel(int) core.KeyValue
}
func (r *record) NumLabels(idx int) core.KeyValue {
   return reflect.ValueOf(record.ordered).Len()
}
func (r *record) GetLabel(idx int) core.KeyValue {
   return *(reflect.ValueOf(record.ordered).Index(i).Addr().Interface().(*core.KeyValue))
}

The export.NewLabels() would take a ExportLabels which is really just a pointer to the record. A slightly different approach is required for observers. Can we change the asynchronousInstrument's map[orderedLabels]labeledRecorder to a map[orderedLabels]*labeledRecorder, put the orderedLabels value inside labeledRecorder, and then have labeledRecorder also implement ExportLabels.

Sorry to ask for another approach--I'm open to debate on this topic. Maybe I've got it wrong.

As for the next part of #539, I was imagining new fields added to the record and labeledRecorder to support a cache of encoded strings by encoder. These two types would also, then, implement a new interface to support getting access to the cached encoded string. It makes sense to create a new struct that would contain just an orderedLabels at this point, later it can be extended with support for cached label encodings.

[krnowak]

I apologize in advance, but there are some things I don't like about this approach. Sorry!

No worries.

The use of an iterator pattern feels not like Go to me, and it requires some additional allocations that don't feel necessary.

I got the inspiration for the iterator pattern from reflect.MapIter and bufio.Scanner. I guess this is as Go-like as doing for i := 0; i < storage.NumLabels(); i++ (which feels like C to me, IMO ;) ).

When the SDK creates its orderedLabels value (i.e., the variable-size array copied from the original slice), the array is immutable at that point and can be used in the export pipeline.

We have a labels struct in the SDK that is already a pointer, so it can be placed in an interface without an allocation. I expect the labels struct to go away, but the record will take its place and store the orderedLabels value. So either the labels or record can implement an interface with two methods, instead of an iterator to address the problem at hand--a NumLabels() int and a GetLabel(int) core.KeyValue perhaps. I think this would be a better option--although it is a slightly less usable interface, it saves on an allocation. It forces the user to allocate an index and step through an array.

Alright, I have kept the iterator, but I made it a simple struct, so creating it shouldn't result in any allocation. This resulted in dropping the zero iterator and reflect iterator thingies. So that's good. I added the LabelStorage interface with methods as you described, so labels implement it for now.

Of the three fields in labels today, two can disappear (meter becomes available in the embedded instrument in #560, and slice disappears as a result of this work). The remaining orderedLabels array can move into the record and at that point, the export pipeline will take a *record that implements the two methods, which will be passed into the export.Labels.

I think I'd like to do it in the next PR to avoid making this bigger. But one thing about the slice (which I renamed to sortSlice in this PR) - we need to keep it for now to avoid the allocation in Labels().

Also (for the future PR), I can't yet see how to move contents of labels into record before actually dropping the Labels method from Meter - I can't return *record from Labels. I think this could be done in a single step.

These methods will use reflection and will not cause an allocation if the address is used, like so:

type ExportLabels interface {
   NumLabels() int
   GetLabel(int) core.KeyValue
}
func (r *record) NumLabels(idx int) core.KeyValue {
   return reflect.ValueOf(record.ordered).Len()
}
func (r *record) GetLabel(idx int) core.KeyValue {
   return *(reflect.ValueOf(record.ordered).Index(i).Addr().Interface().(*core.KeyValue))
}

We seem to lose the ability of using Addr() after r.ordered = reflect.ValueOf(array).Interface(). See https://play.golang.org/p/IM0qJfW04HV. But maybe we can get away from allocations during indexing, so using Addr() should not be needed. Benchmark seems to confirm that.

The export.NewLabels() would take a ExportLabels which is really just a pointer to the record. A slightly different approach is required for observers. Can we change the asynchronousInstrument's map[orderedLabels]labeledRecorder to a map[orderedLabels]*labeledRecorder, put the orderedLabels value inside labeledRecorder, and then have labeledRecorder also implement ExportLabels.

Sorry to ask for another approach--I'm open to debate on this topic. Maybe I've got it wrong.

It's all good - it actually led me to a simpler design I think. Thanks for that. I have pushed my changes so far. One thing - I called my interface LabelStorage instead of ExportLabels. I think it's a bit clearer name as ExportLabels is yet another name with "labels" in it in the export package. Also, export.ExportLabels. Stutter bad.

As for the next part of #539, I was imagining new fields added to the record and labeledRecorder to support a cache of encoded strings by encoder. These two types would also, then, implement a new interface to support getting access to the cached encoded string. It makes sense to create a new struct that would contain just an orderedLabels at this point, later it can be extended with support for cached label encodings.

Yeah, something to experiment with when this PR lands.

[jmacd]

This is great!

[krnowak]

Updated.

[krnowak]

Rebased, resolved the conflicts.