Interestingly I also just ran into this while collecting a histogram of how long goroutines remain running for, which had a big chunk running for absurdly short periods (eg ~30ns). Upon digging I found it was this same loop.

cc @mknyszek

One somewhat hacky workaround would be to check for idle Ps prior to Gosched. If there are idle Ps, then Gosched is very unlikely to find work, so we could just skip it.

I agree it would be great to do something about this. Putting it in backlog since it's not a new issue. At the very least we can do what Michael suggested and just patch it for bgsweep. I like your idea of making Gosched a little smarter at emitting events in general (or calling into the scheduler).

This could bear to be higher priority. Wasting 1% of CPU is expensive these days, certainly on cloud platforms! But thanks for looking into it.

Wasting 1% of CPU is expensive these days

Thanks for the bump. I agree this issue is important to resolve, but I don't think resolving it is going to result in a 1% improvement in CPU utilization (or an improvement to throughput for that matter). Something has to do that sweep work, and bgsweep typically only takes a non-trivial amount of CPU time if the application has idle time to spare anyway, precisely because it does call Gosched so often. There's certainly overhead to calling Gosched each time, but I suspect a large part of that overhead is just eating up available idle time anyway. That was the original intent behind how bgsweep is written. With that said, bgsweep could be more efficient by not calling into Gosched as much (to your point), and the most immediate benefit is to use less of that available idle CPU (nice from a power consumption perspective). But again, I don't see this improving 1% of anything. If I'm wrong though, that'd be cool. :)

I think @dominikh's point is more that 1.31% of CPU time is responsible for 50% of the trace data, which I agree seems wildly disproportionate.

In any case, I've already put this on my list for 1.20 (EDIT: just realized it was still in the Backlog milestone; moved), so I plan to work on it. There are a number of ways to resolve this issue.

1. Treat bgsweep like an idle mark worker.
   • The benefit of this is we never end up in bgsweep at all if the application is firing on all cylinders. I suspect going through the scheduler just to sweep a single span is probably less effective than just letting things that allocate memory do it.
   • The cost is it would require additional complexity to how things are scheduled.
2. Have bgsweep check for idle Ps before calling Gosched. If there are idle Ps, keep going anyway.
   • The benefit of this is it fixes the immediate issue without too much additional complexity. You still have random bgsweep schedules going on to sweep a single span, but it's probably already not a big deal today.
3. (2) but push that check down into Gosched.
   • The benefit of this is that now anything that calls Gosched won't unnecessarily go into the scheduler (though due to races it could be wrong about idleness, so it makes Gosched a tiny bit weaker).

I think (2) -> (3) -> (1) is a reasonable path. I'll do (2) first which should solve the immediate issue, (3) might be nice in general (though I need to double-check that it's not breaking any documented semantics), and (1) just to see how gross it is and if it makes any meaningful performance difference.

And to be clear, (2) is the hacky workaround @prattmic mentioned. I personally don't think it's that hacky, and the effects of such a change are local to just bgsweep which is nice.

I'm worried about (3). Gosched is a full call to schedule, so it can do lots of stuff: check the global runq, run timers, run GC workers, etc. If we vastly simplify Gosched with an idle P short-circuit, those behaviors are lost, and applications that want them (likely for perf/latency reasons) have no way of requesting them without fully blocking.

those behaviors are lost

While this is true and makes (3) more risky, I'm not positive we actually guarantee any of that (and in theory, it could change out from underneath users). Though, I don't feel strongly about this. We can skip (3), and I'll just try (2).

Also, (3) will probably break a whole bunch of our own tests...

Working on (3), and while I think npidle is an easy signal to check for when GOMAXPROCS>1, relying on only that means that when GOMAXPROCS=1 we still have the same issue, basically.

A mitigation for that case could just be to do more work than sweeping one span before yielding. For instance, we could sweep 10 spans at once. Since we have a pretty good idea that this goroutine runs for only 10s of nanoseconds at a time, I think we can easily do 10x the amount of work before trying to yield, which will produce 10% of the events it produces today.

For something slightly more robust, we can more explicitly bound the latency the same way the scavenger does. However, while that conceptually is more robust, it has some issues that we need to work around (e.g. coarse grained timers) which add complexity. I'm inclined to just sweep 10 spans at time, write a comment about it, and call it good (plus the npidle check, which is easy).

Any thoughts?

GOMAXPROCS=1 doesn't seem common enough to warrant a lot of extra complexity. It also seems less likely to run into this issue, as it requires both idle Ps and a lot of garbage being produced (otherwise bgsweep doesn't have much work to do in the first place), and I don't see that happening with a single, idle P.

I tend to agree, but I also don't see much of a downside to sweeping 10 spans at a time, and it would be unfortunate to miss this one case (for the sake of robustness). The bgsweep goroutine is still preemptible in between each span swept, so it's not like it adds any latency to say, a STW or something.

I'm inclined to just sweep 10 spans at time, write a comment about it, and call it good

Fewer calls into the scheduler sounds good to me. I think that doing even 10µs of work at a time would keep this responsive enough to hide in the noise of the other latency sources we have today.

Avoiding coarse-grained timer problems and bumping by a constant 10x (and confirming that in practice that the loop typically takes 10µs or less on modern hardware) seems like a good plan. Maybe the next release goes up to 32 iterations, and the one after increases it to 100, if we're not yet happy with the balance of responsiveness vs efficiency vs simplicity.

Change https://go.dev/cl/429615 mentions this issue: runtime: tweak bgsweep "low-priority" heuristic

I tend to agree, but I also don't see much of a downside to sweeping 10 spans at a time, and it would be unfortunate to miss this one case (for the sake of robustness). The bgsweep goroutine is still preemptible in between each span swept, so it's not like it adds any latency to say, a STW or something.

Sorry, yes, sweeping multiple spans SGTM. I was thinking of the complexity of timer-based latency bounds.

@mknyszek It seems that nSwept will keep increase until it overflows. It's fine in this case, but looks weird. How about to set the sweepBatchSize to 2ⁿ and implement it like this:

	const sweepBatchSize = 1 << 3 // or 1<<4
	const mask = sweepBatchSize - 1
	nSwept := 0
	for sweepone() != ^uintptr(0) {
		sweep.nbgsweep++
		nSwept = (nSwept + 1) & mask
		if nSwept == 0 {
			goschedIfBusy()
		}
	}

https://go.dev/play/p/-sxVGAPj0XZ?v=gotip

Thanks, but I don't think this is necessary.

It's not actually possible for nSwept to overflow because it refers to swept spans in a single GC cycle (because it gets reset on each loop), and at most there's one per 8 KiB of heap memory. Overflowing would require the heap to be 8 KiB * (size of address space) which is by definition impossible. In other words, addresses would start to overflow and we'd be out of memory before then.

However, I don't think it needs a comment or anything; we have plenty of counters in the runtime that are proportional to the size of the heap that won't overflow for exactly the same reason. IMO the logic of the existing code demonstrates intent more clearly, and is also (I believe) correct.

Sorry for the noise! I thought nSwept is a global variable when I wrote the comment. I must be out of my mind then. Thank you for your patience!

It's no worries. :) You may have been thinking of nbgsweep which is a global that has a chance of overflowing if a program just runs long enough, but I think it's pretty unlikely. The amount of time that would need to pass, even with an aggressively allocating application, is pretty high.