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

split runc-init from runc(don't merge) #4343

Closed

Conversation

ningmingxiao
Copy link

Improve performance by about 20%
create and delete 1000 container.
before:17.066672743s
after:13.397641198s

@ningmingxiao ningmingxiao force-pushed the speed_up branch 2 times, most recently from 08f512d to affb027 Compare July 10, 2024 10:48
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

NACK.

We use /proc/self/exe and have a single binary because having multiple binaries will just lead to possible configuration issues (an outdated runc-init binary but a newer runc binary) which could cause security issues (imagine we move the code for configuring a security setting from runc to runc-init -- if runc-init is not updated then the security setting will no longer be updated at all).

I also don't like that we would now search $PATH for half of the runc implementation. If you downloaded the static runc binary and ran it, it would lead to the above issue because it would try to use the old runc-init binary (if you have it installed, if you don't you'll get the same error CI has now).

(I'm surprised you saw a 20% change in execution time -- runc init should pull in so many libraries that it would make very little difference in binary size.)

Signed-off-by: ningmingxiao <ning.mingxiao@zte.com.cn>
@kolyshkin
Copy link
Contributor

NACK. This would make sense if runc-init was a small binary, but practically it's the same size.

I also did some testing (using runc exec as it uses runc-init and is not tied much to I/O etc), and haven't noticed any performance improvement.

@ningmingxiao
Copy link
Author

ningmingxiao commented Jul 11, 2024

I retest it will imporve little performance.
but will use less memory. with runc init
before

USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root     2609091  0.1  0.1 1613464 10100 ?       Ssl  12:51   0:00  \_ runc init
root     2609094  0.0  0.1 1613976 10512 ?       Ssl  12:51   0:00  \_ runc init
root     2609104  0.1  0.1 1613976 10580 ?       Ssl  12:51   0:00  \_ runc init
root     2609114  0.1  0.1 1539476 10008 ?       Ssl  12:51   0:00  \_ runc init
root     2609134  0.0  0.1 1613976 10356 ?       Ssl  12:51   0:00  \_ runc init
root     2609135  0.1  0.1 1539476 10160 ?       Ssl  12:51   0:00  \_ runc init
root     2609136  0.0  0.1 1613976 10700 ?       Ssl  12:51   0:00  \_ runc init
root     2609148  0.0  0.1 1613720 10324 ?       Ssl  12:51   0:00  \_ runc init
root     2609151  0.1  0.1 1613464 9896 ?        Ssl  12:51   0:00  \_ runc init

after

USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root     2580591  0.6  0.1 1611696 8884 ?        Ssl  12:49   0:00  \_ runc init
root     2580592  0.3  0.1 1611952 9400 ?        Ssl  12:49   0:00  \_ runc init
root     2580595  0.6  0.1 1612208 9428 ?        Ssl  12:49   0:00  \_ runc init
root     2580626  0.6  0.1 1611952 9028 ?        Ssl  12:49   0:00  \_ runc init
root     2580627  0.5  0.1 1611952 8980 ?        Ssl  12:49   0:00  \_ runc init
root     2580630  0.5  0.1 1537708 9120 ?        Ssl  12:49   0:00  \_ runc init
root     2580631  0.3  0.1 1612208 9352 ?        Ssl  12:49   0:00  \_ runc init
root     2580636  0.6  0.1 1611952 9160 ?        Ssl  12:49   0:00  \_ runc init
root     2580645  0.3  0.1 1611440 8912 ?        Ssl  12:49   0:00  \_ runc init
root     2580647  0.3  0.1 1611952 9340 ?        Ssl  12:49   0:00  \_ runc init

@cyphar
Copy link
Member

cyphar commented Jul 11, 2024

runc init only runs for a few hundred milliseconds at most, I don't think memory usage is that big of a deal. Not to mention that (because the binary is the same as runc) page cache sharing means that it uses the same cached memory as the runc binary and so splitting the binary will actually result in more memory usage in practice AFAICS because now two binaries need to be loaded into memory.

But the memory usage discussion is a bit of a red herring anyway, I've already outlined the other reasons why splitting the binary is a bad idea.

@ningmingxiao ningmingxiao changed the title split runc-init from runc split runc-init from runc(don't merge) Jul 11, 2024
@ningmingxiao
Copy link
Author

ningmingxiao commented Jul 11, 2024

runc init only runs for a few hundred milliseconds at most, I don't think memory usage is that big of a deal. Not to mention that (because the binary is the same as runc) page cache sharing means that it uses the same cached memory as the runc binary and so splitting the binary will actually result in more memory usage in practice AFAICS because now two binaries need to be loaded into memory.

But the memory usage discussion is a bit of a red herring anyway, I've already outlined the other reasons why splitting the binary is a bad idea.

This pr doesn't need to merge,I'm looKing for some way to make runc use less memory and run more quickly. I want runc can run as ruickly as crun. I can't find some userful way, maybe use c or rust rewrite some compoment can achieve that goal.

@kolyshkin
Copy link
Contributor

This pr doesn't need to merge,I'm looKing for some way to make runc use less memory and run more quickly. I want runc can run as ruickly as crun. I can't find some userful way, maybe use c or rust rewrite some compoment can achieve that goal.

This is an interesting goal. I would start with profiling to see what takes most of the time (for e.g. runc exec) -- maybe something will pop out. There is even an issue about it: #3181

@kolyshkin kolyshkin closed this Jul 11, 2024
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.

3 participants