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.

Sign up for GitHub

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

runtime: TestWindowsStackMemory flakes on windows-386-2016 #58570

Open
prattmic opened this issue Feb 16, 2023 · 32 comments
Open

runtime: TestWindowsStackMemory flakes on windows-386-2016 #58570

prattmic opened this issue Feb 16, 2023 · 32 comments
Assignees
@cagedmantis
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@prattmic
Copy link
Member 

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"

2023-01-17T19:55:02-d4639ec/windows-386-2016

--- FAIL: TestWindowsStackMemory (0.03s)
    crash_test.go:58: C:\Users\gopher\AppData\Local\Temp\1\go-build1812702813\testprog.exe StackMemory (27.351ms): ok
    syscall_windows_test.go:675: expected < 102400 bytes of memory per thread, got 103424
FAIL
FAIL	runtime	47.304s

Notes:

  • This is on the brand new windows-386-2016 builder.
  • 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.

cc @mknyszek @golang/runtime @golang/windows
@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 16, 2023
@prattmic prattmic added this to the Backlog milestone Feb 16, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 16, 2023
@prattmic prattmic mentioned this issue Feb 16, 2023
Closed
@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2023-01-17 19:55 windows-386-2016 go@d4639ecd runtime.TestWindowsStackMemory (log) 
--- FAIL: TestWindowsStackMemory (0.03s)
    crash_test.go:58: C:\Users\gopher\AppData\Local\Temp\1\go-build1812702813\testprog.exe StackMemory (27.351ms): ok
    syscall_windows_test.go:675: expected < 102400 bytes of memory per thread, got 103424

— watchflakes

@alexbrainman
Copy link
Member

  • 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 #22439 (comment) . 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.

@mknyszek
Copy link
Contributor

mknyszek commented Mar 1, 2023

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).

@prattmic
Copy link
Member Author

prattmic commented Mar 3, 2023

On CL 473275: https://storage.googleapis.com/go-build-log/67b9e0cb/windows-386-2016_1815f49f.log

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/473415 mentions this issue: runtime: allow for 5 more threads in TestWindowsStackMemory*

@alexbrainman
Copy link
Member

@alexbrainman up for sending a patch?

Here is my CL

https://go-review.googlesource.com/c/go/+/473415

Alex

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Mar 7, 2023
@gopherbot gopherbot reopened this Jun 21, 2023
@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2023-06-21 16:11 windows-386-2016 go@03063101 runtime.TestWindowsStackMemory (log) 
--- FAIL: TestWindowsStackMemory (0.03s)
    crash_test.go:58: C:\Users\gopher\AppData\Local\Temp\1\go-build3536125218\testprog.exe StackMemory (27.3413ms): ok
    syscall_windows_test.go:675: expected < 102400 bytes of memory per thread, got 102440

— watchflakes

@alexbrainman
Copy link
Member

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

@mknyszek
Copy link
Contributor

Sounds like we just need to backport the test fix to the 1.20 branch? I can handle that.

@mknyszek mknyszek self-assigned this Jun 28, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/506975 mentions this issue: [release-branch.go1.20] runtime: allow for 5 more threads in TestWindowsStackMemory*

gopherbot pushed a commit that referenced this issue Jun 28, 2023
@alexbrainman @gopherbot
[release-branch.go1.20] runtime: allow for 5 more threads in TestWind…
08a58dd 
…owsStackMemory*

Original version of TestWindowsStackMemory did not consider sysmon and
other threads running during the test. Allow for 5 extra threads in this
test - this should cover any new threads in the future.

For #58570

Change-Id: I215790f9b94ff40a32ddd7aa54af715d1dc391c6
Reviewed-on: https://go-review.googlesource.com/c/go/+/473415
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit f6cbc1d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/506975
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
@mknyszek
Copy link
Contributor

@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.

This was referenced Jun 28, 2023
Closed
Closed
@alexbrainman
Copy link
Member

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

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/536058 mentions this issue: runtime: allow for 10 more threads in TestWindowsStackMemory*

@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Oct 23, 2023
@gopherbot gopherbot reopened this Jan 25, 2024
@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2024-01-22 16:09 gotip-windows-386 go@b17bf6dd runtime.TestWindowsStackMemory (log) 
=== RUN   TestWindowsStackMemory
    syscall_windows_test.go:671: C:\b\s\w\ir\x\t\go-build4036579385\testprog.exe StackMemory (28.3075ms): ok
    syscall_windows_test.go:677: expected < 102400 bytes of memory per thread, got 102884
