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

tools/ut: add a parallel parameter #8186

Merged
merged 6 commits into from
May 23, 2024
Merged
Changes from 4 commits
Commits
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
32 changes: 20 additions & 12 deletions tools/pd-ut/ut.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ var (

var (
// runtime
p int
buildParallel int
parallel int
workDir string
coverFileTempDir string
// arguments
Expand All @@ -104,6 +103,7 @@ var (

func main() {
race = handleFlag("--race")
parallelStr := stripFlag("--parallel")
junitFile = stripFlag("--junitfile")
coverProfile = stripFlag("--coverprofile")
ignoreDir = stripFlag("--ignore")
Expand All @@ -118,10 +118,17 @@ func main() {
defer os.RemoveAll(coverFileTempDir)
}

// Get the correct count of CPU if it's in docker.
p = runtime.GOMAXPROCS(0)
// We use 2 * p for `go build` to make it faster.
buildParallel = p * 2
if parallelStr == "" {
// Get the correct count of CPU if it's in docker.
parallel = runtime.GOMAXPROCS(0)
} else {
var err error
parallel, err = strconv.Atoi(parallelStr)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need check if the parallel<=runtime.GOMAXPROCS(0)

Copy link
Member Author

@HuSharp HuSharp May 22, 2024

Choose a reason for hiding this comment

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

There is no need, we can support parallel greater than procs. It depends on the user.

Copy link
Member

@okJiang okJiang May 22, 2024

Choose a reason for hiding this comment

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

I think it's better to set a maximum value. For example, 1000. I just said it casually

what do you think

Copy link
Member Author

Choose a reason for hiding this comment

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

make sense, and add a hint when parallel was set greater than procs :)
f42fc5e#diff-d1de7e6511458d33a769c0c501ec0ea45494dd9d2e830dc315e8fe623eb34b76R132

if err != nil {
fmt.Println("parse parallel error", err)
return
}
}
var err error
workDir, err = os.Getwd()
if err != nil {
Expand Down Expand Up @@ -342,12 +349,12 @@ func cmdRun(args ...string) bool {
}
}

fmt.Printf("building task finish, parallelism=%d, count=%d, takes=%v\n", buildParallel, len(tasks), time.Since(start))
fmt.Printf("building task finish, parallelism=%d, count=%d, takes=%v\n", parallel*2, len(tasks), time.Since(start))

taskCh := make(chan task, 100)
works := make([]numa, p)
works := make([]numa, parallel)
var wg sync.WaitGroup
for i := 0; i < p; i++ {
for i := 0; i < parallel; i++ {
wg.Add(1)
go works[i].worker(&wg, taskCh)
}
Expand Down Expand Up @@ -389,7 +396,7 @@ func cmdRun(args ...string) bool {

// stripFlag strip the '--flag xxx' from the command line os.Args
// Example of the os.Args changes
// Before: ut run pkg TestXXX --coverprofile xxx --junitfile yyy
// Before: ut run pkg TestXXX --coverprofile xxx --junitfile yyy --parallel 16
// After: ut run pkg TestXXX
// The value of the flag is returned.
func stripFlag(flag string) string {
Expand Down Expand Up @@ -625,7 +632,7 @@ func (*numa) testCommand(pkg string, fn string) *exec.Cmd {
args = append(args, "-test.coverprofile", tmpFile)
}
if strings.Contains(fn, "Suite") {
args = append(args, "-test.cpu", fmt.Sprint(p/2))
args = append(args, "-test.cpu", fmt.Sprint(parallel/2))
} else {
args = append(args, "-test.cpu", "1")
}
Expand Down Expand Up @@ -694,7 +701,8 @@ func buildTestBinaryMulti(pkgs []string) error {
packages = append(packages, path.Join(modulePath, pkg))
}

p := strconv.Itoa(buildParallel)
// We use 2 * parallel for `go build` to make it faster.
Copy link
Member

@okJiang okJiang May 22, 2024

Choose a reason for hiding this comment

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

One question, why do we choose 2 * parallel to execute go build? not 1.5 or 3.

Copy link
Member Author

@HuSharp HuSharp May 22, 2024

Choose a reason for hiding this comment

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

background:

When building the test binaries, by default it is the number of cores(pkg.go.dev/cmd/go#hdr-Compile_packages_and_dependencies):

-p n
the number of programs, such as build commands or
test binaries, that can be run in parallel.
The default is GOMAXPROCS, normally the number of CPUs available.

When running the 'building binaries stage' of make ut, the CPU usage is not high with the default value. So I think it's a better idea to increase the parallelism.

why do we choose 2 * parallel to execute go build? not 1.5 or 3.

Choice 2 wasn't much thought or testing, and I think 2 is fine for the time being :) Or do you have any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Or do you have any ideas?

I have no idea about that. I thought 2 had any special meaning before

p := strconv.Itoa(parallel * 2)
cmd := exec.Command("go", "test", "-p", p, "--exec", xprogPath, "-vet", "off", "--tags=tso_function_test,deadlock")
if coverProfile != "" {
coverpkg := "./..."
Expand Down