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

optimize parser options check #4199

Merged
merged 3 commits into from
Aug 23, 2024
Merged

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Jul 23, 2024

1. Explain what the PR does

d7fa48d chore(parsers): add optionIsContainedInArgument
c830bf9 chore(parsers): opti optionsAreContainedInArgument
b15d559 chore(parsers): impr OptionsAreContainedInArgument

d7fa48d chore(parsers): add optionIsContainedInArgument

Currently all usages of the `optionsAreContainedInArgument` function
are using only one option. Then, `optionIsContainedInArgument` comes as
a simplification of the previous function.

Even though `optionsAreContainedInArgument` is not used anymore, it is
still being kept for now, since some parser might need to check for
multiple options in the future.

=== RUN   Benchmark_optionsAreContainedInArgumentWithOnlyOne
Benchmark_optionsAreContainedInArgumentWithOnlyOne
Benchmark_optionsAreContainedInArgumentWithOnlyOne-32            5000000               174.8 ns/op             0 B/op          0 allocs/op
=== RUN   Benchmark_optionIsContainedInArgument
Benchmark_optionIsContainedInArgument
Benchmark_optionIsContainedInArgument-32                         5000000               140.4 ns/op             0 B/op          0 allocs/op
PASS

c830bf9 chore(parsers): opti optionsAreContainedInArgument

optionsAreContainedInArgument now receives a variadic argument of type
uint64 instead of SystemFunctionArgument.

=== RUN   BenchmarkOptionsAreContainedInArgumentPrev
BenchmarkOptionsAreContainedInArgumentPrev
BenchmarkOptionsAreContainedInArgumentPrev-32          5000000               387.7 ns/op             0 B/op          0 allocs/op
=== RUN   BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument-32              5000000               152.3 ns/op             0 B/op

b15d559 chore(parsers): impr OptionsAreContainedInArgument

Optimize OptionsAreContainedInArgument function to return as soon as it
detects a non-contained option.

Running tool: /home/gg/.goenv/versions/1.22.4/bin/go test -benchmem
-run=^$ -tags ebpf
-bench ^(BenchmarkOptionsAreContainedInArgumentOld|BenchmarkOptionsAreContainedInArgument)$
github.com/aquasecurity/tracee/pkg/events/parsers -benchtime=1000000x -race

goos: linux
goarch: amd64
pkg: github.com/aquasecurity/tracee/pkg/events/parsers
cpu: AMD Ryzen 9 7950X 16-Core Processor
=== RUN   BenchmarkOptionsAreContainedInArgumentOld
BenchmarkOptionsAreContainedInArgumentOld
BenchmarkOptionsAreContainedInArgumentOld-32             1000000               500.4 ns/op             0 B/op          0 allocs/op
=== RUN   BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument-32                1000000               351.3 ns/op             0 B/op          0 allocs/op
PASS
ok      github.com/aquasecurity/tracee/pkg/events/parsers       1.861s

2. Explain how to test it

3. Other comments

@geyslan geyslan self-assigned this Jul 23, 2024
@geyslan geyslan marked this pull request as ready for review July 23, 2024 22:26
@geyslan geyslan force-pushed the optimize-parsers branch from 633a3d4 to 45cd736 Compare July 24, 2024 10:13
@geyslan geyslan changed the title chore(parsers): OptionsAreContainedInArgument optimize OptionsAreContainedInArgument Jul 24, 2024
@geyslan geyslan changed the title optimize OptionsAreContainedInArgument optimize parser options check Jul 24, 2024
@@ -1466,7 +1474,7 @@ func ParseMmapProt(rawValue uint64) MmapProtArgument {

var sb strings.Builder
for _, a := range mmapProtArgs {
if OptionAreContainedInArgument(rawValue, a) {
if optionIsContainedInArgument(rawValue, a.Value()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParseMmapProt already iterates its arguments. In the future, the loop approach can be applied to the other parser functions. Despite the improvement in CPU time the only criticism is that we need a global variable containing all arguments. But I believe this can be improved further by using pointers to existing declarations.

Optimize OptionsAreContainedInArgument function to return as soon as it
detects a non-contained option.

Running tool: /home/gg/.goenv/versions/1.22.4/bin/go test -benchmem
-run=^$ -tags ebpf
-bench ^(BenchmarkOptionsAreContainedInArgumentOld|BenchmarkOptionsAreContainedInArgument)$
github.com/aquasecurity/tracee/pkg/events/parsers -benchtime=1000000x -race

goos: linux
goarch: amd64
pkg: github.com/aquasecurity/tracee/pkg/events/parsers
cpu: AMD Ryzen 9 7950X 16-Core Processor
=== RUN   BenchmarkOptionsAreContainedInArgumentOld
BenchmarkOptionsAreContainedInArgumentOld
BenchmarkOptionsAreContainedInArgumentOld-32             1000000               500.4 ns/op             0 B/op          0 allocs/op
=== RUN   BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument-32                1000000               351.3 ns/op             0 B/op          0 allocs/op
PASS
ok      github.com/aquasecurity/tracee/pkg/events/parsers       1.861s
optionsAreContainedInArgument now receives a variadic argument of type
uint64 instead of SystemFunctionArgument.

=== RUN   BenchmarkOptionsAreContainedInArgumentPrev
BenchmarkOptionsAreContainedInArgumentPrev
BenchmarkOptionsAreContainedInArgumentPrev-32          5000000               387.7 ns/op             0 B/op          0 allocs/op
=== RUN   BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument
BenchmarkOptionsAreContainedInArgument-32              5000000               152.3 ns/op             0 B/op
Currently all usages of the `optionsAreContainedInArgument` function
are using only one option. Then, `optionIsContainedInArgument` comes as
a simplification of the previous function.

Even though `optionsAreContainedInArgument` is not used anymore, it is
still being kept for now, since some parser might need to check for
multiple options in the future.

=== RUN   Benchmark_optionsAreContainedInArgumentWithOnlyOne
Benchmark_optionsAreContainedInArgumentWithOnlyOne
Benchmark_optionsAreContainedInArgumentWithOnlyOne-32            5000000               174.8 ns/op             0 B/op          0 allocs/op
=== RUN   Benchmark_optionIsContainedInArgument
Benchmark_optionIsContainedInArgument
Benchmark_optionIsContainedInArgument-32                         5000000               140.4 ns/op             0 B/op          0 allocs/op
PASS
@geyslan geyslan requested a review from rscampos August 23, 2024 12:14
@rscampos
Copy link
Collaborator

double check the benchmarks and its aligned with yours:

impr OptionsAreContainedInArgument:

Benchmark_OptionsAreContainedInArgumentOld-4             5000000              1208 ns/op               0 B/op          0 allocs/op
Benchmark_OptionsAreContainedInArgument-4                5000000               730.6 ns/op             0 B/op          0 allocs/op

opti optionsAreContainedInArgument:

Benchmark_OptionsAreContainedInArgument-4                5000000               732.3 ns/op             0 B/op          0 allocs/op
Benchmark_OptionsAreContainedInArgumentuint64-4          5000000               259.9 ns/op             0 B/op          0 allocs/op

add optionIsContainedInArgument:

Benchmark_optionsAreContainedInArgumentWithOnlyOne-4	5000000	       321.5 ns/op	       0 B/op	       0 allocs/op
Benchmark_optionIsContainedInArgument-4			5000000	       223.0 ns/op	       0 B/op	       0 allocs/op

Copy link
Collaborator

@rscampos rscampos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@geyslan geyslan merged commit 4aaab1d into aquasecurity:main Aug 23, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants