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

refactor: use raw system call instead of clone-and-read-from-stdout to boost GetKernelVersion. #2463

Merged
merged 1 commit into from
Nov 11, 2018

Conversation

SimonCqk
Copy link
Contributor

@SimonCqk SimonCqk commented Nov 10, 2018

Signed-off-by: Simoncqk cqk0100@gmail.com

Ⅰ. Describe what this PR did

refactor: use raw system call instead of clone-and-read-from-stdout to boost GetKernelVersion. Original exec.Run cloned a new process which caused performance hot spot.

Ⅱ. Does this pull request fix one issue?

fixes#2459

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

kernel_test.go has covered this case.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Nov 10, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@e7e89c8). Click here to learn what that means.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2463   +/-   ##
=========================================
  Coverage          ?   68.93%           
=========================================
  Files             ?      277           
  Lines             ?    18297           
  Branches          ?        0           
=========================================
  Hits              ?    12613           
  Misses            ?     4256           
  Partials          ?     1428
Flag Coverage Δ
#criv1alpha1test 31.57% <66.66%> (?)
#criv1alpha2test 35.75% <66.66%> (?)
#integrationtest 40.28% <66.66%> (?)
#nodee2etest 33.07% <66.66%> (?)
#unittest 26.68% <66.66%> (?)
Impacted Files Coverage Δ
pkg/kernel/kernel.go 80.95% <66.66%> (ø)

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

@SimonCqk could you mind to rebase your commits into one and sign it off? thanks

@SimonCqk
Copy link
Contributor Author

@fuweid sorry to trouble you, I've done that.

@zhuangqh
Copy link
Contributor

LGTM also cc @allencloud

pkg/kernel/kernel.go Show resolved Hide resolved
…o boost GetKernelVersion.

Signed-off-by: Simoncqk <cqk0100@gmail.com>
// BenchmarkGetUnameByUnix-4 200000 10584 ns/op
// BenchmarkGetUnameByExecRun-4 200 6255530 ns/op

// Benchmark for executing `uname` by raw unix system call
Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking, how long this two Benchmark case execute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
the result shows 4.135s

@HusterWan
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 11, 2018
@HusterWan HusterWan merged commit a5bdf5a into AliyunContainerService:master Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate and unnecessary system call in pkg/kernel/GetVersionInfo()
5 participants