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.

Sign up for GitHub

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

Jump to bottom

br: set memory limit #53793

Merged
merged 8 commits into from
Aug 5, 2024
Merged

br: set memory limit #53793

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c8f2b0a
set memory limit
Leavrth Jun 4, 2024
5ac7699
set memory limit
Leavrth Jun 4, 2024
55bc0b4
calculate the memory limit
Leavrth Jun 5, 2024
e01fe8b
bazel prepare
Leavrth Jun 5, 2024
438d462
commit some suggestions
Leavrth Jul 19, 2024
e1650d5
add the minimum value of memlimit
Leavrth Jul 19, 2024
d9c42cb
commit some suggestions
Leavrth Jul 19, 2024
1e15fea
commit some suggestions
Leavrth Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion br/cmd/br/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,8 @@ go_test(
srcs = ["main_test.go"],
embed = [":br_lib"],
flaky = True,
deps = ["@org_uber_go_goleak//:goleak"],
deps = [
"@com_github_stretchr_testify//require",
"@org_uber_go_goleak//:goleak",
],
)
37 changes: 37 additions & 0 deletions br/cmd/br/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ package main
import (
"context"
"fmt"
"math"
"os"
"path/filepath"
"runtime/debug"
"sync"
"sync/atomic"
"time"
Expand All @@ -24,6 +26,7 @@ import (
"github.com/pingcap/tidb/pkg/util/memory"
"github.com/pingcap/tidb/pkg/util/redact"
"github.com/spf13/cobra"
"go.uber.org/zap"
)

var (
Expand Down Expand Up @@ -107,6 +110,23 @@ func AddFlags(cmd *cobra.Command) {
_ = cmd.PersistentFlags().MarkHidden(FlagRedactLog)
}

func calculateMemoryLimit(memleft uint64) uint64 {
BornChanger marked this conversation as resolved.
Show resolved Hide resolved
const halfGB = uint64(512 * 1024 * 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it would be better to put it into some util file for reuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Reuse the tidb/pkg/util/size.MB.

const fourGB = 8 * halfGB
// memreserved = f(memleft) = 512MB * memleft / (memleft + 4GB)
Leavrth marked this conversation as resolved.
Show resolved Hide resolved
// * f(0) = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

if the mem unused is 0, do we even bother to start the BR? I guess a correctly configured machine should not use up all its mem and if they do they are facing a bigger problem and we probably don't want to start a BR to make things more complicated. Let me know your thoughts!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think maybe it is already OOM. Maybe we can directly limit the minimum remaining memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I feel like we can have some minimum memory not starting from 0, if below that we just throw error and exit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add the minimum value (256MiB) of memory limit. It's hard to determine the minimum memory br usage. In my test, br uses only about 200 MB~300 MB memory to backup data generated by tiup bench tpcc prepare --warehouses 3.

Copy link
Contributor

@Tristan1900 Tristan1900 Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for digging into that, yeah having an estimation of memory usage is very useful.

// * f(4GB) = 256MB
// * f(+inf) -> 512MB
memreserved := halfGB / (1 + fourGB/(memleft+1))
Copy link
Contributor

Choose a reason for hiding this comment

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

extreme case, if memleft is unit64 max it's going to overflow I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

in what case would memleft be uint64max (= 16 EiB) 👀

anyway changing memleft + 1 to memleft | 1 should be good enough

Copy link
Contributor

@Tristan1900 Tristan1900 Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

in the unexpected error case I would say, the memleft is passed down by memleft := memtotal - memused, if memtotal < memused, it can underflow as in my other comment. it's always better to explicitly check and make sure things won't go wrong for corners cases, cuz a lot of assumptions can turn out to be wrong in production based on my experience : ) (in this case the assumption is memory.MemTotal >= memory.MemUsed )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. And add check memtotal < memused in another comment.

// 0 memused memtotal-memreserved memtotal
// +--------+--------------------+----------------+
// ^ br mem upper limit
// +--------------------^
// GOMEMLIMIT range
memlimit := memleft - memreserved
return memlimit
}

// Init initializes BR cli.
func Init(cmd *cobra.Command) (err error) {
initOnce.Do(func() {
Expand Down Expand Up @@ -162,6 +182,23 @@ func Init(cmd *cobra.Command) (err error) {
}
log.ReplaceGlobals(lg, p)
memory.InitMemoryHook()
if debug.SetMemoryLimit(-1) == math.MaxInt64 {
memtotal, e := memory.MemTotal()
if e != nil {
err = e
return
}
memused, e := memory.MemUsed()
if e != nil {
err = e
return
}
memleft := memtotal - memused
Copy link
Contributor

@Tristan1900 Tristan1900 Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

do we need to add a safety check here to make sure memtotal-memused won't underflow? since we own the memory package and it might have a bug that returned memtotal is actually smaller than memused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. If underflow, output a warning log and skip set memory limit.

memlimit := calculateMemoryLimit(memleft)
log.Info("calculate the rest memory",
zap.Uint64("memtotal", memtotal), zap.Uint64("memused", memused), zap.Uint64("memlimit", memlimit))
debug.SetMemoryLimit(int64(memlimit))
Copy link
Contributor

Choose a reason for hiding this comment

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

to be extra safe, can we do a safe cast from uint64 to int64, something like

func safeCastUint64ToInt64(u uint64) int64 {
	if u > uint64(math.MaxInt64) {
		return math.MaxInt64
	}
	return int64(u)
}

and make it a util method?
cuz in the doc
A zero limit or a limit that's lower than the amount of memory used by the Go runtime may cause the garbage collector to run nearly continuously. However, the application may still make progress.

Copy link
Contributor Author

@Leavrth Leavrth Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

done.

}

redactLog, e := cmd.Flags().GetBool(FlagRedactLog)
if e != nil {
Expand Down
16 changes: 16 additions & 0 deletions br/cmd/br/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/require"
"go.uber.org/goleak"
)

Expand Down Expand Up @@ -75,3 +76,18 @@ func TestRunMain(*testing.T) {

<-waitCh
}

func TestCalculateMemoryLimit(t *testing.T) {
// f(0 Byte) = 0 Byte
require.Equal(t, uint64(0), calculateMemoryLimit(0))
// f(100 KB) = 87.5 KB
require.Equal(t, uint64(89600), calculateMemoryLimit(100*1024))
// f(100 MB) = 87.5 MB
require.Equal(t, uint64(91763188), calculateMemoryLimit(100*1024*1024))
// f(3.99 GB) = 3.74 GB
require.Equal(t, uint64(4026531839), calculateMemoryLimit(4*1024*1024*1024-1))
// f(4 GB) = 3.5 GB
require.Equal(t, uint64(3758096384), calculateMemoryLimit(4*1024*1024*1024))
// f(32 GB) = 31.5 GB
require.Equal(t, uint64(33822867456), calculateMemoryLimit(32*1024*1024*1024))
}
Loading