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

runtime: reported CPU time of STW phases is low #22725

Closed
aclements opened this issue Nov 14, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@aclements
Copy link
Member

commented Nov 14, 2017

The gctrace in https://blog.cloudflare.com/go-dont-collect-my-garbage/ shows CPU times of STW phases that are only a small multiple of the wall-clock time, even though GOMAXPROCS is 48. It should be min(ncpu, GOMAXPROCS) * wallTime.

This also causes the overall GC CPU fraction reported in the gctrace to be much too low, since it's based on these CPU times.

/cc @RLH

@aclements aclements added this to the Go1.10 milestone Nov 14, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Nov 14, 2017

Change https://golang.org/cl/77710 mentions this issue: runtime: fix gctrace STW CPU time and CPU fraction

@bronze1man

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

Is it possible for the compiler to find that the memory just need free at some time. and use this infomation to skip some gc steps?

@aclements

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2017

@bronze1man, I'm not sure I understand your question or why it's relevant to this issue. In general, the compiler's escape analysis already lets objects be freed without a GC in specific but very common cases. It can be made better, and we'd like to make it better, but that's a huge topic and an area of active research in the field that I can't really summarize here. :) That reduces load on the GC, but isn't related to eliminating any phases of the GC algorithm (there are only four phases, anyway).

@gopherbot gopherbot closed this in 89b7a08 Nov 15, 2017

@aclements aclements modified the milestones: Go1.10, Go1.9.3 Nov 15, 2017

@aclements

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2017

Re-opening for consideration for 1.9.3.

@aclements aclements reopened this Nov 15, 2017

@bronze1man

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

@aclements From the html page https://blog.cloudflare.com/go-dont-collect-my-garbage/ , that guy alloc a lot object ,throw the object and return his function , he do not touch that alloc object again. I think the compiler can mark those objects can be freed for sure just like the user call free in c, and the GC algorithm do not need to find which object is not used, It can just free them or just reuse them to another object, this way should reduces load on the GC.

The compiler's escape analysis can not handle some unknow(unknow at compiler time) length objects. My suggestion may reduces load on the GC more.(may be no need to gc any more?) But i do not understand the GC algorithm in detail.

My suggestion looks like the compiler can use a *sync.Pool if it can save alloc.

@aclements

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2017

@bronze1man, we don't know specifically what Vlad was doing in his benchmark, so I can't say how easy or hard it would be for the compiler to optimize the allocation pattern.

We'd certainly like to improve the escape analysis to be able to handle variable-length allocations (and many other things it can't currently handle). It's on our list, but, alas, it's a hard problem and there are only so many hours in a day. :)

@andybons

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

CL 77710 OK for Go 1.9.3.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 18, 2018

Change https://golang.org/cl/88321 mentions this issue: [release-branch.go1.9] runtime: fix gctrace STW CPU time and CPU fraction

gopherbot pushed a commit that referenced this issue Jan 22, 2018

[release-branch.go1.9] runtime: fix gctrace STW CPU time and CPU frac…
…tion

The CPU time reported in the gctrace for STW phases is simply
work.stwprocs times the wall-clock duration of these phases. However,
work.stwprocs is set to gcprocs(), which is wrong for multiple
reasons:

1. gcprocs is intended to limit the number of Ms used for mark
   termination based on how well the garbage collector actually
   scales, but the gctrace wants to report how much CPU time is being
   stolen from the application. During STW, that's *all* of the CPU,
   regardless of how many the garbage collector can actually use.

2. gcprocs assumes it's being called during STW, so it limits its
   result to sched.nmidle+1. However, we're not calling it during STW,
   so sched.nmidle is typically quite small, even if GOMAXPROCS is
   quite large.

Fix this by setting work.stwprocs to min(ncpu, GOMAXPROCS). This also
fixes the overall GC CPU fraction, which is based on the computed CPU
times.

Fixes #22725.

Change-Id: I64b5ce87e28dbec6870aa068ce7aecdd28c058d1
Reviewed-on: https://go-review.googlesource.com/77710
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
Reviewed-on: https://go-review.googlesource.com/88321
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@andybons

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

go1.9.3 has been packaged and includes:

  • CL 77710 runtime: fix gctrace STW CPU time and CPU fraction

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Jan 22 21:02:55 UTC

@andybons andybons closed this Jan 22, 2018

@golang golang locked and limited conversation to collaborators Jan 22, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.