-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Benchmark: fix build tags #2099
Conversation
Those are all cpu time related syscalls. Or, since they are all the same function, we should move those to a separate |
55077ac
to
7e9f518
Compare
Review status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @lyuxuan and @menghanl) internal/benchmarkutil/benchmarkutil_linux.go, line 21 at r2 (raw file):
Name this package syscall? internal/benchmarkutil/benchmarkutil_linux.go, line 44 at r2 (raw file):
Make this function return Comments from Reviewable |
Review status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @menghanl and @lyuxuan) internal/benchmarkutil/benchmarkutil_linux.go, line 21 at r2 (raw file): Previously, menghanl (Menghan Li) wrote…
both syscall and unix package functions are included here. internal/benchmarkutil/benchmarkutil_linux.go, line 44 at r2 (raw file): Previously, menghanl (Menghan Li) wrote…
done Comments from Reviewable |
Review status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @menghanl) internal/benchmarkutil/benchmarkutil_linux.go, line 21 at r2 (raw file): Previously, lyuxuan wrote…
What I was thinking is, this will be the grpc syscall package. Whenever there's a requirement for syscall in grpc, the code should go here. unix should be considered as part of syscall. (And it's under Comments from Reviewable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 8 files at r2, 6 of 6 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyuxuan)
benchmark/server/main.go, line 35 at r4 (raw file):
"google.golang.org/grpc/benchmark" "google.golang.org/grpc/grpclog" grpcsyscall "google.golang.org/grpc/internal/syscall"
Why does this need to be renamed?
internal/syscall/syscall_linux.go, line 40 at r4 (raw file):
// Rusage is an alias for syscall.Rusage under linux non-appengine environment. // TODO(lyuxuan): use type alias after go1.8 is no longer supported.
Nit: I don't think we want to do this.
type alias actually leaks details of how Rusage
is implemented, but I think we want to keep that internal to this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyuxuan)
benchmark/server/main.go, line 35 at r4 (raw file):
Previously, menghanl (Menghan Li) wrote…
Why does this need to be renamed?
Because we also import syscall.
internal/syscall/syscall_linux.go, line 40 at r4 (raw file):
Previously, menghanl (Menghan Li) wrote…
Nit: I don't think we want to do this.
type alias actually leaks details of howRusage
is implemented, but I think we want to keep that internal to this package.
OK.
internal/benchmarkutil/benchmarkutil_linux.go, line 21 at r2 (raw file):
Previously, menghanl (Menghan Li) wrote…
What I was thinking is, this will be the grpc syscall package. Whenever there's a requirement for syscall in grpc, the code should go here.
unix should be considered as part of syscall. (And it's under
x/sys
)
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @menghanl)
benchmark/server/main.go, line 35 at r4 (raw file):
Previously, lyuxuan wrote…
Because we also import syscall.
Hmm, it shouldn't... syscall should always be guarded: https://go-proverbs.github.io/
It seems syscall.SIGTERM
is only defined for unix-like systems. This file might not build on windows.
It's defined here:
https://github.com/golang/go/blob/go1.10/src/syscall/zerrors_linux_amd64.go#L1365
But not on windows:
https://github.com/golang/go/blob/go1.10/src/syscall/zerrors_windows.go
some syscall and unix package functions/variables are not defined in environments like windows, darwin, etc.
Restrict to linux only.
This change is