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

maglev: Allocate permutations slice ahead of time #14622

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Jan 15, 2021

Previously, the Maglev permutations slice is allocated inside
getPermutation(), which means every time maglev.GetLookupTable() is
called, the slice will be recreated. That means it would recreated on
every Service creation or update.

This commit allocates the slice ahead of time when the Maglev subsystem
is initialized (maglev.Init()). This gives a rough improvement in time
of around 15%.

The slice is allocated based on a heuristic computation, involving the M
size. Nodes running with less than 8GB of RAM do not apply and Cilium
will not be preallocate the slice ahead of time. For nodes with more
than the aforementioned threshold, the formula for the heuristic is:

(M / 100) * 100

This is derived from the maximum backends property from Maglev where
|backends| * 100 < M. This gives the following memory pressure profile
based on M (left-hand side):

251:    0.004806594848632812 MB
509:    0.019766311645507812 MB
1021:   0.07953193664550783 MB
2039:   0.3171936798095703 MB
4093:   1.2781256866455077 MB
8191:   5.118750076293945 MB
16381:  20.472500686645507 MB
32749:  81.82502754211426 MB
65521:  327.5300171661377 MB
131071: 1310.700000076294 MB

If the user has more backends than the preallocated size, we will adjust
the allocation to be |backends| * M. This is because we want to ensure
that the Maglev subsystem isn't thrashing trying to reallocate the slice
on each Service create or update.

Benchmark timings:

Before

$ go test -v ./pkg/maglev -check.v -check.b -check.btime 5s -check.bmem
=== RUN   Test
PASS: maglev_test.go:91: MaglevTestSuite.BenchmarkGetMaglevTable              50         341255883 ns/op1049633800 B/op        12 allocs/op
OK: 1 passed
--- PASS: Test (17.42s)
PASS
ok      github.com/cilium/cilium/pkg/maglev     17.475s

After

$ go test -v ./pkg/maglev -check.v -check.b -check.btime 5s -check.bmem
=== RUN   Test
PASS: maglev_test.go:91: MaglevTestSuite.BenchmarkGetMaglevTable              50         282792516 ns/op 1057899 B/op          11 allocs/op
OK: 1 passed
--- PASS: Test (17.89s)
PASS
ok      github.com/cilium/cilium/pkg/maglev     17.943s

Suggested-by: Martynas Pumputis m@lambda.lt

FlameGraphs:

Before
After

Related: #14397

@christarazi christarazi added kind/performance There is a performance impact of this. needs-backport/1.9 release-note/misc This PR makes changes that have no direct user impact. labels Jan 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.2 Jan 15, 2021
@christarazi christarazi requested a review from brb January 15, 2021 00:56
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Nice! Some comments.

pkg/maglev/maglev.go Outdated Show resolved Hide resolved
pkg/maglev/maglev.go Show resolved Hide resolved
pkg/maglev/maglev.go Outdated Show resolved Hide resolved
@christarazi
Copy link
Member Author

test-me-please

@christarazi christarazi marked this pull request as ready for review January 15, 2021 08:07
@christarazi christarazi requested a review from a team January 15, 2021 08:07
@christarazi christarazi requested a review from a team as a code owner January 15, 2021 08:07
@christarazi christarazi requested review from a team and tklauser January 15, 2021 08:07
@christarazi christarazi requested a review from brb January 15, 2021 08:07
daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Two nits, otherwise LGTM.

pkg/maglev/maglev.go Outdated Show resolved Hide resolved
pkg/maglev/maglev.go Outdated Show resolved Hide resolved
@tklauser tklauser removed their assignment Jan 15, 2021
@christarazi christarazi force-pushed the pr/christarazi/maglev-perf branch 3 times, most recently from 2f91c70 to be5555a Compare January 18, 2021 20:59
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

🚀

@christarazi
Copy link
Member Author

test-me-please

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

A few nits.

go.mod Outdated Show resolved Hide resolved
pkg/maglev/maglev.go Outdated Show resolved Hide resolved
pkg/maglev/maglev.go Outdated Show resolved Hide resolved
@christarazi
Copy link
Member Author

CI has passed. Pushing fixups to nits, which don't require a re-run of CI.

@christarazi christarazi force-pushed the pr/christarazi/maglev-perf branch 2 times, most recently from af12caa to 1159b37 Compare January 29, 2021 18:32
@christarazi christarazi force-pushed the pr/christarazi/maglev-perf branch 2 times, most recently from 989c1bf to b14d077 Compare February 1, 2021 17:23
pkg/maglev/maglev.go Outdated Show resolved Hide resolved
@christarazi
Copy link
Member Author

test-me-please

pkg/maglev/maglev.go Outdated Show resolved Hide resolved
pkg/maglev/maglev.go Outdated Show resolved Hide resolved
Previously, the Maglev permutations slice is allocated inside
`getPermutation()`, which means every time `maglev.GetLookupTable()` is
called, the slice will be recreated. That means it would recreated on
every Service creation or update.

This commit allocates the slice ahead of time when the Maglev subsystem
is initialized (`maglev.Init()`). This gives a rough improvement in time
of around 15%.

The slice is allocated based on a heuristic computation, involving the M
size. Nodes running with less than 8GB of RAM do not apply and Cilium
will not be preallocate the slice ahead of time. For nodes with more
than the aforementioned threshold, the formula for the heuristic is:

    (M / 100) * 100

This is derived from the maximum backends property from Maglev where
|backends| * 100 < M. This gives the following memory pressure profile
based on M (left-hand side):

    251:    0.004806594848632812 MB
    509:    0.019766311645507812 MB
    1021:   0.07953193664550783 MB
    2039:   0.3171936798095703 MB
    4093:   1.2781256866455077 MB
    8191:   5.118750076293945 MB
    16381:  20.472500686645507 MB
    32749:  81.82502754211426 MB
    65521:  327.5300171661377 MB
    131071: 1310.700000076294 MB

If the user has more backends than the preallocated size, we will adjust
the allocation to be |backends| * M. This is because we want to ensure
that the Maglev subsystem isn't thrashing trying to reallocate the slice
on each Service create or update.

Benchmark timings:

Before
```
$ go test -v ./pkg/maglev -check.v -check.b -check.btime 5s -check.bmem
=== RUN   Test
PASS: maglev_test.go:91: MaglevTestSuite.BenchmarkGetMaglevTable              50         341255883 ns/op1049633800 B/op        12 allocs/op
OK: 1 passed
--- PASS: Test (17.42s)
PASS
ok      github.com/cilium/cilium/pkg/maglev     17.475s
```

After
```
$ go test -v ./pkg/maglev -check.v -check.b -check.btime 5s -check.bmem
=== RUN   Test
PASS: maglev_test.go:91: MaglevTestSuite.BenchmarkGetMaglevTable              50         282792516 ns/op 1057899 B/op          11 allocs/op
OK: 1 passed
--- PASS: Test (17.89s)
PASS
ok      github.com/cilium/cilium/pkg/maglev     17.943s
```

Suggested-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
Copy link
Member Author

CI passed on the last run. Pushed a change that only updated the comments so no actual code was changed.

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

🚀

@borkmann borkmann merged commit e567f8b into cilium:master Feb 3, 2021
@christarazi christarazi deleted the pr/christarazi/maglev-perf branch February 3, 2021 16:55
@christarazi christarazi added this to Needs backport from master in 1.9.5 Feb 3, 2021
@christarazi christarazi removed this from Needs backport from master in 1.9.4 Feb 3, 2021
@tklauser tklauser removed this from Needs backport from master in 1.9.5 Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/performance There is a performance impact of this. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants