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

Jump to bottom

go/types: audit memory usage for go1.17 #45580

Closed
findleyr opened this issue Apr 15, 2021 · 7 comments
Closed

go/types: audit memory usage for go1.17 #45580

findleyr opened this issue Apr 15, 2021 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Milestone
Go1.17

Comments

@findleyr
Copy link
Contributor

With the merging of the dev.typeparams branch, a number of the types in go/types picked up additional fields that are only used during the type checking pass. Many users of go/types (particularly gopls) are sensitive to the memory footprint of a type checked package. Filing this issue as a reminder for go1.17: we should verify that we're not pinning memory unnecessarily.

I did a quick check using both pprof and a memory debugging tool that @heschi is experimenting with for gopls, and they indicated that typechecked packages in go1.17 consume perhaps 5-10% more memory than in go1.16. We're not yet convinced that these are accurate numbers, but such a relative change is probably meaningful. If accurate, a 5-10% increase is not so bad, though we could probably eliminate it.

CC @griesemer
@findleyr findleyr added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 15, 2021
@findleyr findleyr added this to the Go1.17 milestone Apr 15, 2021
@findleyr findleyr self-assigned this Apr 15, 2021
@findleyr
Copy link
Contributor Author

Marking this as a release blocker until we determine that any memory regressions are acceptable.

@toothrot
Copy link
Contributor

toothrot commented May 6, 2021

@findleyr Checking in as this is a release-blocker. Is this acceptable to do after Beta 1?

@findleyr findleyr added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label May 6, 2021
@findleyr
Copy link
Contributor Author

findleyr commented May 6, 2021

Thanks Alex!

This is okay after Beta 1, marked it accordingly. I expect we'll address this before Beta 1, though.

@gopherbot
Copy link

Change https://golang.org/cl/318849 mentions this issue: go/types: ensure that Named.check is nilled out when it is completed

@findleyr
Copy link
Contributor Author

Update: measured a couple ways, I was consistently seeing around a 6% increase in memory footprint when working on x/tools. CL 318849 should unpin the Checker, which in my testing almost exactly reclaimed the 6% regression.

I also tried externalizing object.color_ and object.order_ into maps, but this didn't seem to make much difference in memory footprint and slowed down the type checking pass by 2-4%. There may be something worth pursuing there, but not for 1.17.

@gopherbot
Copy link

Change https://golang.org/cl/323030 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: ensure that Named.check is nilled out once it is expanded

@gopherbot
Copy link

Change https://golang.org/cl/322974 mentions this issue: go/types: guard against check==nil in newNamed

gopherbot pushed a commit that referenced this issue May 27, 2021
@findleyr
go/types: guard against check==nil in newNamed
6b8c94b 
When importing generic named types, it is possible for Checker.newNamed
to be called during type instantiation when the Checker is nil.

In this case we should be able to safely skip this delayed expansion.

Updates #45580

Change-Id: I75422100464d57eba24642c93e06e8b47d904fc3
Reviewed-on: https://go-review.googlesource.com/c/go/+/322974
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit that referenced this issue May 27, 2021
@griesemer
[dev.typeparams] cmd/compile/internal/types2: ensure that Named.check…
8c99e5d 
… is nilled out once it is expanded

This is a port of
- https://golang.org/cl/318849
- https://golang.org/cl/322974

For #45580.

Change-Id: Ie0700ed6c8d472305d5ba7ff97da1ae063152aa3
Reviewed-on: https://go-review.googlesource.com/c/go/+/323030
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@seankhliao seankhliao mentioned this issue Oct 18, 2021
Closed
@golang golang locked and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
3 participants
@toothrot @gopherbot @findleyr