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

Avoid allocating slice for finding Process #4058

Merged
merged 1 commit into from Mar 6, 2020

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented Feb 26, 2020

For checkProcesses(), we obtain slice of Processes via allProcesses() and only use one of them where Pid matches.

allProcesses() is not exported and not used elsewhere in service.go.

This PR replaces allProcesses() by finding Process given Pid inside checkProcesses().

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 26, 2020

Build succeeded.

@codecov-io
Copy link

codecov-io commented Feb 26, 2020

Codecov Report

Merging #4058 into master will decrease coverage by 3.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4058      +/-   ##
==========================================
- Coverage   45.89%   42.53%   -3.36%     
==========================================
  Files         117      130      +13     
  Lines       11891    14828    +2937     
==========================================
+ Hits         5457     6307     +850     
- Misses       5516     7602    +2086     
- Partials      918      919       +1
Flag Coverage Δ
#linux 45.89% <ø> (ø) ⬆️
#windows 38.22% <ø> (?)
Impacted Files Coverage Δ
archive/tar_opts.go 58.82% <0%> (-12.61%) ⬇️
snapshots/native/native.go 42.42% <0%> (-10.27%) ⬇️
archive/tar.go 48.58% <0%> (-8.63%) ⬇️
cio/io.go 46.47% <0%> (-8.53%) ⬇️
metadata/snapshot.go 47.57% <0%> (-7.2%) ⬇️
metadata/containers.go 49.84% <0%> (-6.26%) ⬇️
content/local/writer.go 58.65% <0%> (-5.55%) ⬇️
content/local/store.go 49.52% <0%> (-5.29%) ⬇️
metadata/images.go 57.41% <0%> (-4.97%) ⬇️
archive/compression/compression.go 58.69% <0%> (-4.7%) ⬇️
... and 74 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aac6a51...e3ab8bd. Read the comment docs.

Copy link
Member

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

Please sign your commit git commit -s

runtime/v1/shim/service.go Show resolved Hide resolved
@AkihiroSuda
Copy link
Member

What's the purpose of this PR?

@tedyu
Copy link
Contributor Author

tedyu commented Feb 26, 2020

@AkihiroSuda
allProcesses() composes a slice where only one element is used.
This is wasteful.

This PR does cleanup.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 26, 2020

Build succeeded.

runtime/v1/shim/service.go Outdated Show resolved Hide resolved
@tedyu tedyu changed the title Introduce getProcess for retrieving Process based on Pid Avoid allocating slice for finding Process Feb 26, 2020
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 26, 2020

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 26, 2020

Build succeeded.

runtime/v1/shim/service.go Outdated Show resolved Hide resolved
@crosbymichael
Copy link
Member

should be good to rebase now

Signed-off-by: zyu <yuzhihong@gmail.com>
@tedyu
Copy link
Contributor Author

tedyu commented Mar 6, 2020

@crosbymichael
Rebase has been done.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 6, 2020

Build succeeded.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 97ca1be into containerd:master Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants