diff --git a/AGENTS.md b/AGENTS.md index 253c1a37..0aa242fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,6 +112,9 @@ exercised a live Windows role. instance occurrences within a sample; their suffixes are not stable identities. Do not remove every name ending in `_Total`: legitimate process and database names can use that suffix. Use the shared aggregate-instance policy. + `pdh.Collector.Collect` reuses the backing array of its destination slice + and `Row.Values`; do not keep rows, sub-slices or pointers into it across + scrapes. - PDH collectors add counters by English path. On a Windows installation without en-US resources, the 009 name table holds localized names for v2 (manifest-based) counter sets such as `SMB Server Shares` or `HTTP Service`; diff --git a/internal/pdh/collector.go b/internal/pdh/collector.go index ab2e58a3..30ed5600 100644 --- a/internal/pdh/collector.go +++ b/internal/pdh/collector.go @@ -18,6 +18,7 @@ package pdh import ( + "context" "errors" "fmt" "log/slog" @@ -50,6 +51,7 @@ type Collector[T any] struct { object string resultType CounterType counters []Counter + counterIndex map[string]int // index in counters by counter name handle pdhQueryHandle totalCounterRequested bool mu sync.RWMutex @@ -73,10 +75,12 @@ type Row struct { Values []float64 } -// rowAccessor creates rows of type T and sets their names and counter values. +// rowAccessor initializes rows of type T and sets their names and counter values. // field is the index the counterField of the counter was created with. type rowAccessor[T any] struct { - newRow func(instance string) T + // resetRow initializes row for instance. row can hold the values of a + // previous sample; resetRow can reuse its memory. + resetRow func(row *T, instance string) setName func(row *T, instance string) setValue func(row *T, field int, value float64) } @@ -84,7 +88,9 @@ type rowAccessor[T any] struct { // counterField maps a counter to the field index passed to rowAccessor.setValue. type counterField struct { // tag is the counter name, optionally suffixed with ",secondvalue". - tag string + tag string + // index is the byte offset of the field in a struct row, or the index + // in Row.Values. index int minOSBuild string } @@ -132,15 +138,30 @@ func NewQueryCollector[T any](logger *slog.Logger, query *Query, resultType Coun } func newStructCollector[T any](logger *slog.Logger, query *Query, resultType CounterType, object string, instances []string) (*Collector[T], error) { - valueType := reflect.TypeFor[T]() - if valueType.Kind() != reflect.Struct { - return nil, fmt.Errorf("expected a struct, got %s", valueType) + fields, rows, errs, err := structFields[T]() + if err != nil { + return nil, err } - nameIndex := -1 + return newCollector(logger, query, resultType, object, instances, fields, rows, false, errs) +} + +// structFields returns the counter fields declared by the perfdata tags of +// the struct T and an accessor that writes them. The index of a counterField +// is the byte offset of its float64 field in T, so the accessor writes values +// without reflection. +func structFields[T any]() ([]counterField, rowAccessor[T], []error, error) { + valueType := reflect.TypeFor[T]() + if valueType.Kind() != reflect.Struct { + return nil, rowAccessor[T]{}, nil, fmt.Errorf("expected a struct, got %s", valueType) + } + + nameOffset := -1 if f, ok := valueType.FieldByName("Name"); ok && f.Type.Kind() == reflect.String { - nameIndex = f.Index[0] + if offset, ok := fieldOffset(valueType, f.Index); ok { + nameOffset = offset + } } var errs []error @@ -159,34 +180,100 @@ func newStructCollector[T any](logger *slog.Logger, query *Query, resultType Cou continue } + offset, ok := fieldOffset(valueType, f.Index) + if !ok { + errs = append(errs, fmt.Errorf("field %s must not be promoted through an embedded pointer", f.Name)) + + continue + } + fields = append(fields, counterField{ tag: counterName, - index: f.Index[0], + index: offset, minOSBuild: f.Tag.Get("perfdata_min_build"), }) } - rows := rowAccessor[T]{ - newRow: func(instance string) T { - var row T - - if nameIndex != -1 { - reflect.ValueOf(&row).Elem().Field(nameIndex).SetString(instance) - } - - return row - }, - setName: func(row *T, instance string) { - if nameIndex != -1 { - reflect.ValueOf(row).Elem().Field(nameIndex).SetString(instance) - } - }, - setValue: func(row *T, field int, value float64) { - reflect.ValueOf(row).Elem().Field(field).SetFloat(value) - }, + // structRowAccessor writes through these offsets with unsafe, so an index + // that is not the offset of a float64 field, e.g. a field number, would + // corrupt memory. Refuse it instead. + if err := validateFloatOffsets(valueType, fields); err != nil { + return nil, rowAccessor[T]{}, nil, err } - return newCollector(logger, query, resultType, object, instances, fields, rows, false, errs) + return fields, structRowAccessor[T](nameOffset), errs, nil +} + +// validateFloatOffsets checks that the index of every field is the byte +// offset of a float64 field stored in the struct t. +func validateFloatOffsets(t reflect.Type, fields []counterField) error { + floatOffsets := make(map[int]struct{}, len(fields)) + + for _, f := range reflect.VisibleFields(t) { + if f.Type.Kind() != reflect.Float64 { + continue + } + + if offset, ok := fieldOffset(t, f.Index); ok { + floatOffsets[offset] = struct{}{} + } + } + + size := int(t.Size()) + floatSize := int(unsafe.Sizeof(float64(0))) + + for _, f := range fields { + if _, ok := floatOffsets[f.index]; !ok || f.index%floatSize != 0 || f.index+floatSize > size { + return fmt.Errorf("perfdata field %q: %d is not the byte offset of a float64 field of %s", f.tag, f.index, t) + } + } + + return nil +} + +// fieldOffset returns the byte offset of the field with the index sequence +// index in the struct t. ok is false if the field is reached through an +// embedded pointer and is therefore not stored in t itself. +func fieldOffset(t reflect.Type, index []int) (int, bool) { + var offset uintptr + + for _, i := range index { + if t.Kind() != reflect.Struct { + return 0, false + } + + f := t.Field(i) + offset += f.Offset + t = f.Type + } + + return int(offset), true +} + +// structRowAccessor returns the accessor of the struct T. nameOffset is the +// byte offset of its string field Name, or -1. The offsets passed to setValue +// come from [structFields] and address float64 fields of T, so values are +// written without reflection. +func structRowAccessor[T any](nameOffset int) rowAccessor[T] { + setName := func(row *T, instance string) { + if nameOffset >= 0 { + *(*string)(unsafe.Add(unsafe.Pointer(row), nameOffset)) = instance + } + } + + return rowAccessor[T]{ + resetRow: func(row *T, instance string) { + var zero T + + *row = zero + + setName(row, instance) + }, + setName: setName, + setValue: func(row *T, offset int, value float64) { + *(*float64)(unsafe.Add(unsafe.Pointer(row), offset)) = value + }, + } } // NewDynamicCollector creates a collector for counters that are only known at runtime. @@ -199,9 +286,24 @@ func NewDynamicCollector(logger *slog.Logger, resultType CounterType, object str fields[i] = counterField{tag: counter, index: i} } - rows := rowAccessor[Row]{ - newRow: func(instance string) Row { - return Row{Name: instance, Values: make([]float64, len(counters))} + return newCollector(logger, nil, resultType, object, instances, fields, dynamicRowAccessor(len(counters)), true, nil) +} + +// dynamicRowAccessor returns the accessor of rows with n values. A reused row +// keeps its Values array if it is large enough. +func dynamicRowAccessor(n int) rowAccessor[Row] { + return rowAccessor[Row]{ + resetRow: func(row *Row, instance string) { + row.Name = instance + + if cap(row.Values) < n { + row.Values = make([]float64, n) + + return + } + + row.Values = row.Values[:n] + clear(row.Values) }, setName: func(row *Row, instance string) { row.Name = instance @@ -210,8 +312,6 @@ func NewDynamicCollector(logger *slog.Logger, resultType CounterType, object str row.Values[field] = value }, } - - return newCollector(logger, nil, resultType, object, instances, fields, rows, true, nil) } func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterType, object string, instances []string, @@ -246,6 +346,7 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy object: object, resultType: resultType, counters: make([]Counter, 0, len(fields)), + counterIndex: make(map[string]int, len(fields)), handle: handle, totalCounterRequested: slices.ContainsFunc(instances, func(instance string) bool { return IsTotalInstance(object, instance) }), mu: sync.RWMutex{}, @@ -254,15 +355,13 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy partialRows: partialRows, } - counterIndex := make(map[string]int, len(fields)) - for _, f := range fields { counterName, secondValue := strings.CutSuffix(f.tag, ",secondvalue") - i, ok := counterIndex[counterName] + i, ok := collector.counterIndex[counterName] if !ok { i = len(collector.counters) - counterIndex[counterName] = i + collector.counterIndex[counterName] = i collector.counters = append(collector.counters, Counter{ Name: counterName, Instances: make(map[string]pdhCounterHandle, len(instances)), @@ -438,13 +537,14 @@ func (c *Collector[T]) MetricType(counterName string) (prometheus.ValueType, boo c.mu.RLock() defer c.mu.RUnlock() - for _, counter := range c.counters { - if counter.Name == counterName { - return counter.MetricType, counter.MetricType != 0 - } + i, ok := c.counterIndex[counterName] + if !ok { + return 0, false } - return 0, false + metricType := c.counters[i].MetricType + + return metricType, metricType != 0 } // Frequency returns the time base in ticks per second of the raw timer counter @@ -461,16 +561,22 @@ func (c *Collector[T]) Frequency(counterName string) (int64, bool) { c.mu.RLock() defer c.mu.RUnlock() - for _, counter := range c.counters { - if counter.Name == counterName { - return counter.Frequency, counter.Frequency > 0 - } + i, ok := c.counterIndex[counterName] + if !ok { + return 0, false } - return 0, false + frequency := c.counters[i].Frequency + + return frequency, frequency > 0 } // Collect replaces the content of dst with one row per collected instance. +// +// Collect reuses the backing array of dst and, for [Row], the Values slices of +// its rows, so a sample does not allocate them again. Rows, sub-slices of dst +// and Row.Values are only valid until the next Collect into the same slice; +// copy what must be kept longer. func (c *Collector[T]) Collect(dst *[]T) error { if c == nil { return ErrPerformanceCounterNotInitialized @@ -530,7 +636,16 @@ func (c *Collector[T]) collect(dst *[]T, state *collectState) (err error) { } } - *dst = (*dst)[:0:0] + // Keep the backing array; rowSet initializes every row it reuses. + previousLen := len(*dst) + *dst = (*dst)[:0] + + defer func() { + // Release the instance names of rows that this sample no longer uses. + if n := len(*dst); n < previousLen { + clear((*dst)[n:previousLen]) + } + }() rows := newRowSet(c, dst, state) @@ -607,9 +722,9 @@ const pdhBufferSlack = 8 // getCounterArray reads the counter array of instance into buf and returns the number of items. // ok is false if the counter has no data in this sample. func (c *Collector[T]) getCounterArray(instance pdhCounterHandle, buf *[]byte) (uint32, bool, error) { - getArray, name := GetRawCounterArray, "GetRawCounterArray" + name := "GetRawCounterArray" if c.resultType == CounterTypeFormatted { - getArray, name = GetFormattedCounterArrayDouble, "GetFormattedCounterArrayDouble" + name = "GetFormattedCounterArrayDouble" } var itemCount uint32 @@ -618,18 +733,28 @@ func (c *Collector[T]) getCounterArray(instance pdhCounterHandle, buf *[]byte) ( bytesNeeded := uint32(len(*buf)) for { - ret := getArray(instance, &bytesNeeded, &itemCount, unsafe.SliceData(*buf)) + // Direct calls instead of a function value keep bytesNeeded and + // itemCount on the stack. + var ret uint32 + if c.resultType == CounterTypeFormatted { + ret = GetFormattedCounterArrayDouble(instance, &bytesNeeded, &itemCount, unsafe.SliceData(*buf)) + } else { + ret = GetRawCounterArray(instance, &bytesNeeded, &itemCount, unsafe.SliceData(*buf)) + } if ret == ErrorSuccess { return itemCount, true, nil } - if err := NewPdhError(ret); ret != MoreData { - if isKnownCounterDataError(err) { + // Check the status before creating an error: NewPdhError formats the + // message text, which a growing buffer or a counter without data in + // this sample does not need. + if ret != MoreData { + if isKnownCounterDataStatus(ret) { return 0, false, nil } - return 0, false, fmt.Errorf("%s: %w", name, err) + return 0, false, fmt.Errorf("%s: %w", name, NewPdhError(ret)) } if bytesNeeded <= uint32(len(*buf)) { @@ -734,11 +859,15 @@ func (r *rowSet[T]) row(counter *Counter, szName *uint16, status uint32) (int, b r.occurrences[instanceName] = max(r.occurrences[instanceName], occurrence+1) if status != CstatusValidData && status != CstatusNewData { - r.c.logger.Debug("skipping counter item with invalid data status", - slog.String("counter", counter.Name), - slog.String("instance", instanceName), - slog.Uint64("status", uint64(status)), - ) + // Check the level first, so the attributes are not built for every + // invalid item while debug logging is off. + if r.c.logger.Enabled(context.Background(), slog.LevelDebug) { + r.c.logger.LogAttrs(context.Background(), slog.LevelDebug, "skipping counter item with invalid data status", + slog.String("counter", counter.Name), + slog.String("instance", instanceName), + slog.Uint64("status", uint64(status)), + ) + } return 0, false } @@ -752,10 +881,21 @@ func (r *rowSet[T]) row(counter *Counter, szName *uint16, status uint32) (int, b return index, true } - index := len(*r.dst) + rows := *r.dst + index := len(rows) r.index[key] = index - *r.dst = append(*r.dst, r.c.rows.newRow(instanceName)) + // Reuse a row of a previous sample within the capacity of dst. + if index < cap(rows) { + rows = rows[:index+1] + } else { + var zero T + + rows = append(rows, zero) + } + + r.c.rows.resetRow(&rows[index], instanceName) + *r.dst = rows n := len(r.c.counters) r.valid = slices.Grow(r.valid, n)[:len(r.valid)+n] @@ -814,7 +954,7 @@ func (r *rowSet[T]) nameDuplicates() { for { suffix++ - instanceName = name + "#" + strconv.Itoa(suffix) + instanceName = r.nameCache.suffixed(name, suffix) if _, literalName := r.occurrences[instanceName]; !literalName { break @@ -866,12 +1006,20 @@ func (r *rowSet[T]) finish() { continue } - rows[kept] = rows[row] + if row != kept { + rows[kept] = rows[row] + + // Do not leave two rows that share a Row.Values array. + var zero T + + rows[row] = zero + } + kept++ } if dropped := len(rows) - kept; dropped > 0 { - r.c.logger.Debug("omitting instances without a valid value for every counter", + r.c.logger.LogAttrs(context.Background(), slog.LevelDebug, "omitting instances without a valid value for every counter", slog.String("object", r.c.object), slog.Int("instances", dropped), ) @@ -944,8 +1092,11 @@ func formatCounterPath(object, instance, counterName string) string { // Names that a sample did not decode are removed at its end, because instance names that // contain process IDs, like those of GPU Engine, change all the time. type instanceNameCache struct { - names map[string]*cachedInstanceName - sample uint64 + names map[string]*cachedInstanceName + // suffixedNames maps a name and an occurrence suffix to the name with + // the suffix, e.g. svchost and 3 to svchost#3. + suffixedNames map[instanceKey]*cachedInstanceName + sample uint64 } type cachedInstanceName struct { @@ -989,22 +1140,50 @@ func (cache *instanceNameCache) decode(p *uint16) string { return name } -// removeUnseen removes the names that the current sample did not decode. +// suffixed returns name with the occurrence suffix #suffix. Duplicate names, +// like those of the many svchost processes, need the same suffixed names in +// every sample. +func (cache *instanceNameCache) suffixed(name string, suffix int) string { + key := instanceKey{name: name, occurrence: suffix} + + if cached, ok := cache.suffixedNames[key]; ok { + cached.sample = cache.sample + + return cached.name + } + + if cache.suffixedNames == nil { + cache.suffixedNames = map[instanceKey]*cachedInstanceName{} + } + + suffixedName := name + "#" + strconv.Itoa(suffix) + cache.suffixedNames[key] = &cachedInstanceName{name: suffixedName, sample: cache.sample} + + return suffixedName +} + +// removeUnseen removes the names that the current sample did not use. func (cache *instanceNameCache) removeUnseen() { for raw, cached := range cache.names { if cached.sample != cache.sample { delete(cache.names, raw) } } + + for key, cached := range cache.suffixedNames { + if cached.sample != cache.sample { + delete(cache.suffixedNames, key) + } + } } -func isKnownCounterDataError(err error) bool { - var pdhErr *Error - - return errors.As(err, &pdhErr) && (pdhErr.ErrorCode == InvalidData || - pdhErr.ErrorCode == CalcNegativeDenominator || - pdhErr.ErrorCode == CalcNegativeValue || - pdhErr.ErrorCode == CstatusInvalidData || - pdhErr.ErrorCode == CstatusNoInstance || - pdhErr.ErrorCode == NoData) +// isKnownCounterDataStatus reports whether status means that a counter has no +// valid data in a sample, which is not a collection error. +func isKnownCounterDataStatus(status uint32) bool { + switch status { + case InvalidData, CalcNegativeDenominator, CalcNegativeValue, CstatusInvalidData, CstatusNoInstance, NoData: + return true + default: + return false + } } diff --git a/internal/pdh/collector_internal_test.go b/internal/pdh/collector_internal_test.go index eb13e9f5..7c5d1ab2 100644 --- a/internal/pdh/collector_internal_test.go +++ b/internal/pdh/collector_internal_test.go @@ -43,25 +43,18 @@ type rowSetValues struct { // newRowSetTestCollector returns a collector with the counters A and B, which // have instances, and Optional, which was skipped at Build and has none. func newRowSetTestCollector(partialRows bool) *Collector[rowSetValues] { + var row rowSetValues + return &Collector[rowSetValues]{ partialRows: partialRows, logger: slog.New(slog.DiscardHandler), - rows: rowAccessor[rowSetValues]{ - newRow: func(instance string) rowSetValues { - return rowSetValues{Name: instance} - }, - setName: func(row *rowSetValues, instance string) { - row.Name = instance - }, - setValue: func(row *rowSetValues, field int, value float64) { - reflect.ValueOf(row).Elem().Field(field).SetFloat(value) - }, - }, + rows: structRowAccessor[rowSetValues](int(unsafe.Offsetof(row.Name))), counters: []Counter{ - {Name: "A", Instances: map[string]pdhCounterHandle{"*": 1}, FieldIndexValue: 1, FieldIndexSecondValue: -1}, - {Name: "B", Instances: map[string]pdhCounterHandle{"*": 2}, FieldIndexValue: 2, FieldIndexSecondValue: 3}, - {Name: "Optional", Instances: map[string]pdhCounterHandle{}, FieldIndexValue: 4, FieldIndexSecondValue: -1}, + {Name: "A", Instances: map[string]pdhCounterHandle{"*": 1}, FieldIndexValue: int(unsafe.Offsetof(row.A)), FieldIndexSecondValue: -1}, + {Name: "B", Instances: map[string]pdhCounterHandle{"*": 2}, FieldIndexValue: int(unsafe.Offsetof(row.B)), FieldIndexSecondValue: int(unsafe.Offsetof(row.BSecond))}, + {Name: "Optional", Instances: map[string]pdhCounterHandle{}, FieldIndexValue: int(unsafe.Offsetof(row.Optional)), FieldIndexSecondValue: -1}, }, + counterIndex: map[string]int{"A": 0, "B": 1, "Optional": 2}, } } @@ -348,7 +341,7 @@ func TestSetRawValueElapsedTime(t *testing.T) { var values rowSetValues - counter := &Counter{Type: PERF_ELAPSED_TIME, Frequency: 10_000_000, FieldIndexValue: 1, FieldIndexSecondValue: -1} + counter := &Counter{Type: PERF_ELAPSED_TIME, Frequency: 10_000_000, FieldIndexValue: int(unsafe.Offsetof(values.A)), FieldIndexSecondValue: -1} // 2.5 s at 10 MHz. Integer division truncated this to 2. require.True(t, c.setRawValue(&values, counter, RawCounter{FirstValue: 100_000_000, SecondValue: 125_000_000})) @@ -359,7 +352,7 @@ func TestSetRawValueElapsedTime(t *testing.T) { require.False(t, c.setRawValue(&values, counter, RawCounter{FirstValue: 1, SecondValue: 2})) // A counter with only a second value field must not set field -1. - counter = &Counter{Type: PERF_ELAPSED_TIME, Frequency: 1, FieldIndexValue: -1, FieldIndexSecondValue: 3} + counter = &Counter{Type: PERF_ELAPSED_TIME, Frequency: 1, FieldIndexValue: -1, FieldIndexSecondValue: int(unsafe.Offsetof(values.BSecond))} require.True(t, c.setRawValue(&values, counter, RawCounter{FirstValue: 1, SecondValue: 2})) } @@ -474,17 +467,14 @@ func TestCollectRecoversPanic(t *testing.T) { require.NoError(t, c.Collect(&dst)) - // Point the counter at a field that does not exist, so that reflect panics. - c.mu.Lock() - c.counters[0].FieldIndexValue = 42 - c.mu.Unlock() + setValue := setPanicOnSetValue(c) err = c.Collect(&dst) require.ErrorContains(t, err, "panic while collecting performance counters of Process") // Collection still works after the panic. c.mu.Lock() - c.counters[0].FieldIndexValue = 1 + c.rows.setValue = setValue c.mu.Unlock() require.NoError(t, c.Collect(&dst)) @@ -515,19 +505,23 @@ func TestCollectKeepsInstanceNames(t *testing.T) { require.Same(t, unsafe.StringData(systemName(first)), unsafe.StringData(systemName(second)), "name decoded again") // Assertions run without holding the lock, because a failed assertion would leave it locked for Close. - cachedNames := func(fieldIndex int) int { + setValue := setPanicOnSetValue(c) + + cachedNames := func() int { c.mu.Lock() defer c.mu.Unlock() - c.counters[0].FieldIndexValue = fieldIndex - return len(c.state.names.names) } - // Point the counter at a field that does not exist, so that the first item panics. - require.Greater(t, cachedNames(42), 1) + // Let the first item panic. + require.Greater(t, cachedNames(), 1) require.Error(t, c.Collect(&first)) - require.Equal(t, 1, cachedNames(1), "names not reached by the failed sample kept") + require.Equal(t, 1, cachedNames(), "names not reached by the failed sample kept") + + c.mu.Lock() + c.rows.setValue = setValue + c.mu.Unlock() require.NoError(t, c.Collect(&first)) require.Equal(t, systemName(second), systemName(first)) @@ -597,3 +591,15 @@ func TestRowSetReusesState(t *testing.T) { }, dst) } } + +// setPanicOnSetValue lets every value that c sets panic and returns the +// previous setter. +func setPanicOnSetValue[T any](c *Collector[T]) func(row *T, field int, value float64) { + c.mu.Lock() + defer c.mu.Unlock() + + setValue := c.rows.setValue + c.rows.setValue = func(*T, int, float64) { panic("test panic") } + + return setValue +} diff --git a/internal/pdh/collector_rows_internal_test.go b/internal/pdh/collector_rows_internal_test.go new file mode 100644 index 00000000..db187db7 --- /dev/null +++ b/internal/pdh/collector_rows_internal_test.go @@ -0,0 +1,346 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Copyright The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build windows + +package pdh + +import ( + "log/slog" + "reflect" + "runtime" + "testing" + "unsafe" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +type embeddedCounters struct { + Name string + Inner float64 `perfdata:"Inner"` +} + +type pointerCounters struct { + Pointer float64 `perfdata:"Pointer"` +} + +type structFieldsValues struct { + _ int32 // Gives the embedded struct a non-zero offset. + embeddedCounters //nolint:embeddedstructfieldcheck // Tests the offset of a promoted field. + *pointerCounters + + Outer float64 `perfdata:"Outer"` + Base float64 `perfdata:"Outer,secondvalue"` + Invalid int64 `perfdata:"Invalid"` +} + +// TestStructFields checks that the counter fields of a struct, including +// fields promoted from an embedded struct, are written at their offsets. +func TestStructFields(t *testing.T) { + t.Parallel() + + fields, rows, errs, err := structFields[structFieldsValues]() + require.NoError(t, err) + + require.Len(t, errs, 2) + require.ErrorContains(t, errs[0], "field Pointer must not be promoted through an embedded pointer") + require.ErrorContains(t, errs[1], "field Invalid must be a float64") + + offsets := make(map[string]int, len(fields)) + for _, f := range fields { + offsets[f.tag] = f.index + } + + require.Len(t, offsets, 3) + + var row structFieldsValues + + row.Invalid = 42 + rows.resetRow(&row, "instance") + require.Equal(t, structFieldsValues{Name: "instance"}, row) + + rows.setValue(&row, offsets["Inner"], 1) + rows.setValue(&row, offsets["Outer"], 2) + rows.setValue(&row, offsets["Outer,secondvalue"], 3) + rows.setName(&row, "renamed") + + require.Equal(t, structFieldsValues{ + Name: "renamed", + Inner: 1, + Outer: 2, + Base: 3, + }, row) + + _, _, _, err = structFields[float64]() + require.ErrorContains(t, err, "expected a struct, got float64") +} + +// TestStructFieldsIndexIsByteOffset checks that the index of a counter field +// is the byte offset that structRowAccessor writes to, not a field number. +func TestStructFieldsIndexIsByteOffset(t *testing.T) { + t.Parallel() + + type values struct { + Name string + A float64 `perfdata:"A"` + B float64 `perfdata:"B"` + C float64 `perfdata:"B,secondvalue"` + } + + fields, _, errs, err := structFields[values]() + require.NoError(t, err) + require.Empty(t, errs) + + var row values + + require.Equal(t, []int{ + int(unsafe.Offsetof(row.A)), + int(unsafe.Offsetof(row.B)), + int(unsafe.Offsetof(row.C)), + }, []int{fields[0].index, fields[1].index, fields[2].index}) +} + +// TestValidateFloatOffsets checks that field numbers or other indexes that do +// not address a float64 field are rejected before they are written through. +func TestValidateFloatOffsets(t *testing.T) { + t.Parallel() + + type values struct { + Name string + A float64 `perfdata:"A"` + B float64 `perfdata:"B"` + C float64 `perfdata:"C"` + } + + valueType := reflect.TypeFor[values]() + + var row values + + valid := []counterField{ + {tag: "A", index: int(unsafe.Offsetof(row.A))}, + {tag: "C", index: int(unsafe.Offsetof(row.C))}, + } + require.NoError(t, validateFloatOffsets(valueType, valid)) + + // Field numbers, as reflect.StructField.Index[0] would give them. + fieldNumbers := make([]counterField, 0, 3) + + for _, f := range reflect.VisibleFields(valueType) { + if tag, ok := f.Tag.Lookup("perfdata"); ok { + fieldNumbers = append(fieldNumbers, counterField{tag: tag, index: f.Index[0]}) + } + } + + require.ErrorContains(t, validateFloatOffsets(valueType, fieldNumbers), "is not the byte offset of a float64 field") + + for _, index := range []int{ + int(unsafe.Offsetof(row.Name)), // a string, not a float64 + int(unsafe.Offsetof(row.A)) + 4, // misaligned + int(unsafe.Sizeof(row)), // past the end + int(unsafe.Sizeof(row)) - 4, // overlaps the end + -8, // before the start + } { + require.Error(t, validateFloatOffsets(valueType, []counterField{{tag: "X", index: index}}), "index %d", index) + } +} + +// TestStructRowAccessorWithoutName checks that a struct without a Name field +// only gets its values. +func TestStructRowAccessorWithoutName(t *testing.T) { + t.Parallel() + + type values struct { + Value float64 `perfdata:"Value"` + } + + fields, rows, errs, err := structFields[values]() + require.NoError(t, err) + require.Empty(t, errs) + require.Len(t, fields, 1) + + row := values{Value: 1} + rows.resetRow(&row, "instance") + rows.setName(&row, "instance") + require.Equal(t, values{}, row) + + rows.setValue(&row, fields[0].index, 2) + require.Equal(t, values{Value: 2}, row) +} + +// TestDynamicRowAccessor checks that a reused Row keeps a large enough Values +// array and that no value of an earlier sample remains. +func TestDynamicRowAccessor(t *testing.T) { + t.Parallel() + + rows := dynamicRowAccessor(2) + + values := []float64{1, 2, 3} + row := Row{Name: "old", Values: values} + + rows.resetRow(&row, "new") + require.Equal(t, Row{Name: "new", Values: []float64{0, 0}}, row) + require.Same(t, unsafe.SliceData(values), unsafe.SliceData(row.Values), "Values allocated again") + + rows.setValue(&row, 1, 4) + require.Equal(t, []float64{0, 4}, row.Values) + + row = Row{Name: "old", Values: []float64{1}} + rows.resetRow(&row, "new") + require.Equal(t, Row{Name: "new", Values: []float64{0, 0}}, row) +} + +// TestRowSetReusesRows checks that a sample reuses the backing array of dst +// and that no value of an earlier sample remains in a reused row. +func TestRowSetReusesRows(t *testing.T) { + t.Parallel() + + c := newRowSetTestCollector(false) + c.counters[1].FieldIndexSecondValue = -1 + + dst := make([]rowSetValues, 3, 4) + for i := range dst { + dst[i] = rowSetValues{Name: "stale", A: 9, B: 9, BSecond: 9, Optional: 9} + } + + backing := unsafe.SliceData(dst) + dst = dst[:0] + + rows := newRowSet(c, &dst, &collectState{}) + addDuplicateTestItems(&rows, CounterTypeRaw, 0, []string{"b", "c"}, []float64{4, 5}) + addDuplicateTestItems(&rows, CounterTypeRaw, 1, []string{"b", "c"}, []float64{6, 7}) + rows.finish() + + require.Same(t, backing, unsafe.SliceData(dst), "rows allocated again") + require.Equal(t, []rowSetValues{ + {Name: "b", A: 4, B: 6}, + {Name: "c", A: 5, B: 7}, + }, dst) +} + +// TestRowSetSampleDoesNotAllocate checks that a sample of known instances, +// including duplicate names, into a reused slice does not allocate. +func TestRowSetSampleDoesNotAllocate(t *testing.T) { + c := newRowSetTestCollector(true) + + const instances = 100 + + items := make([]RawCounterItem, instances) + for i := range items { + items[i] = RawCounterItem{ + // Every name occurs four times, like svchost. + SzName: windows.StringToUTF16Ptr("instance_" + string(rune('A'+i%25))), + RawValue: RawCounter{CStatus: CstatusValidData, FirstValue: int64(i)}, + } + } + + buf := unsafe.Slice((*byte)(unsafe.Pointer(unsafe.SliceData(items))), len(items)*int(unsafe.Sizeof(RawCounterItem{}))) + state := &collectState{} + + var dst []rowSetValues + + sample := func() { + dst = dst[:0] + rows := newRowSet(c, &dst, state) + + rows.addRawItems(0, buf, instances) + rows.addRawItems(1, buf, instances) + rows.finish() + state.names.removeUnseen() + + state.valid = rows.valid + } + + sample() + + require.Len(t, dst, instances) + require.Equal(t, "instance_A#3", dst[75].Name) + require.Zero(t, testing.AllocsPerRun(10, sample)) + + runtime.KeepAlive(items) +} + +// TestInstanceNameCacheSuffixed checks that suffixed names are built once +// while their instances exist. +func TestInstanceNameCacheSuffixed(t *testing.T) { + t.Parallel() + + var cache instanceNameCache + + cache.startSample() + + first := cache.suffixed("svchost", 2) + require.Equal(t, "svchost#2", first) + require.Equal(t, "svchost#3", cache.suffixed("svchost", 3)) + cache.removeUnseen() + + cache.startSample() + + second := cache.suffixed("svchost", 2) + require.Same(t, unsafe.StringData(first), unsafe.StringData(second), "name built again") + cache.removeUnseen() + + require.Len(t, cache.suffixedNames, 1, "name of a vanished instance kept") + require.Contains(t, cache.suffixedNames, instanceKey{name: "svchost", occurrence: 2}) +} + +// TestCollectReusesRows checks that Collect keeps the backing array of dst and +// releases the rows a sample does not use. +func TestCollectReusesRows(t *testing.T) { + t.Parallel() + + type processor struct { + Name string + ProcessorTime float64 `perfdata:"% Processor Time"` + } + + c, err := NewCollector[processor](slog.New(slog.DiscardHandler), CounterTypeRaw, "Processor", InstancesAll) + require.NoError(t, err) + + t.Cleanup(c.Close) + + dst := make([]processor, 1024) + for i := range dst { + dst[i] = processor{Name: "stale", ProcessorTime: -1} + } + + backing := unsafe.SliceData(dst) + + require.NoError(t, c.Collect(&dst)) + require.NotEmpty(t, dst) + require.Less(t, len(dst), 1024) + require.Same(t, backing, unsafe.SliceData(dst), "rows allocated again") + + for _, row := range dst { + require.NotEqual(t, "stale", row.Name) + require.GreaterOrEqual(t, row.ProcessorTime, 0.0) + } + + for _, row := range dst[len(dst):cap(dst)] { + require.Equal(t, processor{}, row, "row of an earlier sample kept") + } +} + +func TestIsKnownCounterDataStatus(t *testing.T) { + t.Parallel() + + for _, status := range []uint32{InvalidData, CalcNegativeDenominator, CalcNegativeValue, CstatusInvalidData, CstatusNoInstance, NoData} { + require.True(t, isKnownCounterDataStatus(status), "0x%08X", status) + } + + for _, status := range []uint32{ErrorSuccess, MoreData, CstatusNoObject, CstatusNoCounter, InvalidHandle} { + require.False(t, isKnownCounterDataStatus(status), "0x%08X", status) + } +} diff --git a/internal/pdh/collector_rowset_bench_test.go b/internal/pdh/collector_rowset_bench_test.go index c19d82ad..5a1db4b7 100644 --- a/internal/pdh/collector_rowset_bench_test.go +++ b/internal/pdh/collector_rowset_bench_test.go @@ -19,10 +19,13 @@ package pdh import ( "fmt" + "log/slog" "runtime" + "strings" "testing" "unsafe" + "github.com/prometheus/client_golang/prometheus" "golang.org/x/sys/windows" ) @@ -49,7 +52,7 @@ func BenchmarkRowSetInstanceNames(b *testing.B) { b.ReportAllocs() for b.Loop() { - dst = dst[:0:0] + dst = dst[:0] rows := newRowSet(c, &dst, state) rows.addRawItems(0, buf, instances) @@ -62,3 +65,134 @@ func BenchmarkRowSetInstanceNames(b *testing.B) { runtime.KeepAlive(items) } + +// processShape has the shape of the Process object: one row per process with +// 20 counters, two of which also use their second value. +type processShape struct { + Name string + + C00 float64 `perfdata:"C00"` + C01 float64 `perfdata:"C01"` + C02 float64 `perfdata:"C02"` + C03 float64 `perfdata:"C03"` + C04 float64 `perfdata:"C04"` + C05 float64 `perfdata:"C05"` + C06 float64 `perfdata:"C06"` + C07 float64 `perfdata:"C07"` + C08 float64 `perfdata:"C08"` + C09 float64 `perfdata:"C09"` + C10 float64 `perfdata:"C10"` + C11 float64 `perfdata:"C11"` + C12 float64 `perfdata:"C12"` + C13 float64 `perfdata:"C13"` + C14 float64 `perfdata:"C14"` + C15 float64 `perfdata:"C15"` + C16 float64 `perfdata:"C16"` + C17 float64 `perfdata:"C17"` + C18 float64 `perfdata:"C18"` + C19 float64 `perfdata:"C19"` + B00 float64 `perfdata:"C00,secondvalue"` + B01 float64 `perfdata:"C01,secondvalue"` +} + +// newProcessShapeCollector returns a collector of processShape rows without +// a PDH query. +func newProcessShapeCollector(b *testing.B) *Collector[processShape] { + b.Helper() + + fields, rows, errs, err := structFields[processShape]() + if err != nil || len(errs) != 0 { + b.Fatal(err, errs) + } + + c := &Collector[processShape]{ + logger: slog.New(slog.DiscardHandler), + rows: rows, + counterIndex: map[string]int{}, + } + + for _, f := range fields { + counterName, secondValue := strings.CutSuffix(f.tag, ",secondvalue") + + i, ok := c.counterIndex[counterName] + if !ok { + i = len(c.counters) + c.counterIndex[counterName] = i + c.counters = append(c.counters, Counter{ + Name: counterName, + MetricType: prometheus.CounterValue, + Instances: map[string]pdhCounterHandle{"*": pdhCounterHandle(i + 1)}, + FieldIndexValue: -1, + FieldIndexSecondValue: -1, + }) + } + + if secondValue { + c.counters[i].FieldIndexSecondValue = f.index + } else { + c.counters[i].FieldIndexValue = f.index + } + } + + return c +} + +// BenchmarkRowSetProcessShape measures the work of one Collect of an object +// shaped like Process, 500 instances with 20 counters, apart from the PDH +// calls. Like Collect, it reuses dst between samples. +func BenchmarkRowSetProcessShape(b *testing.B) { + const instances = 500 + + c := newProcessShapeCollector(b) + + items := make([]RawCounterItem, instances) + for i := range items { + items[i] = RawCounterItem{ + SzName: windows.StringToUTF16Ptr(fmt.Sprintf("process_%d", i)), + RawValue: RawCounter{CStatus: CstatusValidData, FirstValue: int64(i), SecondValue: int64(2 * i)}, + } + } + + buf := unsafe.Slice((*byte)(unsafe.Pointer(unsafe.SliceData(items))), len(items)*int(unsafe.Sizeof(RawCounterItem{}))) + state := &collectState{} + + var dst []processShape + + b.ReportAllocs() + + for b.Loop() { + dst = dst[:0] + rows := newRowSet(c, &dst, state) + + for counterIndex := range c.counters { + rows.addRawItems(counterIndex, buf, instances) + } + + rows.finish() + state.names.removeUnseen() + + state.valid = rows.valid + } + + if len(dst) != instances { + b.Fatalf("got %d rows, want %d", len(dst), instances) + } + + runtime.KeepAlive(items) +} + +// BenchmarkCollectorMetricType measures the lookup of the metric type of +// every counter, as the performancecounter collector does for every scrape. +func BenchmarkCollectorMetricType(b *testing.B) { + c := newProcessShapeCollector(b) + + b.ReportAllocs() + + for b.Loop() { + for i := range c.counters { + if _, ok := c.MetricType(c.counters[len(c.counters)-1-i].Name); !ok { + b.Fatal("unknown counter") + } + } + } +} diff --git a/internal/pdh/pdh.go b/internal/pdh/pdh.go index c77615e5..752008fa 100644 --- a/internal/pdh/pdh.go +++ b/internal/pdh/pdh.go @@ -50,6 +50,7 @@ package pdh import ( "fmt" + "syscall" "time" "unsafe" @@ -472,8 +473,10 @@ func GetFormattedCounterValueDouble(hCounter pdhCounterHandle, lpdwType *uint32, // time.Sleep(2000 * time.Millisecond) // } // } +// +// Like [GetRawCounterArray], it uses syscall.SyscallN to keep the sizes on the stack. func GetFormattedCounterArrayDouble(hCounter pdhCounterHandle, lpdwBufferSize *uint32, lpdwBufferCount *uint32, itemBuffer *byte) uint32 { - ret, _, _ := pdhGetFormattedCounterArrayW.Call( + ret, _, _ := syscall.SyscallN(pdhGetFormattedCounterArrayW.Addr(), uintptr(hCounter), uintptr(FmtDouble), uintptr(unsafe.Pointer(lpdwBufferSize)), @@ -631,8 +634,11 @@ func GetRawCounterValue(hCounter pdhCounterHandle, lpdwType *uint32, pValue *Raw // ItemBuffer // Caller-allocated buffer that receives the array of RawCounterItem structures; the structures contain the raw instance counter values. // Set to NULL if lpdwBufferSize is zero. +// +// It is called for every counter in every sample. syscall.SyscallN, unlike +// LazyProc.Call, does not move the pointed-to sizes to the heap. func GetRawCounterArray(hCounter pdhCounterHandle, lpdwBufferSize *uint32, lpdwBufferCount *uint32, itemBuffer *byte) uint32 { - ret, _, _ := pdhGetRawCounterArrayW.Call( + ret, _, _ := syscall.SyscallN(pdhGetRawCounterArrayW.Addr(), uintptr(hCounter), uintptr(unsafe.Pointer(lpdwBufferSize)), uintptr(unsafe.Pointer(lpdwBufferCount)),