Prevent GC from schedule itself with 0 period.#9795
Conversation
|
Hi @catap. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
catap
force-pushed
the
prevent-zero-timer
branch
2 times, most recently
from
fc72892 to
1e1d037
Compare
|
I'm curious what |
| // anyway, on startup gcTimeSum might work fast and return 0 | ||
| // on this case algorithm above turns in infinity loop which | ||
| // simple consume CPU on timer which fires without any interval | ||
| if interval < time.Second { |
There was a problem hiding this comment.
Rather than define a lower bound for the interval, I think it would be better to define it for the avg. A sensible value there may be 5ms. With the default threshold of 0.02 that would have the interval set at a minimum 245ms, but adjustable by changing the pause threshold.
There was a problem hiding this comment.
That still make it possible to turn into the same issue. If someone put threshold 1 for example.
My approach should help for not heavy loaded docker machine which runs on background, on that case 1s doesn't make any differences.
There was a problem hiding this comment.
That still make it possible to turn into the same issue. If someone put threshold 1 for example.
.5 is the max value and .02 is the default. The pauseThreshold is the configurable value to modulate the interval though. Setting the threshold to 0.0 would also solve the problem in this case to keep the interval will stay at a second.
Better solution would be to update the default pauseThreshold along with changing the lower bound avg to have a better default interval for unloaded machines and generally spend less time checking the gc variables. We already have an event system that can reschedule in the cases when gc is expected to run following some user action and we see this commonly used now by clients.
There was a problem hiding this comment.
Agreed and updated the PR.
Thanks for explanation.
Nothing. It is clean VM (alpine 3.19) where I've installed only docker. So, on start it doesn't do anything and it turns that |
catap
force-pushed
the
prevent-zero-timer
branch
3 times, most recently
from
2ed629a to
5153b4e
Compare
| // on this case the algorithm turns in infinity loop which | ||
| // simple consume CPU on timer which fires without any delay. | ||
| // Use 5ms as fallback value to have interval 245ms for that case. | ||
| if avg == 0 { |
There was a problem hiding this comment.
The time.Duration is actually in nanoseconds. It should never actually be zero here. Rather just comparing < 5000000 it probably makes more sent to define a const like const minimumGCTime = float64(5 * time.Millisecond) and use that to compare less than and setting avg.
catap
force-pushed
the
prevent-zero-timer
branch
2 times, most recently
from
6774776 to
e365070
Compare
|
On Fri, 09 Feb 2024 21:02:03 +0100, Derek McGowan wrote:
The `time.Duration` is actually in nanoseconds. It should never actually be
zero here.
Well.. It is. I assume than on the start is no GC cycle had happened, that turns
to zero elapsed time.
Rather just comparing `< 5000000` it probably makes more sent to define a
const like `const minimumGCTime = float64(5 * time.Millisecond)` and use that
to compare less than and setting `avg`.
Done. It makes code cleaner, indeed.
…--
wbr, Kirill
|
|
@dmcgowan ping here |
|
Code change looks good. The comment above it could be cleaned up. Maybe remove the first 3 lines and just have |
dmcgowan
added
cherry-pick/1.6.x
cherry-pick/1.7.x
catap
force-pushed
the
prevent-zero-timer
branch
from
e365070 to
a422176
Compare
|
On Thu, 15 Feb 2024 00:59:38 +0100, Derek McGowan wrote:
Code change looks good. The comment above it could be cleaned up. Maybe remove the first 3 lines and just have
```
// Enforce that avg is no less than minimumGCTime to prevent immediate rescheduling
```
Here it is.
…--
wbr, Kirill
|
On startup `gcTimeSum` might work fast and return `0`, so on this case the algorithm turns in infinity loop which simple consume CPU on timer which fires without any interval. Use `5ms` as fallback to have interval `245ms` for that case. Closes: containerd#5089 Signed-off-by: Kirill A. Korinsky <[email protected]>
catap
force-pushed
the
prevent-zero-timer
branch
from
a422176 to
c876612
Compare
|
Yes, this one is awaiting backport. Right now the automated backport is not working but anyone is free to open up the backport PRs. |
|
@dmcgowan here it is! |
dmcgowan
added
cherry-picked/1.6.x
4 participants
On startup
gcTimeSummight work fast and return0, so on this case the algorithm turns in infinity loop which simple consume CPU on timer which fires without any interval.Closes: #5089