Open prattmic opened 1 year ago
Found new dashboard test flakes for:
#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
- The test computes the average assuming there are 100 threads. If there are actually 101 threads (the test doesn't consider sysmon...), then the average comes out to exactly 102400 bytes.
The test was suggested by @aclements at https://github.com/golang/go/issues/22439#issuecomment-339981702 . You should be able to adjust the test to allow for a couple of extra threads to be started by runtime without TestWindowsStackMemory noticing. But we want to keep the test to prevent regressions like this one #22439.
Alex.
In triage, I think we agree this is a test issue. @alexbrainman up for sending a patch? If not one of us will get around to it (especially if it happens again).
Change https://go.dev/cl/473415 mentions this issue: runtime: allow for 5 more threads in TestWindowsStackMemory*
@alexbrainman up for sending a patch?
Here is my CL
https://go-review.googlesource.com/c/go/+/473415
Alex
Found new dashboard test flakes for:
#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2023-06-21 16:11 windows-386-2016 go@03063101 runtime.TestWindowsStackMemory
I can see that this failure happened on release-branch.go1.20 release branch - on commit
commit 03063101a2b40e78a44bdc1da84900d41a49a3ec
Author: Ian Lance Taylor <iant@golang.org>
Date: Wed Jun 14 16:17:31 2023 -0700
[release-branch.go1.20] text/template: set variables correctly in range assignment
I unintentionally flipped them in CL 446795.
For #56490
For #60801
Fixes #60802
Change-Id: I57586bec052e1b2cc61513870ce24dd6ce17e56b
Reviewed-on: https://go-review.googlesource.com/c/go/+/503576
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
I suspect the branch does not have CL 473415 fix. Otherwise I don't have explanation for the flake 2 days ago .
Alex
Sounds like we just need to backport the test fix to the 1.20 branch? I can handle that.
Change https://go.dev/cl/506975 mentions this issue: [release-branch.go1.20] runtime: allow for 5 more threads in TestWindowsStackMemory*
@gopherbot Please open backport issues for Go 1.19 and Go 1.20.
This is a test-only fix that is very small and very safe.
Backport issue(s) opened: #61054 (for 1.19), #61055 (for 1.20).
Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.
The backport issues now track the backport fixes.
Change https://go.dev/cl/506976 mentions this issue: [release-branch.go1.19] runtime: allow for 5 more threads in TestWindowsStackMemory*
I have just hit this again with current tip: https://ci.chromium.org/ui/p/golang/builders/try/gotip-windows-386/b8766941830696722705/overview
Re-open the issue to see if it still be problem.
There's another one: https://ci.chromium.org/ui/p/golang/builders/try/gotip-windows-386/b8766892579985845249/overview
Sounds like we should increase the allowed threads from 5 to a bigger number, probably 10.
cc @prattmic @alexbrainman
There's another one: https://ci.chromium.org/ui/p/golang/builders/try/gotip-windows-386/b8766892579985845249/overview
Indeed.
Sounds like we should increase the allowed threads from 5 to a bigger number, probably 10.
@cuonglm why do you think 10 will work better than 5?
5 was selected as per this logic
Do you think runtime changed since CL 473415 (about 6 month ago)?
Thank you.
Alex
@cuonglm why do you think 10 will work better than 5?
5 was selected as per this logic
I saw the comment said that "sysmon and others", without explicitly said that what others are, so I assume we could increase if the test start flaking again.
Interestingly it always fails on windows-386, and never on windows-amd64.
@qmuntal is it possible that there are some extra threads that are started for every single process depending on a particular system used?
For example, every Go process loads a bunch of DLLs. Is it possible that some of those DLLs start their own threads? This could even vary from system to system.
Thank you.
Alex
I saw the comment said that "sysmon and others", without explicitly said that what others are,
I am pretty sure that sysmon
is the only extra thread that is currently started - others will correct me. I added and others
to future-proof my solution.
so I assume we could increase if the test start flaking again.
Yes. We could increase to 10 from 5.
Alex
Change https://go.dev/cl/536058 mentions this issue: runtime: allow for 10 more threads in TestWindowsStackMemory*
Found new dashboard test flakes for:
#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
Looking at this issue again, since we have another failure, our current guess is that getPagefileUsage
probably includes memory other than stack memory, and it's occasionally pushing us over the edge. What exactly is being counted is unclear to us at this point, but this test seems like it's going to be inherently a bit flaky. We should dig into what exactly is being counted here.
Found new dashboard test flakes for:
#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
Found new dashboard test flakes for:
#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2023-01-17T19:55:02-d4639ec/windows-386-2016
Notes:
windows-386-2016
builder.cc @mknyszek @golang/runtime @golang/windows