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: js-wasm builder broken due to timeout #34768

Closed
katiehockman opened this issue Oct 8, 2019 · 11 comments
Closed

runtime: js-wasm builder broken due to timeout #34768

katiehockman opened this issue Oct 8, 2019 · 11 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS release-blocker
Milestone
Go1.14

Comments

@katiehockman
Copy link
Contributor

The js-wasm builder is consistently failing after https://go-review.googlesource.com/c/go/+/199537 was merged. It's unclear exactly what exactly is going wrong from the test output, but this seems suspicious:

##### ../test/bench/go1

##### ../test

##### 
Test "test:4_10" ran over 20m0s limit (20m0.001274792s)

https://build.golang.org/log/0ebc31b62df29629efb94fc41ea07895f0b346bd

/cc @aclements @cherrymui @neelance
@katiehockman katiehockman added OS-JS NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 8, 2019
@katiehockman katiehockman added this to the Go1.14 milestone Oct 8, 2019
@neelance
Copy link
Member

neelance commented Oct 8, 2019

It hangs here:

go/test/goprint.go

Lines 18 to 20 in ed7e430

for runtime.NumGoroutine() > 1 {
time.Sleep(10*time.Millisecond)
}

The additional goroutine that handles async events now also counts for NumGoroutine, so it never becomes 1.

@aclements @cherrymui I am not sure how to solve this the cleanest way. Do we need to adapt the test? Should the additional goroutine not count for NumGoroutine? Or should it not count as a user goroutine at all as defined by isSystemGoroutine?

@cherrymui
Copy link
Member

In my opinion runtime.NumGoroutine should probably include the callback goroutine, as it is mostly running user code instead of runtime code. Then we should adjust the test. Unless there are a lot of code depending on the old behavior?

@cherrymui
Copy link
Member

Related: could we start the callback goroutine lazily? Maybe at the first time a JS callback is created?

@dmitshur dmitshur added the arch-wasm WebAssembly issues label Oct 9, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@bcmills bcmills modified the milestones: Backlog, Go1.14 Oct 9, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 9, 2019

If the fix for this test isn't trivial enough to apply right away, CL 199537 should be rolled back so that it does not mask other regressions on the builder. (We can roll it forward again when the fix for the test is ready.)

@neelance
Copy link
Member

neelance commented Oct 9, 2019

I'll have a fix ready by tomorrow. :)

@gopherbot
Copy link

Change https://golang.org/cl/200477 mentions this issue: dashboard: re-enable js/wasm test dir tests

@gopherbot
Copy link

Change https://golang.org/cl/200438 mentions this issue: test: adjust a test to work with js/wasm's background goroutine

gopherbot pushed a commit to golang/build that referenced this issue Oct 10, 2019
@bradfitz
dashboard: re-enable js/wasm test dir tests
ed99f86 
They were removed to make the build faster but they've let a few
bugs slip by now.

Re-enable.

The 10 test shards adds 812 seconds of test time. Over 5 machines,
that's 162.4 seconds each. Add another test shard to bring it down
more.

js-wasm trybots are currently at ~350 seconds, so we have some budget
to get a bit slower and still be under 5 minutes.

Updates golang/go#34768

Change-Id: Ic40c226d26e873227510e644756b0cc969151186
Reviewed-on: https://go-review.googlesource.com/c/build/+/200477
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/200497 mentions this issue: runtime: make goroutine for wasm async events short-lived

gopherbot pushed a commit that referenced this issue Oct 11, 2019
@neelance
runtime: make goroutine for wasm async events short-lived
2686e74 
An extra goroutine is necessary to handle asynchronous events on wasm.
However, we do not want this goroutine to exist all the time.
This change makes it short-lived, so it ends after the asynchronous
event was handled.

Fixes #34768

Change-Id: I24626ff0af9d803a01ebe33fbb584d04d2059a44
Reviewed-on: https://go-review.googlesource.com/c/go/+/200497
Run-TryBot: Richard Musiol <neelance@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@neelance
Copy link
Member

@bradfitz Do you want to keep https://golang.org/cl/200438 ?

@bradfitz
Copy link
Contributor

@neelance, yeah, it's harmless. And I like that it has a deadline now, rather than looping forever. Let's just keep it. There's nothing really wasm-specific in there.

@neelance
Copy link
Member

All right.

@gopherbot gopherbot mentioned this issue Oct 28, 2019
Closed
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@bradfitz @codebien
dashboard: re-enable js/wasm test dir tests
c0b026a 
They were removed to make the build faster but they've let a few
bugs slip by now.

Re-enable.

The 10 test shards adds 812 seconds of test time. Over 5 machines,
that's 162.4 seconds each. Add another test shard to bring it down
more.

js-wasm trybots are currently at ~350 seconds, so we have some budget
to get a bit slower and still be under 5 minutes.

Updates golang/go#34768

Change-Id: Ic40c226d26e873227510e644756b0cc969151186
Reviewed-on: https://go-review.googlesource.com/c/build/+/200477
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Oct 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS release-blocker
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
8 participants
@bradfitz @neelance @rsc @dmitshur @bcmills @gopherbot @cherrymui @katiehockman