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: count globals toward GC trigger #19839

Closed
josharian opened this issue Apr 4, 2017 · 13 comments
Closed

runtime: count globals toward GC trigger #19839

josharian opened this issue Apr 4, 2017 · 13 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@josharian
Copy link
Contributor

CL 39471 moves a large cache from the heap to a global. (A single Ctxt object is allocated at the beginning of compilation, so there's exactly one of these Prog caches, both before and after the CL.) The CL is just a simplified demo, but it mimics something real I want to do.

The CL causes GC to use lots more CPU when compiling package archive/tar. (It affects other packages as well, but archive/tar shows it most prominently.)

name  old time/op     new time/op     delta
Tar       119ms ± 5%      120ms ± 3%     ~     (p=0.061 n=45+46)

name  old user-ns/op  new user-ns/op  delta
Tar        138M ± 5%       158M ± 7%  +14.30%  (p=0.000 n=44+48)

Note that the real time is about the same--the compiler itself is doing the same amount of work--but the CPU consumed goes up considerably. I'd expect it to be unchanged.

@aclements @RLH

@randall77
Copy link
Contributor

Maybe our global scanning code is suboptimal somehow?

@josharian
Copy link
Contributor Author

Found an (utterly obvious in retrospect) workaround. Instead of

var cache [10000]obj.Prog

do

var cache *[10000]obj.Prog

func init() {
  cache = new([10000]obj.Prog)
}

With that change, performance returns to previous levels. We can revert that workaround once this issue is fixed.

Maybe our global scanning code is suboptimal somehow?

Or because globals are roots, they are always (expensively) rescanned? I'll leave this for people who know the GC.

@aclements
Copy link
Member

Or because globals are roots, they are always (expensively) rescanned?

Scanning globals is probably slightly more efficient than scanning the heap (the process is basically identical, but globals use a 1 bit bitmap instead of a 2 bit bitmap). But I don't think that's what's going on here.

GC is triggered by the size of the heap, so if you move a large block of memory from the heap to a global, GC is going to be triggered more often, but it still has just as much work to do, so you spend more aggregate time in GC.

@cespare
Copy link
Contributor

cespare commented Apr 4, 2017

Add heap ballast to the compiler? :P

@josharian
Copy link
Contributor Author

Sigh. This is a pretty silly situation.

@josharian
Copy link
Contributor Author

Silliness aside, this also seems like the sort of thing that encourages abuse. Should globals perhaps be included in the trigger calculation?

@aclements
Copy link
Member

Should globals perhaps be included in the trigger calculation?

That's not a bad idea. In fact, part of the point of GOGC is to amortize the cost of scanning and if we don't count all of the scannable stuff, we're not getting the full amortization. Some stuff is just hard to count (e.g., stacks), but scannable globals would be easy.

@RLH, thoughts?

@RLH
Copy link
Contributor

RLH commented Apr 5, 2017 via email

@gopherbot
Copy link

CL https://golang.org/cl/39713 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Apr 6, 2017
There's a surprising amount of variety
in the drivers of compilation speed.
It's helpful to have a variety of packages here.
For example, archive/tar exhibits golang/go#19839
much more than the others.

Change-Id: If66b332d63427fb246305cb14cfee9ef450bcdcf
Reviewed-on: https://go-review.googlesource.com/39713
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@aclements aclements changed the title runtime: increased GC work for non-heap-allocated memory runtime: count globals toward GC trigger Jun 19, 2017
@aclements aclements added this to the Go1.10 milestone Jun 19, 2017
@aclements aclements self-assigned this Jun 19, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jul 9, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 9, 2018
@aclements aclements modified the milestones: Go1.12, Go1.13 Dec 18, 2018
@aclements aclements modified the milestones: Go1.13, Go1.14 Jun 25, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@prattmic
Copy link
Member

@mknyszek I believe this is done in #44167?

@mknyszek
Copy link
Contributor

Yup, this is done.

@go101
Copy link

go101 commented Aug 19, 2022

BTW, is the GC pacer resign mentioned in #44167 still only available in experimental mode?

@mknyszek
Copy link
Contributor

It's been enabled by default since Go 1.18 released (and the internal flag was removed in 1.19).

@golang golang locked and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests