performance improvement: setup signal notify in a new go routine#4654
Conversation
987a0d5 to
60026a7
Compare
|
Can we include this one in |
I'm not against it, and let @cyphar decide. The code LGTM, and in my measurements it shows 5% to 20% wall clock time improvement, which is still very good. |
60026a7 to
a7b0a82
Compare
| // started. | ||
| handler := newSignalHandler(r.enableSubreaper, r.notifySocket) | ||
| handlerCh := newSignalHandler(r.enableSubreaper, r.notifySocket) | ||
| tty, err := setupIO(process, r.container, config.Terminal, detach, r.consoleSocket) |
There was a problem hiding this comment.
Did you see if doing setupIO or setupPidfdSocket in a goroutine also provides a speed-up? I guess they're both not very complicated...
There was a problem hiding this comment.
I test these two paths, both of them spend only several milliseconds on doing the setup tasks.
I checked the code, they only connect to a socket or run a small loop for stdio, so I think it's not worth to run them in a new go routine.
|
I'm happy to take it in -rc.1 if it's ready (I had a nit and a follow-up question) but we could also add it to -rc.2 since it's a trivial bug / performance fix and not a new feature. |
89c945f to
33b9f2b
Compare
There is a big loop(at least 65 times) in `signal.Notify`, it costs as much time as `runc init`, so we can call it in parallel ro reduce the container start time. In a general test, it can be reduced about 38.70%. Signed-off-by: lifubang <lifubang@acmcoder.com> (cyphar: move signal channel definition inside goroutine) Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
33b9f2b to
d92dd22
Compare
|
FWIW, it seems like it's more like 5-10% faster on my box, but still a decent improvement: |
Very strange, the improve percentage may be related to the configuration of the machine? And has a 93-106% improvement for Details
lifubang@acmcoder:/opt/debian$ sudo hyperfine -w 50 -r 500 "./runc-patch exec test true" "./runc-HEAD exec test true"
Benchmark 1: ./runc-patch exec test true
Time (mean ± σ): 22.4 ms ± 3.2 ms [User: 6.9 ms, System: 21.7 ms]
Range (min … max): 14.1 ms … 36.7 ms 500 runs
|
|
I find it interesting that your runs spend much more time in system time than me (as well as each run being ~5x slower). Out of interest, what CPU are you running on / what kernel? I'm on an AMD Ryzen 7 7840U and Linux 6.12.6-1-default. |
|
As I said earlier, I'm also seeing 5% to 20% improvement (Linux 6.12 / Intel Core i7-12800H). |
Perhaps I should run these performance tests on the host machine instead of a virtual machine. I think we also need to test it in a virtual machine, because most of servers are running in a virtual machine in the cloud. But maybe my virtual machine needs to be improved, I will test it on the cloud host when I have the chance. |
There is a big loop(at least 65 times) in
signal.Notify, it costs as much time asrunc init, so we can call it in parallel ro reduce the container start time. In a general test, it can be reduced about 38.70% ↓.In my test with this bash script:
runc v1.2.5
runc with this patch
(5152 - 3158)/5152 = 0.38