Repository navigation
fix: process start time from /proc/<pid>/stat instead of taskstats (B8 of #369) - #379
mayankpande88 wants to merge 4 commits into
Conversation
…atsPID (cherry picked from commit 27bc75dedc2ad64e6beaee57c35924bba383c290) Conflict: this fork's NewProcess has no tracer or instrumentDone field. Its pid-reuse check in onProcessStart (#353) now compares the start time from /proc/<pid>/stat, which has the same one-second resolution as the taskstats BeginTime it replaces.
(cherry picked from commit 8c147002075082d60b91a0554a219f12d9ce8db0)
…okup (cherry picked from commit a9fd10174a90f5ea515896a2e5c0f0586dc01701)
Start ticks from field 22 of /proc/<pid>/stat plus the boot time, a comm containing spaces and ')', a missing pid and an unknown boot time.
There was a problem hiding this comment.
Code Review
This pull request replaces the dependency on taskstats for retrieving process start times with a custom implementation that reads and parses /proc//stat directly. It introduces proc.GetStartTime and initializes bootTime from /proc/stat, accompanied by unit tests. Feedback suggests optimizing GetStartTime to scan the byte slice directly instead of converting the file to a string and splitting it, which would eliminate allocations and reduce overhead in high-churn environments.
| func GetStartTime(pid uint32) time.Time { | ||
| data, err := os.ReadFile(Path(pid, "stat")) | ||
| if err != nil { | ||
| return time.Time{} | ||
| } | ||
| s := string(data) | ||
| idx := strings.LastIndex(s, ")") | ||
| if idx < 0 { | ||
| return time.Time{} | ||
| } | ||
| fields := strings.Fields(s[idx+1:]) | ||
| if len(fields) < 20 { | ||
| return time.Time{} | ||
| } | ||
| startTicks, err := strconv.ParseUint(fields[19], 10, 64) | ||
| if err != nil || bootTime == 0 { | ||
| return time.Time{} | ||
| } | ||
| return time.Unix(bootTime+int64(float64(startTicks)/100), 0) | ||
| } |
There was a problem hiding this comment.
The current implementation of GetStartTime converts the entire /proc/<pid>/stat file to a string and splits it into fields using strings.Fields. Since /proc/<pid>/stat contains around 50 fields, this approach triggers dozens of string allocations on every process start.
Because GetStartTime is executed on every process start event, optimizing it to be allocation-free significantly reduces CPU and memory overhead in high-churn environments.
We can achieve this by scanning the byte slice directly to find the 20th field after the last ) character, and parsing the uint64 directly from the bytes without any string conversions or allocations.
func GetStartTime(pid uint32) time.Time {
data, err := os.ReadFile(Path(pid, "stat"))
if err != nil {
return time.Time{}
}
idx := bytes.LastIndex(data, []byte{')'})
if idx < 0 {
return time.Time{}
}
tail := data[idx+1:]
fieldCount := 0
inField := false
start, end := -1, -1
for i, b := range tail {
if b == ' ' || b == '\t' || b == '\n' || b == '\r' {
if inField {
inField = false
if fieldCount == 20 {
end = i
break
}
}
} else {
if !inField {
inField = true
fieldCount++
if fieldCount == 20 {
start = i
}
}
}
}
if inField && fieldCount == 20 {
end = len(tail)
}
if start < 0 || end < 0 {
return time.Time{}
}
var startTicks uint64
for _, b := range tail[start:end] {
if b < '0' || b > '9' {
return time.Time{}
}
startTicks = startTicks*10 + uint64(b-'0')
}
if bootTime == 0 {
return time.Time{}
}
return time.Unix(bootTime+int64(startTicks/100), 0)
}There was a problem hiding this comment.
I measured this, and I'm not changing it.
strings.Fieldsdoesn't allocate per field: the substrings share the original string's memory.- With
testing.AllocsPerRun,GetStartTimemakes 10 allocations per call. 8 of them come fromPathplusos.ReadFile, which the suggested version keeps. Parsing adds 2: thestring(data)copy and theFieldsslice. - So the hand-written scanner would save 2 small allocations per process start: about 200/s at 100 process starts per second. That isn't worth the extra parsing code.
- The change this replaces, a taskstats netlink call per process start, already cut agent CPU by about 14% in the e2e.
Summary
B8 of #369: process start times now come from
/proc/<pid>/statinstead of a taskstats netlink call, with the host boot time read once. Ported from upstream coroot-node-agent with-x.onProcessStarttakes the start time from/proc/<pid>/stat(start ticks + boot time) instead ofTaskstatsPID.init().Why it matters here:
TaskstatsPIDwas a netlink round trip, under a global lock, for every process start.onProcessStartreturned nil, so the process was never registered. Registration no longer depends on taskstats; per-process delay accounting (TaskstatsTGID) still does.Also adds a test for
GetStartTime, which upstream didn't have.Engineering detail
Conflict in 27bc75d: this fork's
NewProcesshas notracerorinstrumentDonefield. Its pid-reuse check inonProcessStart(#353) now compares the start time from/proc/<pid>/stat. That value is the same for every read of one process, and has the same one-second resolution as the taskstatsBeginTimeit replaces.Test:
)is parsed from the last);The fixture is a
statfile added to the existing fixture pid 123, soTestListPidsis unchanged.CI: gofmt, goimports, vet, golangci-lint,
go test(excluding/containers) and the build all pass in a Linux container with Go 1.26.5.Local e2e: I built agent binaries from this branch and from main and ran each as a systemd service on a local Debian 12 VM (kernel 6.1), one after the other with the same workload.
container_restarts_total= 2 on both builds.