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

image/gif: TestDecodeMemoryConsumption flake on dragonfly-amd64 #35166

Open
bcmills opened this issue Oct 25, 2019 · 4 comments
Open

image/gif: TestDecodeMemoryConsumption flake on dragonfly-amd64 #35166

bcmills opened this issue Oct 25, 2019 · 4 comments
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Oct 25, 2019

Seen on the dragonfly-amd64 builder (https://build.golang.org/log/40aa63372648fd03bc608538deaf94c00c314369):

--- FAIL: TestDecodeMemoryConsumption (0.14s)
    reader_test.go:422: Decode of 3000 frames increased heap by 77MB
FAIL
FAIL	image/gif	0.214s

Possibly a GC bug? (CC @aclements @mknyszek @danscales)

@bcmills bcmills added this to the Backlog milestone Oct 25, 2019
@danscales
Copy link

@danscales danscales commented Nov 6, 2019

I reproduced by using dragonfly gomote, and repeatedly calling:

gomote run user-danscales-dragonfly-amd64-0 go/bin/go test -test.count=100 image/gif

Reproduced about 4 times in 50 runs.

I changed the test so that when failure happens (because heap is more than 30MB bigger at end of decode than at the beginning), the test does a runtime.GC() and then measures the heap difference. This new code shows that GC fully recovers the 77MB and actually 4MB more. So, I'm guessing this is not a bug, just a case where sometimes memory is not quite scanned/freed in the same way by the initial GC call.

If this happens a lot, we should probably just change the test threshold from 30MB to 100MB, or something like that.

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Nov 6, 2019

There was a GC pacing bug floating around that I fixed in the 1.14 cycle (#28574) wherein assists didn't kick in often enough in GOMAXPROCS=1 situations and you ended up with accidental, unnecessary heap growth. There's a chance this is something in that vein?

@danscales
Copy link

@danscales danscales commented Nov 6, 2019

Hmmm, I should have said that I reproduced it at the change where the bug was originally reported, so that is at 316fb95 (Around Oct 25th). So the pacing fix for #28574 (submitted Sept 4th) would have been included, but the problem still occurred.

But I justed tried with most recent master (today), and I can't seem to reproduce the problem. So, was there any other GC pacing change made recently (between Oct. 25th and Sept 4th)?

@mknyszek

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Nov 6, 2019

@danscales I should clarify: I figured this is unrelated to the fix from a while ago given timelines, but I did recently land a GC pacing change as part of the allocator work, which may account for the difference you're seeing here?

In particular, we prevent the mark phase from starting too early which leads to more assists, but prevents a pacing-related heap size explosion if you're allocating rapidly. I figured this wouldn't have much of an effect without my other patches, but maybe it's doing something here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.