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: sweep increased allocation count #21297

Closed
heschi opened this issue Aug 3, 2017 · 8 comments
Closed

runtime: sweep increased allocation count #21297

heschi opened this issue Aug 3, 2017 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@heschi
Copy link
Contributor

heschi commented Aug 3, 2017

We have reports inside Google of crashes like:

runtime: nelems=34 nalloc=2 previous allocCount=1 nfreed=65535
fatal error: sweep increased allocation count

I've reproduced the problem and I have some confidence that it's a runtime or compiler bug, so I wanted to make sure that we had an external issue to represent the problem and track it as a release blocker. I'll update this as soon as we have detail that can be shared publicly.

@heschi heschi added this to the Go1.9 milestone Aug 3, 2017
@heschi heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 3, 2017
@ianlancetaylor
Copy link
Contributor

CC @aclements

@ianlancetaylor
Copy link
Contributor

Current analysis suggests a bug in https://golang.org/cl/38091. That adds a fast path for map assignment, and uses it for pointers among other values. When a pointer is stored in the map, no write barrier is used.

@gopherbot
Copy link

Change https://golang.org/cl/53413 mentions this issue: cmd/compile: don't use map{assign,delete}_fast* for pointer-ful types

@gopherbot
Copy link

Change https://golang.org/cl/53414 mentions this issue: runtime: mapassign_* should use typedmemmove to update keys

@randall77
Copy link
Contributor

I have another fix which just uses typedmemmove to write the keys. It's just a 2-line change.

@andreimatei
Copy link
Contributor

FWIW, I've encountered a similar crash with go version go1.8 darwin/amd64:

runtime: nelems=9 nfree=0 nalloc=9 previous allocCount=1 nfreed=65528
fatal error: sweep increased allocation count
runtime: nelems=256 nfree=12 nalloc=244 previous allocCount=223 nfreed=65515
fatal error: sweep increased allocation count

https://golang.org/cl/38091 seems to have been committed in March, so after go 1.8 was released.

@ianlancetaylor
Copy link
Contributor

@andreimatei If you are seeing a problem with Go 1.8, then it is a different problem. Please open a new issue with reproduction instructions. Thanks.

@gopherbot
Copy link

Change https://golang.org/cl/59110 mentions this issue: runtime: optimize storing new keys in mapassign_fastNN

gopherbot pushed a commit that referenced this issue Aug 25, 2017
Prior to this change, we use typedmemmove to write the key
value to its new location in mapassign_fast32 and mapassign_fast64.
(The use of typedmemmove was a last-minute fix in the 1.9 cycle;
see #21297 and CL 53414.)

This is significantly less inefficient than direct assignment or
calling writebarrierptr directly.

Fortunately, there aren't many cases to consider.

On systems with 32 bit pointers:

* A 32 bit AMEM value either is a single pointer or has no pointers.
* A 64 bit AMEM value may contain a pointer at the beginning,
  a pointer at 32 bits, or two pointers.

On systems with 64 bit pointers:

* A 32 bit AMEM value contains no pointers.
* A 64 bit AMEM value either is a single pointer or has no pointers.

All combinations except the 32 bit pointers / 64 bit AMEM value are
cheap and easy to handle, and the problematic case is likely rare.
The most popular map keys appear to be ints and pointers.

So we handle them exhaustively. The sys.PtrSize checks are constant branches
and are eliminated by the compiler.

An alternative fix would be to return a pointer to the key,
and have the calling code do the assignment, at which point the compiler
would have full type information.

Initial tests suggest that the performance difference between these
strategies is negligible, and this fix is considerably simpler,
and has much less impact on binary size.

Fixes #21321

Change-Id: Ib03200e89e2324dd3c76d041131447df66f22bfe
Reviewed-on: https://go-review.googlesource.com/59110
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Aug 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants