libct/system.Stat: fix panic, speed up, add benchmarks #2696

kolyshkin · 2020-11-30T20:39:33Z

1. libct/system/proc_test: fix, improve, add benchmarks

Add a test case that tests parentheses in command.
Replace individual comparisons with reflect.DeepEqual.
This also fixes wrong %-style types used in Fatalf statements.
Replace Fatalf with Errorf so we don't bail out on the first
failure, and do not check result on error.
Add two benchmarks.

2. libct/system.Stat: fix/improve/speedup

Remove PID field. It is totally useless -- we already know the PID.
Rewrite parseStat() to make it faster and more correct:
- do not use fmt.Scanf as it is very slow;
- avoid splitting data into 20+ fields, of which we only need 2 (should also help garbage collector);
- make sure to not panic on short lines and other bad input;
- add some bad input tests (some fail with old code);
- use LastIndexByte instead of LastIndex where appropriate.

Benchmarks:

before (from the previous commit message):

BenchmarkParseStat-4              116415             10804 ns/op
BenchmarkParseRealStat-4             240           4781769 ns/op

after:

BenchmarkParseStat-4           1164948              1068 ns/op
BenchmarkParseRealStat-4           331           3458315 ns/op

We are seeing 10x speedup in a synthetic benchmark, and about 1.4x
speedup in a real world benchmark. All this while being more scrupulous
about any possible errors. I also suspect much less allocations and thus
garbage to collect.

While at it, do not ignore any possible errors, and properly wrap those.

Add I and P process states (available since Linux 4.14).

History

v2: use pkg/errors more, remove t.Logf from test
v3: rebased; drop pkg/errors; gofumpt'ed
v4: rebased; improved description
v5: rebased; mention bad input tests, added second benchmark results
v6: remove PID field, do not use strings.Split, further speedup.
v7: readability improvements, added new process states.
v8: improved parser to bail earier on incorrect data, more doc, more tests.

libcontainer/system/proc.go

kolyshkin · 2020-12-01T02:05:15Z

Rebased to benefit from #2690 (while Travis was not able to test this PR for 6+ hours 😢)

thaJeztah

LGTM

libcontainer/system/proc.go

kolyshkin · 2020-12-15T20:23:26Z

@AkihiroSuda @mrunalp PTAL

kolyshkin · 2021-01-06T22:55:46Z

rebased (for new CI)

thaJeztah

LGTM, but left some thoughts/suggestions for consideration

libcontainer/system/proc_test.go

libcontainer/system/proc.go

libcontainer/system/proc_test.go

cyphar · 2021-02-04T04:33:36Z

Is this a bugfix or an improvement? If it's an improvement it belongs in post-1.0 as a milestone.

kolyshkin · 2021-02-04T19:55:51Z

Is this a bugfix or an improvement? If it's an improvement it belongs in post-1.0 as a milestone.

Improvement (except eliminating a linter warning which might be classified as bugfix), and that's lots of code, so moving to post 1.0

cyphar · 2021-02-05T02:05:15Z

Yeah agreed, this change looks a little scary tbh.

kolyshkin · 2021-03-31T21:35:17Z

close/open to re-kick ci

kolyshkin · 2021-06-24T17:40:05Z

v3: rebased; drop using pkg/errors; gofumpt'ed

kolyshkin · 2021-06-29T20:38:25Z