--- FAIL: TestWindowsStackMemory (0.03s)

— watchflakes

@mknyszek mknyszek removed their assignment Jan 31, 2024
@mknyszek mknyszek modified the milestones: Go1.21, Backlog Jan 31, 2024
@mknyszek
Copy link
Contributor

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.

@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2024-01-30 20:08 gotip-windows-386 go@b0799674 runtime.TestWindowsStackMemory (log) 
=== RUN   TestWindowsStackMemory
    syscall_windows_test.go:671: C:\b\s\w\ir\x\t\go-build580327174\testprog.exe StackMemory (24.111ms): ok
    syscall_windows_test.go:677: expected < 102400 bytes of memory per thread, got 102809
--- FAIL: TestWindowsStackMemory (0.02s)

— watchflakes

@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2024-08-27 17:40 gotip-windows-386 go@994d1d44 runtime.TestWindowsStackMemory (log) 
=== RUN   TestWindowsStackMemory
    syscall_windows_test.go:671: C:\b\s\w\ir\x\t\go-build2480393434\testprog.exe StackMemory (21.0091ms): ok
    syscall_windows_test.go:677: expected < 102400 bytes of memory per thread, got 103144
--- FAIL: TestWindowsStackMemory (0.02s)

— watchflakes

@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2024-10-23 04:43 gotip-windows-386 go@7d9802ac runtime.TestWindowsStackMemory (log) 
=== RUN   TestWindowsStackMemory
    syscall_windows_test.go:671: C:\b\s\w\ir\x\t\go-build4292561115\testprog.exe StackMemory (26.2509ms): ok
    syscall_windows_test.go:677: expected < 102400 bytes of memory per thread, got 104261
--- FAIL: TestWindowsStackMemory (0.03s)

— watchflakes

@rsc
Copy link
Contributor

rsc commented Nov 17, 2024

Just got one of these in a trybot. All the failures here are just barely over 100kB. Sent CL 628795 to bump the limit to 128 kB.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/628795 mentions this issue: runtime: relax TestWindowsStackMemory from 100kB to 128kB

@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2024-11-17 14:32 gotip-windows-386 go@49b3ab0d runtime.TestWindowsStackMemory (log) 
=== RUN   TestWindowsStackMemory
    syscall_windows_test.go:671: C:\b\s\w\ir\x\t\go-build2739463943\testprog.exe StackMemory (23.0881ms): ok
    syscall_windows_test.go:677: expected < 102400 bytes of memory per thread, got 102660
--- FAIL: TestWindowsStackMemory (0.02s)
2024-11-17 14:32 gotip-windows-386 go@9060fa5a runtime.TestWindowsStackMemory (log) 
=== RUN   TestWindowsStackMemory
    syscall_windows_test.go:671: C:\b\s\w\ir\x\t\go-build2041856165\testprog.exe StackMemory (18.8138ms): ok
    syscall_windows_test.go:677: expected < 102400 bytes of memory per thread, got 106533
--- FAIL: TestWindowsStackMemory (0.02s)

— watchflakes

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Nov 18, 2024
@gopherbot gopherbot reopened this Jan 8, 2025
@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "runtime" && test == "TestWindowsStackMemory"
2025-01-08 17:33 go1.22-windows-386 release-branch.go1.22@223260bc runtime.TestWindowsStackMemory (log) 
=== RUN   TestWindowsStackMemory
    syscall_windows_test.go:671: C:\b\s\w\ir\x\t\go-build3754068840\testprog.exe StackMemory (38.0496ms): ok
    syscall_windows_test.go:677: expected < 102400 bytes of memory per thread, got 102586
--- FAIL: TestWindowsStackMemory (0.04s)

— watchflakes

@github-project-automation github-project-automation bot moved this from Done to In Progress in Go Compiler / Runtime Jan 8, 2025
@github-project-automation github-project-automation bot moved this from Done to Active in Test Flakes Jan 8, 2025
@seankhliao seankhliao mentioned this issue Feb 21, 2025
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/651295 mentions this issue: cmd/go: look at runes, not bytes, when printing env vars

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cagedmantis cagedmantis

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
Go Compiler / Runtime
Status: In Progress
Test Flakes
Status: Active
Milestone
Backlog
Development

No branches or pull requests
9 participants
@rsc @cagedmantis @mknyszek @prattmic @dmitshur @cuonglm @gopherbot @alexbrainman @seankhliao