From 4f14733271830a75c0631f2006fb294862e2de4c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 2 Jan 2020 15:32:30 -0800 Subject: [PATCH 1/2] Add comments on needed filed alignment Add comment about alignment requirements to all struct fields who's values are passed to 64-bit atomic operations. Update any struct's field ordering if one or more of those fields has alignment requirements to support 64-bit atomic operations. --- api/core/number_test.go | 6 ++++-- api/metric/api.go | 3 ++- internal/metric/mock.go | 6 ++++-- internal/trace/mock_tracer.go | 8 +++++--- sdk/metric/aggregator/array/array.go | 3 ++- sdk/metric/aggregator/counter/counter.go | 2 ++ sdk/metric/aggregator/gauge/gauge.go | 2 ++ sdk/metric/aggregator/minmaxsumcount/mmsc.go | 5 ++++- sdk/metric/aggregator/test/test.go | 3 ++- sdk/metric/monotone_test.go | 7 ++++--- sdk/metric/sdk.go | 20 ++++++++++++++------ sdk/metric/stress_test.go | 2 ++ 12 files changed, 47 insertions(+), 20 deletions(-) diff --git a/api/core/number_test.go b/api/core/number_test.go index 67583686c54..0fb75b89294 100644 --- a/api/core/number_test.go +++ b/api/core/number_test.go @@ -67,12 +67,14 @@ func TestNumber(t *testing.T) { cmpsForPos := [3]int{1, 1, 0} type testcase struct { - n Number + // n needs to be aligned for 64-bit atomic operations. + n Number + // nums needs to be aligned for 64-bit atomic operations. + nums [3]Number kind NumberKind pos bool zero bool neg bool - nums [3]Number cmps [3]int } testcases := []testcase{ diff --git a/api/metric/api.go b/api/metric/api.go index f96bd75d5bc..2e546c697a4 100644 --- a/api/metric/api.go +++ b/api/metric/api.go @@ -85,8 +85,9 @@ type MeasureOptionApplier interface { // values. Instances of this type should be created by instruments // (e.g., Int64Counter.Measurement()). type Measurement struct { - instrument InstrumentImpl + // number needs to be aligned for 64-bit atomic operations. number core.Number + instrument InstrumentImpl } // Instrument returns the instrument that created this measurement. diff --git a/internal/metric/mock.go b/internal/metric/mock.go index 68b7d7582a9..8bd37574f5b 100644 --- a/internal/metric/mock.go +++ b/internal/metric/mock.go @@ -41,9 +41,10 @@ type ( } Batch struct { + // Measurement needs to be aligned for 64-bit atomic operations. + Measurements []Measurement Ctx context.Context LabelSet *LabelSet - Measurements []Measurement } MeterProvider struct { @@ -58,8 +59,9 @@ type ( Kind int8 Measurement struct { - Instrument *Instrument + // Number needs to be aligned for 64-bit atomic operations. Number core.Number + Instrument *Instrument } ) diff --git a/internal/trace/mock_tracer.go b/internal/trace/mock_tracer.go index d6d033d03be..98d973f8b5b 100644 --- a/internal/trace/mock_tracer.go +++ b/internal/trace/mock_tracer.go @@ -28,12 +28,14 @@ import ( // It only supports ChildOf option. SpanId is atomically increased every time a // new span is created. type MockTracer struct { - // Sampled specifies if the new span should be sampled or not. - Sampled bool - // StartSpanID is used to initialize spanId. It is incremented by one // every time a new span is created. + // + // StartSpanID has to be aligned for 64-bit atomic operations. StartSpanID *uint64 + + // Sampled specifies if the new span should be sampled or not. + Sampled bool } var _ apitrace.Tracer = (*MockTracer)(nil) diff --git a/sdk/metric/aggregator/array/array.go b/sdk/metric/aggregator/array/array.go index f9b79d60df9..3855e8cc28c 100644 --- a/sdk/metric/aggregator/array/array.go +++ b/sdk/metric/aggregator/array/array.go @@ -28,10 +28,11 @@ import ( type ( Aggregator struct { + // ckptSum needs to be aligned for 64-bit atomic operations. + ckptSum core.Number lock sync.Mutex current points checkpoint points - ckptSum core.Number } points []core.Number diff --git a/sdk/metric/aggregator/counter/counter.go b/sdk/metric/aggregator/counter/counter.go index 93660e0d2d9..7d4b8cd0ed4 100644 --- a/sdk/metric/aggregator/counter/counter.go +++ b/sdk/metric/aggregator/counter/counter.go @@ -25,9 +25,11 @@ import ( // Aggregator aggregates counter events. type Aggregator struct { // current holds current increments to this counter record + // current needs to be aligned for 64-bit atomic operations. current core.Number // checkpoint is a temporary used during Checkpoint() + // checkpoint needs to be aligned for 64-bit atomic operations. checkpoint core.Number } diff --git a/sdk/metric/aggregator/gauge/gauge.go b/sdk/metric/aggregator/gauge/gauge.go index 5b86d7585bf..a9e73f73de8 100644 --- a/sdk/metric/aggregator/gauge/gauge.go +++ b/sdk/metric/aggregator/gauge/gauge.go @@ -45,6 +45,8 @@ type ( // a sequence number to determine the winner of a race. gaugeData struct { // value is the int64- or float64-encoded Set() data + // + // value needs to be aligned for 64-bit atomic operations. value core.Number // timestamp indicates when this record was submitted. diff --git a/sdk/metric/aggregator/minmaxsumcount/mmsc.go b/sdk/metric/aggregator/minmaxsumcount/mmsc.go index 24876979fa2..8d9b67c7d6a 100644 --- a/sdk/metric/aggregator/minmaxsumcount/mmsc.go +++ b/sdk/metric/aggregator/minmaxsumcount/mmsc.go @@ -26,12 +26,15 @@ type ( // Aggregator aggregates measure events, keeping only the max, // sum, and count. Aggregator struct { - current state + // current has to be aligned for 64-bit atomic operations. + current state + // checkpoint has to be aligned for 64-bit atomic operations. checkpoint state kind core.NumberKind } state struct { + // all fields have to be aligned for 64-bit atomic operations. count core.Number sum core.Number min core.Number diff --git a/sdk/metric/aggregator/test/test.go b/sdk/metric/aggregator/test/test.go index 3bd484c813f..6df4c3f34c4 100644 --- a/sdk/metric/aggregator/test/test.go +++ b/sdk/metric/aggregator/test/test.go @@ -64,8 +64,9 @@ func RunProfiles(t *testing.T, f func(*testing.T, Profile)) { } type Numbers struct { - kind core.NumberKind + // numbers has to be aligned for 64-bit atomic operations. numbers []core.Number + kind core.NumberKind } func NewNumbers(kind core.NumberKind) Numbers { diff --git a/sdk/metric/monotone_test.go b/sdk/metric/monotone_test.go index 0c3c5dadd88..9fb6ee99fcb 100644 --- a/sdk/metric/monotone_test.go +++ b/sdk/metric/monotone_test.go @@ -31,11 +31,12 @@ import ( ) type monotoneBatcher struct { - t *testing.T - - collections int + // currentValue needs to be aligned for 64-bit atomic operations. currentValue *core.Number + collections int currentTime *time.Time + + t *testing.T } func (*monotoneBatcher) AggregatorFor(*export.Descriptor) export.Aggregator { diff --git a/sdk/metric/sdk.go b/sdk/metric/sdk.go index 30cb13e7901..5c15eca14bc 100644 --- a/sdk/metric/sdk.go +++ b/sdk/metric/sdk.go @@ -97,31 +97,39 @@ type ( // `record` in existence at a time, although at most one can // be referenced from the `SDK.current` map. record struct { - // labels is the LabelSet passed by the user. - labels *labels - - // descriptor describes the metric instrument. - descriptor *export.Descriptor - // refcount counts the number of active handles on // referring to this record. active handles prevent // removing the record from the current map. + // + // refcount has to be aligned for 64-bit atomic operations. refcount int64 // collectedEpoch is the epoch number for which this // record has been exported. This is modified by the // `Collect()` method. + // + // collectedEpoch has to be aligned for 64-bit atomic operations. collectedEpoch int64 // modifiedEpoch is the latest epoch number for which // this record was updated. Generally, if // modifiedEpoch is less than collectedEpoch, this // record is due for reclaimation. + // + // modifiedEpoch has to be aligned for 64-bit atomic operations. modifiedEpoch int64 // reclaim is an atomic to control the start of reclaiming. + // + // reclaim has to be aligned for 64-bit atomic operations. reclaim int64 + // labels is the LabelSet passed by the user. + labels *labels + + // descriptor describes the metric instrument. + descriptor *export.Descriptor + // recorder implements the actual RecordOne() API, // depending on the type of aggregation. If nil, the // metric was disabled by the exporter. diff --git a/sdk/metric/stress_test.go b/sdk/metric/stress_test.go index 6e17802dc28..a7149f81068 100644 --- a/sdk/metric/stress_test.go +++ b/sdk/metric/stress_test.go @@ -51,6 +51,7 @@ const ( type ( testFixture struct { + // stop has to be aligned for 64-bit atomic operations. stop int64 expected sync.Map received sync.Map // Note: doesn't require synchronization @@ -93,6 +94,7 @@ type ( // where a race condition causes duplicate records. We always // take the later timestamp. gaugeState struct { + // raw has to be aligned for 64-bit atomic operations. raw core.Number ts time.Time } From 8e8d9b708ea327db54e3351fb6b0259af81167bb Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 3 Jan 2020 14:42:20 -0800 Subject: [PATCH 2/2] Add 64-bit alignment tests Most `struct` that have field alignment requirements are now statically validated prior to testing. The only `struct`s not validated that have these requirements are ones defined in tests themselves where multiple `TestMain` functions would be needed to test them. Given the fields are already identified with comments specifying the alignment requirements and they are in the test themselves, this seems like an OK omission. --- api/metric/alignment_test.go | 24 ++++++++ internal/metric/mock_test.go | 28 +++++++++ internal/testing/alignment.go | 57 +++++++++++++++++++ internal/trace/mock_tracer_test.go | 24 ++++++++ sdk/metric/aggregator/array/array_test.go | 18 ++++++ sdk/metric/aggregator/counter/counter_test.go | 22 +++++++ sdk/metric/aggregator/gauge/gauge_test.go | 18 ++++++ .../aggregator/minmaxsumcount/mmsc_test.go | 38 +++++++++++++ sdk/metric/aggregator/test/test.go | 18 ++++++ sdk/metric/alignment_test.go | 36 ++++++++++++ 10 files changed, 283 insertions(+) create mode 100644 api/metric/alignment_test.go create mode 100644 internal/metric/mock_test.go create mode 100644 internal/testing/alignment.go create mode 100644 internal/trace/mock_tracer_test.go create mode 100644 sdk/metric/alignment_test.go diff --git a/api/metric/alignment_test.go b/api/metric/alignment_test.go new file mode 100644 index 00000000000..92ca9937a0a --- /dev/null +++ b/api/metric/alignment_test.go @@ -0,0 +1,24 @@ +package metric + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Measurement.number", + Offset: unsafe.Offsetof(Measurement{}.number), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} diff --git a/internal/metric/mock_test.go b/internal/metric/mock_test.go new file mode 100644 index 00000000000..7995b17d02d --- /dev/null +++ b/internal/metric/mock_test.go @@ -0,0 +1,28 @@ +package metric + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Batch.Measurments", + Offset: unsafe.Offsetof(Batch{}.Measurements), + }, + { + Name: "Measurement.Number", + Offset: unsafe.Offsetof(Measurement{}.Number), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} diff --git a/internal/testing/alignment.go b/internal/testing/alignment.go new file mode 100644 index 00000000000..92b8b1e5834 --- /dev/null +++ b/internal/testing/alignment.go @@ -0,0 +1,57 @@ +package testing + +/* +This file contains common utilities and objects to validate memory alignment +of Go types. The primary use of this functionality is intended to ensure +`struct` fields that need to be 64-bit aligned so they can be passed as +arguments to 64-bit atomic operations. + +The common workflow is to define a slice of `FieldOffset` and pass them to the +`Aligned8Byte` function from within a `TestMain` function from a package's +tests. It is important to make this call from the `TestMain` function prior +to running the rest of the test suit as it can provide useful diagnostics +about field alignment instead of ambiguous nil pointer dereference and runtime +panic. + +For more information: +https://github.com/open-telemetry/opentelemetry-go/issues/341 +*/ + +import ( + "fmt" + "io" +) + +// FieldOffset is a preprocessor representation of a struct field alignment. +type FieldOffset struct { + // Name of the field. + Name string + + // Offset of the field in bytes. + // + // To compute this at compile time use unsafe.Offsetof. + Offset uintptr +} + +// Aligned8Byte returns if all fields are aligned modulo 8-bytes. +// +// Error messaging is printed to out for any fileds determined misaligned. +func Aligned8Byte(fields []FieldOffset, out io.Writer) bool { + misaligned := make([]FieldOffset, 0) + for _, f := range fields { + if f.Offset%8 != 0 { + misaligned = append(misaligned, f) + } + } + + if len(misaligned) == 0 { + return true + } + + fmt.Fprintln(out, "struct fields not aligned for 64-bit atomic operations:") + for _, f := range misaligned { + fmt.Fprintf(out, " %s: %d-byte offset\n", f.Name, f.Offset) + } + + return false +} diff --git a/internal/trace/mock_tracer_test.go b/internal/trace/mock_tracer_test.go new file mode 100644 index 00000000000..fd8791b18ce --- /dev/null +++ b/internal/trace/mock_tracer_test.go @@ -0,0 +1,24 @@ +package trace + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "MockTracer.StartSpanID", + Offset: unsafe.Offsetof(MockTracer{}.StartSpanID), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} diff --git a/sdk/metric/aggregator/array/array_test.go b/sdk/metric/aggregator/array/array_test.go index aa5ec525290..53af8415316 100644 --- a/sdk/metric/aggregator/array/array_test.go +++ b/sdk/metric/aggregator/array/array_test.go @@ -18,16 +18,34 @@ import ( "context" "fmt" "math" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" ) +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Aggregator.ckptSum", + Offset: unsafe.Offsetof(Aggregator{}.ckptSum), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + type updateTest struct { count int absolute bool diff --git a/sdk/metric/aggregator/counter/counter_test.go b/sdk/metric/aggregator/counter/counter_test.go index 7ce5d16cdac..4c66a78e4a1 100644 --- a/sdk/metric/aggregator/counter/counter_test.go +++ b/sdk/metric/aggregator/counter/counter_test.go @@ -16,17 +16,39 @@ package counter import ( "context" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" ) const count = 100 +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Aggregator.current", + Offset: unsafe.Offsetof(Aggregator{}.current), + }, + { + Name: "Aggregator.checkpoint", + Offset: unsafe.Offsetof(Aggregator{}.checkpoint), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + func TestCounterMonotonic(t *testing.T) { ctx := context.Background() diff --git a/sdk/metric/aggregator/gauge/gauge_test.go b/sdk/metric/aggregator/gauge/gauge_test.go index aeff19643f1..34fd7f53da8 100644 --- a/sdk/metric/aggregator/gauge/gauge_test.go +++ b/sdk/metric/aggregator/gauge/gauge_test.go @@ -17,11 +17,14 @@ package gauge import ( "context" "math/rand" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" @@ -31,6 +34,21 @@ const count = 100 var _ export.Aggregator = &Aggregator{} +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "gaugeData.value", + Offset: unsafe.Offsetof(gaugeData{}.value), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + func TestGaugeNonMonotonic(t *testing.T) { ctx := context.Background() diff --git a/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go b/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go index fb2142fcd76..21feacd19e1 100644 --- a/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go +++ b/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go @@ -18,11 +18,14 @@ import ( "context" "math" "math/rand" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" @@ -59,6 +62,41 @@ var ( } ) +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Aggregator.current", + Offset: unsafe.Offsetof(Aggregator{}.current), + }, + { + Name: "Aggregator.checkpoint", + Offset: unsafe.Offsetof(Aggregator{}.checkpoint), + }, + { + Name: "state.count", + Offset: unsafe.Offsetof(state{}.count), + }, + { + Name: "state.sum", + Offset: unsafe.Offsetof(state{}.sum), + }, + { + Name: "state.min", + Offset: unsafe.Offsetof(state{}.min), + }, + { + Name: "state.max", + Offset: unsafe.Offsetof(state{}.max), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + func TestMinMaxSumCountAbsolute(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { minMaxSumCount(t, profile, positiveOnly) diff --git a/sdk/metric/aggregator/test/test.go b/sdk/metric/aggregator/test/test.go index 6df4c3f34c4..582183fc189 100644 --- a/sdk/metric/aggregator/test/test.go +++ b/sdk/metric/aggregator/test/test.go @@ -17,10 +17,13 @@ package test import ( "context" "math/rand" + "os" "sort" "testing" + "unsafe" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" ) @@ -63,6 +66,21 @@ func RunProfiles(t *testing.T, f func(*testing.T, Profile)) { } } +// Ensure local struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Numbers.numbers", + Offset: unsafe.Offsetof(Numbers{}.numbers), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + type Numbers struct { // numbers has to be aligned for 64-bit atomic operations. numbers []core.Number diff --git a/sdk/metric/alignment_test.go b/sdk/metric/alignment_test.go new file mode 100644 index 00000000000..9e00ae9e827 --- /dev/null +++ b/sdk/metric/alignment_test.go @@ -0,0 +1,36 @@ +package metric + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "record.refcount", + Offset: unsafe.Offsetof(record{}.refcount), + }, + { + Name: "record.collectedEpoch", + Offset: unsafe.Offsetof(record{}.collectedEpoch), + }, + { + Name: "record.modifiedEpoch", + Offset: unsafe.Offsetof(record{}.modifiedEpoch), + }, + { + Name: "record.reclaim", + Offset: unsafe.Offsetof(record{}.reclaim), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +}