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

x/text/unicode/norm: tests run over 10 minutes and time out on js/wasm #31282

Open
bradfitz opened this issue Apr 5, 2019 · 18 comments
Open

x/text/unicode/norm: tests run over 10 minutes and time out on js/wasm #31282

bradfitz opened this issue Apr 5, 2019 · 18 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 5, 2019

What's so hard about x/text/unicode/norm that makes it kill js/wasm?

https://build.golang.org/log/275a5abfdca723fd4574927bd1e076ee93e04ee8

...
ok  	golang.org/x/text/search	7.146s
?   	golang.org/x/text/secure	[no test files]
ok  	golang.org/x/text/secure/bidirule	11.127s
ok  	golang.org/x/text/secure/precis	13.066s
ok  	golang.org/x/text/transform	13.374s
?   	golang.org/x/text/unicode	[no test files]
ok  	golang.org/x/text/unicode/bidi	10.438s
ok  	golang.org/x/text/unicode/cldr	11.382s
*** Test killed with quit: ran too long (10m0s).
FAIL	golang.org/x/text/unicode/norm	600.299s
ok  	golang.org/x/text/unicode/rangetable	32.598s
ok  	golang.org/x/text/unicode/runenames	11.347s
ok  	golang.org/x/text/width	9.961s

/cc @neelance @cherrymui @mpvl

@mpvl
Copy link
Member

@mpvl mpvl commented Apr 5, 2019

One of the tests downloads data, but that should be disabled either way, and doubly so because the --short flag is used.

It runs in a few seconds on my laptop. I had performance issues with this package before a long time ago, but that was at compile/link time. The package has some fairly large tables included in the tests, which may be an issue (see data<Unicode_version>_test.go).

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 5, 2019

Yeah, I only see 1.9 seconds on Linux too. Must be the tables.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 5, 2019

I can reproduce the js/wasm slowness locally with nodejs 8.11.1.

perf top says it's all spent in v8::internal::compiler::UsePosition::HintRegister. Generally pinned at 99%, sometimes dropping to 95%. I don't know how to interpret that.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 5, 2019

And notably, I never see any output even with go test -v:

bradfitz@go:~/src/golang.org/x/text/unicode/norm$ GOOS=js GOARCH=wasm go test -v -short
^Csignal: interrupt
FAIL    golang.org/x/text/unicode/norm  19.709s

And if I add a TestMain func to just skip all tests on js/wasm, I still see it spin 100% CPU ~forever:

bradfitz@go:~/src/golang.org/x/text/unicode/norm$ git dic
diff --git a/unicode/norm/main_test.go b/unicode/norm/main_test.go
new file mode 100644
index 0000000..a7ede11
--- /dev/null
+++ b/unicode/norm/main_test.go
@@ -0,0 +1,16 @@
+package norm
+
+import (
+       "fmt"
+       "os"
+       "runtime"
+       "testing"
+)
+
+func TestMain(m *testing.M) {
+       if runtime.GOOS == "js" {
+               fmt.Fprintf(os.Stderr, "skipping on wasm\n")
+               os.Exit(0)
+       }
+       os.Exit(m.Run())
+}
bradfitz@go:~/src/golang.org/x/text/unicode/norm$ go test -short
PASS
ok      golang.org/x/text/unicode/norm  1.885s
bradfitz@go:~/src/golang.org/x/text/unicode/norm$ GOOS=js GOARCH=wasm go test -v -short
^Csignal: interrupt
FAIL    golang.org/x/text/unicode/norm  12.068s
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 5, 2019

I guess that means the Go code hasn't started to run -- all the time spent in V8 trying to compile the Wasm bytecode to machine code?

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 5, 2019

@cherrymui, oh, I hadn't thought of that. I assumed it was stuck in Go init funcs before TestMain.

@neelance
Copy link
Member

@neelance neelance commented Apr 5, 2019

For me, the WebAssembly.instantiate call in wasm_exec.js does not return in a reasonable amount of time on Node.js v11.12.0 and Chrome. It initializes quickly on Firefox, so the issue seems to be specific to V8.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 29, 2019

Here's the wasm module if others are interested at debugging: norm.test.gz

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 29, 2019

Ben Titzer (V8 team) said we should use V8 7.4+. (so min Node.js 12.x.)

I'll update our builders.

@bradfitz bradfitz self-assigned this Apr 29, 2019
@bradfitz bradfitz added the Builders label Apr 29, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented May 2, 2019

Change https://golang.org/cl/175098 mentions this issue: env/js: update NodeJS from v8.x to v12.x

gopherbot pushed a commit to golang/build that referenced this issue May 3, 2019
Before:

$ docker run -ti gcr.io/symbolic-datum-552/js-wasm /usr/bin/nodejs --version
v8.11.1

After:

$ docker run -ti gcr.io/symbolic-datum-552/js-wasm /usr/bin/nodejs --version
v12.1.0

Also update the Makefile to permit building separately from pushing.

Updates golang/go#31282

Change-Id: I3b5fd47ab41abc7721ffa48bc3f577832db24bb2
Reviewed-on: https://go-review.googlesource.com/c/build/+/175098
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Richard Musiol <neelance@gmail.com>
@bradfitz bradfitz removed their assignment May 29, 2019
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented May 29, 2019

The upgrade to Node 12 didn't fix it. It's at least completing and passing on the dashboard (sometimes?) now, but it's super slow. Logs show a median of 833 seconds over the past week for running go test -short golang.org/x/text/... on js-wasm (which is Node 12).

And I can reproduce locally on my dev machines:

$ node --version
v12.2.0
$ PATH=$GOROOT/misc/wasm:$PATH GOOS=js GOARCH=wasm time go test -v -short golang.org/x/text/unicode/norm
...
(node CPU at 100% for 10+ minutes)
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@agnivade
Copy link
Contributor

@agnivade agnivade commented Jul 30, 2019

It is indeed the tables, but now when I run it in Chrome 75 (linux/amd64), the tests start to run immediately. So I think it's a matter of time before NodeJS catches up to the latest.

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 30, 2019

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 24, 2019

Change https://golang.org/cl/202641 mentions this issue: env/js-wasm: update to Node.js v13.0

gopherbot pushed a commit to golang/build that referenced this issue Oct 24, 2019
I'm hoping the V8 bump gets us past some tests that were too slow
before.

Updates golang/go#31282 (slow x/text/unicode/norm)

Change-Id: I80bdded59c0148c4d3f277acc82c5cac7a339666
Reviewed-on: https://go-review.googlesource.com/c/build/+/202641
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Oct 28, 2019

@titzer, we're using NodeJS 13 now (V8 7.8.279.17) and this is still failing for us.

Could you take a look? Or should we file a V8 bug?

codebien added a commit to codebien/build that referenced this issue Nov 13, 2019
I'm hoping the V8 bump gets us past some tests that were too slow
before.

Updates golang/go#31282 (slow x/text/unicode/norm)

Change-Id: I80bdded59c0148c4d3f277acc82c5cac7a339666
Reviewed-on: https://go-review.googlesource.com/c/build/+/202641
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@neelance
Copy link
Member

@neelance neelance commented Apr 17, 2020

The latest nightly of Node.js still fails to instantiate the WebAssembly module in a reasonable amount of time. It reports the following V8 version:

> node --version
v14.0.0-nightly202004175a4f24e7e1
> node -p process.versions.v8
8.1.307.26-node.12

However, Chrome 80.0.3987.163 with V8 8.0.426.30 is able to instantiate the module in a few seconds.

Node.js is using the newer V8 version, so I guess the issue is on Node.js' side, not V8's.

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
9 participants
You can’t perform that action at this time.