CI failure is unrelated (#3050)

kolyshkin · 2021-07-12T00:36:50Z

Rebased to fix CI.

libcontainer/system/proc_test.go

kolyshkin · 2021-08-04T02:38:08Z

Some quite interesting observations:

removing strings.Split (which result in ~20 fields slice) and a couple of fmt.Scanfs results in 7x improvement!
removing strings.Split (for much shorter data, only the first two fields) results in further 2x improvement!

kolyshkin · 2021-08-19T02:31:42Z

@thaJeztah PTAL

thaJeztah

overall looks good; I was looking for corner-cases, and possibly found some (may not be important to add, but thought I'd post them)

I'm fine with merging as-is though

libcontainer/system/proc_test.go

1. Add a test case that tests parentheses in command. 2. Replace individual comparisons with reflect.DeepEqual. This also fixes wrong %-style types in Fatalf statements. 3. Replace Fatalf with Errorf so we don't bail out on the first failure, and do not check result on error. 4. Add two benchmarks. On my laptop, they show: BenchmarkParseStat-4 116415 10804 ns/op BenchmarkParseRealStat-4 240 4781769 ns/op Signed-off-by: Kir Kolyshkin <[email protected]>

1. Remove PID field as it is useless. 2. Rewrite parseStat() to make it faster and more correct: - do not use fmt.Scanf as it is very slow; - avoid splitting data into 20+ fields, of which we only need 2; - make sure to not panic on short lines and other bad input; - add some bad input tests (some fail with old code); - use LastIndexByte instead of LastIndex. Benchmarks: before (from the previous commit message): > BenchmarkParseStat-4 116415 10804 ns/op > BenchmarkParseRealStat-4 240 4781769 ns/op after: > BenchmarkParseStat-4 1164948 1068 ns/op > BenchmarkParseRealStat-4 331 3458315 ns/op We are seeing 10x speedup in a synthetic benchmark, and about 1.4x speedup in a real world benchmark. While at it, do not ignore any possible errors, and properly wrap those. [v2: use pkg/errors more, remove t.Logf from test] [v3: rebased; drop pkg/errors; gofumpt'ed] [v4: rebased; improved description] [v5: rebased; mention bad input tests, added second benchmark results] [v6: remove PID field, do not use strings.Split, further speedup] Signed-off-by: Kir Kolyshkin <[email protected]>

Those states are available since Linux 4.14 (kernel commits 8ef9925b02c23e3838d5 and 06eb61844d841d003). Before this patch, they were shown as unknown. This is mostly cosmetical. Note that I is described in /proc/pid/status as just "idle", although elsewhere it says it's an idle kernel thread. Let's have it as "idle" for brevity. Signed-off-by: Kir Kolyshkin <[email protected]>

thaJeztah

LGTM

cyphar · 2021-09-09T04:37:00Z

libcontainer/system/proc.go

+	// 1. Look for the first '(' and the last ')' first, what's in between is Name.
+	//    We expect at least 20 fields and a space after the last one.


Here's hoping that they don't add a new field in a few kernel versions that contains ) grimace. Why on earth doesn't /proc/self/status contain StartTime? If it did we could just use that rather than this unparseable crap. /rant

I heard they will switch to YAML, because it seems to be popular 🤪

cyphar

LGTM.

thaJeztah reviewed Nov 30, 2020

View reviewed changes

libcontainer/system/proc.go Outdated Show resolved Hide resolved

kolyshkin force-pushed the proc-stat branch from 4dbe8ca to ef6fd85 Compare December 1, 2020 02:04

kolyshkin force-pushed the proc-stat branch from ef6fd85 to 670a674 Compare December 1, 2020 03:27

thaJeztah previously approved these changes Dec 1, 2020

View reviewed changes

libcontainer/system/proc.go Outdated Show resolved Hide resolved

This was referenced Dec 3, 2020

Misc linter nits #2695

Merged

2627 fix "some of" golangci-lint problems #2641

Closed

Review and address golang-ci linting errors #2627

Closed

kolyshkin dismissed thaJeztah’s stale review via 9e9b3b1 January 6, 2021 22:55

kolyshkin force-pushed the proc-stat branch from 670a674 to 9e9b3b1 Compare January 6, 2021 22:55

thaJeztah previously approved these changes Jan 7, 2021

View reviewed changes

kolyshkin commented Jan 16, 2021

View reviewed changes

libcontainer/system/proc_test.go Outdated Show resolved Hide resolved

kolyshkin added this to the 1.0.0-rc94 milestone Feb 2, 2021

kolyshkin modified the milestones: 1.0.0-rc94, post-1.0 Feb 4, 2021

kolyshkin dismissed thaJeztah’s stale review via 1ca7e5c February 4, 2021 20:15

kolyshkin force-pushed the proc-stat branch 2 times, most recently from 1ca7e5c to 8d6a5d4 Compare February 4, 2021 20:18

kolyshkin closed this Mar 31, 2021

kolyshkin reopened this Mar 31, 2021

kolyshkin mentioned this pull request May 26, 2021

*: clean up remaining golangci-lint failures #2962

Merged

kolyshkin force-pushed the proc-stat branch from 8d6a5d4 to b304e3b Compare June 11, 2021 19:52

kolyshkin modified the milestones: post-1.0, 1.1.0 Jun 11, 2021

kolyshkin force-pushed the proc-stat branch from b304e3b to 21550bb Compare June 24, 2021 17:40

kolyshkin added kind/refactor refactoring kind/performance labels Jun 24, 2021

kolyshkin requested review from cyphar and AkihiroSuda June 29, 2021 04:13

kolyshkin changed the title ~~libct/system.Stat: improve and speed up~~ libct/system.Stat: fix panic, speed up, add benchmarks Jun 29, 2021

kolyshkin force-pushed the proc-stat branch from 21550bb to 9a1a545 Compare June 29, 2021 19:05

kolyshkin added the kind/bug label Jun 29, 2021

kolyshkin force-pushed the proc-stat branch from 9a1a545 to 0eda452 Compare July 12, 2021 00:36

kolyshkin force-pushed the proc-stat branch from 0eda452 to 82408b8 Compare July 28, 2021 03:15

thaJeztah reviewed Jul 29, 2021

View reviewed changes

libcontainer/system/proc_test.go Outdated Show resolved Hide resolved

kolyshkin force-pushed the proc-stat branch 2 times, most recently from f4716d3 to 89deda8 Compare August 4, 2021 01:02

kolyshkin force-pushed the proc-stat branch from 89deda8 to cd87853 Compare August 4, 2021 02:38

kolyshkin requested a review from thaJeztah August 19, 2021 02:31

thaJeztah reviewed Aug 19, 2021

View reviewed changes

libcontainer/system/proc_test.go Show resolved Hide resolved

kolyshkin added 3 commits August 19, 2021 19:46

kolyshkin force-pushed the proc-stat branch from cd87853 to a37a89f Compare August 20, 2021 02:49

thaJeztah approved these changes Aug 24, 2021

View reviewed changes

cyphar reviewed Sep 9, 2021

View reviewed changes

cyphar approved these changes Sep 9, 2021

View reviewed changes

cyphar merged commit 1e5fe26 into opencontainers:master Sep 9, 2021

thaJeztah mentioned this pull request Nov 5, 2022

replace strings.Split(N) for strings.Cut() or alternatives containerd/containerd#7631

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libct/system.Stat: fix panic, speed up, add benchmarks #2696

libct/system.Stat: fix panic, speed up, add benchmarks #2696

kolyshkin commented Nov 30, 2020 •

edited

Loading

kolyshkin commented Dec 1, 2020

thaJeztah left a comment

kolyshkin commented Dec 15, 2020

kolyshkin commented Jan 6, 2021

thaJeztah left a comment

cyphar commented Feb 4, 2021

kolyshkin commented Feb 4, 2021

cyphar commented Feb 5, 2021

kolyshkin commented Mar 31, 2021

kolyshkin commented Jun 24, 2021

kolyshkin commented Jun 29, 2021

kolyshkin commented Jul 12, 2021

kolyshkin commented Aug 4, 2021

kolyshkin commented Aug 19, 2021

thaJeztah left a comment

thaJeztah left a comment

cyphar Sep 9, 2021 •

edited

Loading

thaJeztah Sep 9, 2021

cyphar left a comment

		// 1. Look for the first '(' and the last ')' first, what's in between is Name.
		// We expect at least 20 fields and a space after the last one.

libct/system.Stat: fix panic, speed up, add benchmarks #2696

libct/system.Stat: fix panic, speed up, add benchmarks #2696

Conversation

kolyshkin commented Nov 30, 2020 • edited Loading

1. libct/system/proc_test: fix, improve, add benchmarks

2. libct/system.Stat: fix/improve/speedup

History

kolyshkin commented Dec 1, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented Dec 15, 2020

kolyshkin commented Jan 6, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

cyphar commented Feb 4, 2021

kolyshkin commented Feb 4, 2021

cyphar commented Feb 5, 2021

kolyshkin commented Mar 31, 2021

kolyshkin commented Jun 24, 2021

kolyshkin commented Jun 29, 2021

kolyshkin commented Jul 12, 2021

kolyshkin commented Aug 4, 2021

kolyshkin commented Aug 19, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

cyphar Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

thaJeztah Sep 9, 2021

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

kolyshkin commented Nov 30, 2020 •

edited

Loading

cyphar Sep 9, 2021 •

edited

Loading