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

Add comments and test for 64-bit field alignment #418

Merged
merged 3 commits into from
Jan 6, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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.
Tyler Yahn committed Jan 2, 2020
commit 4f14733271830a75c0631f2006fb294862e2de4c
6 changes: 4 additions & 2 deletions api/core/number_test.go
Original file line number Diff line number Diff line change
@@ -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{
3 changes: 2 additions & 1 deletion api/metric/api.go
Original file line number Diff line number Diff line change
@@ -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.
6 changes: 4 additions & 2 deletions internal/metric/mock.go
Original file line number Diff line number Diff line change
@@ -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
}
)

8 changes: 5 additions & 3 deletions internal/trace/mock_tracer.go
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 2 additions & 1 deletion sdk/metric/aggregator/array/array.go
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions sdk/metric/aggregator/counter/counter.go
Original file line number Diff line number Diff line change
@@ -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
}

2 changes: 2 additions & 0 deletions sdk/metric/aggregator/gauge/gauge.go
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 4 additions & 1 deletion sdk/metric/aggregator/minmaxsumcount/mmsc.go
Original file line number Diff line number Diff line change
@@ -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
3 changes: 2 additions & 1 deletion sdk/metric/aggregator/test/test.go
Original file line number Diff line number Diff line change
@@ -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 {
7 changes: 4 additions & 3 deletions sdk/metric/monotone_test.go
Original file line number Diff line number Diff line change
@@ -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 {
20 changes: 14 additions & 6 deletions sdk/metric/sdk.go
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 2 additions & 0 deletions sdk/metric/stress_test.go
Original file line number Diff line number Diff line change
@@ -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
}