internal/task: use asyncify on WASM #2011

niaow · 2021-07-21T19:16:18Z

This currently requires manually running the compiled wasm through wasm-opt.
Very little works so far.
Issue: #1101

dgryski · 2021-09-24T05:32:32Z

What work is remaining to get this PR finished? I'd love to help out any way I can.

niaow · 2021-09-24T17:25:07Z

This is mainly blocked by #2035 I think

dgryski · 2021-09-24T17:46:44Z

While that bug is getting fixed, is there work that could be done on this issue even with having to run wasm-opt manually?

aykevl · 2021-09-24T20:33:33Z

I'm almost inclined to say that we should perhaps just get this finished (even without #2035). That means compiling for WebAssembly requires an extra binary - but I think that's only required when using the new Asincify based scheduler?

Thoughts?
I don't like adding an external dependency (it makes distribution a whole lot harder) but right now concurrency on WebAssembly is already somewhat broken and this PR might fix it.

niaow · 2021-09-25T14:15:26Z

Alright, I can modify this to just shell out to wasm-opt for now when using asyncify.

niaow · 2021-09-25T15:37:28Z

I made that change and attempted to debug some issues. It appears that after the initial launch suspend, resumed goroutines will not suspend again. I don't quite understand it yet.

dgryski · 2021-09-25T16:22:25Z

I'll take a look at this on Monday and see what I can come up with.

niaow · 2021-10-12T16:17:34Z

I have fixed what was probably the biggest bug. However, there are still some things left to be fixed apparently, such as weird WASI crashes and some sort of unicode error in the reflect test (. . . and i still havent added binaryen to CI).

dgryski · 2021-10-12T20:48:57Z

\o/. This works great on my little test ping pong program (included below)

package main

import (
	"time"
)

func pinger(pinger <-chan int, ponger chan<- int, who string) {
	for {
		<-pinger
		println(who)
		time.Sleep(time.Second)
		ponger <- 1
	}
}

func main() {
	ping := make(chan int)
	pong := make(chan int)

	go pinger(ping, pong, "ping")
	go pinger(pong, ping, "pong")

	ping <- 1

	select {}
}

However, I'm getting an unaligned pointer error with gc=leaking on wasmtime when trying to read the clock.

I managed to fix it with this patch.

 func ticks() timeUnit {
-       var nano uint64
-       clock_time_get(0, timePrecisionNanoseconds, &nano)
+       var b [16]byte
+       ptr := uintptr(unsafe.Pointer(&b[0]))
+       ptr += 8 + (^ptr & 0x7) + 1
+
+       clock_time_get(0, timePrecisionNanoseconds, (*uint64)(unsafe.Pointer(ptr)))
+       nano := *(*uint64)(unsafe.Pointer(ptr))
        return timeUnit(nano)
 }

It seems like a proper fix lies elsewhere, although I'm not sure where..

Trying to run the above ping-pong code with gc=conservative instead produces:

Caused by:
    0: failed to invoke command default
    1: wasm trap: uninitialized element
       wasm backtrace:
           0: 0x6c376 - <unknown>!<wasm function 266>
           1: 0x1489c - <unknown>!<wasm function 68>
           2: 0x2e336 - <unknown>!<wasm function 158>
           3: 0x2d77d - <unknown>!<wasm function 156>
           4: 0x2d636 - <unknown>!<wasm function 155>

I'm still looking into this..

niaow · 2021-10-12T21:21:09Z

However, I'm getting an unaligned pointer error with gc=leaking on wasmtime when trying to read the clock.

I suspect this may be fixed by #2009. This PR is kinda old so I can rebase tommorow.

dgryski · 2021-10-12T21:23:33Z

Adding -g to the options provided to wasm-opt with the following patch:

diff --git a/builder/build.go b/builder/build.go
index df329bd7..7568e8ae 100644
--- a/builder/build.go
+++ b/builder/build.go
@@ -641,7 +641,7 @@ func Build(pkgName, outpath string, config *compileopts.Config, action func(Buil
                                default:
                                        return fmt.Errorf("unknown opt level: %q", config.Options.Opt)
                                }
-                               cmd := exec.Command("wasm-opt", "--asyncify",
+                               cmd := exec.Command("wasm-opt", "-g", "--asyncify",
                                        "--optimize-level", strconv.Itoa(optLevel),
                                        "--shrink-level", strconv.Itoa(shrinkLevel),
                                        executable, "--output", executable)

gives me a much better error:

Caused by:
    0: failed to invoke command default
    1: In func wasi_snapshot_preview1::args_sizes_get at write size: Pointer not aligned to 4: Region { start: 1066183, len: 4 }
       wasm backtrace:
           0: 0x320f - <unknown>!runtime.initAll
           1: 0x307f - runtime.run$1
                           at /Users/dgryski/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:19:10
           2: 0x301d - <goroutine wrapper>
                           at /Users/dgryski/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:18:2
           3: 0x3cb6 - <unknown>!tinygo_launch
           4:  0x538 - (*internal/task.Task).Resume
                           at /Users/dgryski/go/src/github.com/tinygo-org/tinygo/src/internal/task/task_asyncify.go:94:17
           5: 0x2edb - runtime.scheduler
                           at /Users/dgryski/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler.go:171:11
                     - runtime.run
                           at /Users/dgryski/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:24:11
                     - _start
                           at /Users/dgryski/go/src/github.com/tinygo-org/tinygo/src/runtime/runtime_wasm_wasi.go:21:5

So indeed, something about asyncify is futzing with the alignment of the pointers and making things unhappy.

dgryski · 2021-10-12T21:28:38Z

I rebased my local copy against dev with no success. Same pointer alignment issue. I'll keep looking at this..

dgryski · 2021-10-13T01:52:03Z

Patching gc_leaking.go with

 func initHeap() {
-       // preinit() may have moved heapStart; reset heapptr
-       heapptr = heapStart
+       // preinit() may have moved heapStart; reset heapptr and make sure it's aligned
+       heapptr = align(heapStart)
 }

allows both gc=conservative and gc=leaking to run the pingpong example.

niaow · 2021-10-13T16:50:51Z

For some reason I thought chocolatey had binaryen. I guess I will need to switch over the install then.

dgryski · 2021-10-13T20:21:20Z

Something about the asyncify code is causing #2172 to fail

aykevl

This looks pretty clean. I would like to take a more closer look at some parts, but so far I only have some relatively minor comments.

azure-pipelines.yml

compiler/compiler_test.go

aykevl · 2021-10-14T00:38:14Z

src/internal/task/task_asyncify.go

+	s.args = args
+
+	// Create a stack.
+	stackSize = align(stackSize)


Normally, I would expect an aligned stack to be passed in. For example, a stack size of 2048 is very much aligned.
I never really thought about aligning it, but yeah, technically it might need to be aligned.

I don't think this needs to be done at runtime (although there is a good chance it's optimized away in this case): it's only ever specified in code (in compileopts/target.go for example) or in target JSON files. The only way to easily change it is by modifying the JSON file directly, for example. If we're going to align it, I think it would be better to do it at compile time instead of at runtime, for example here:

tinygo/compiler/goroutine.go

Line 125 in 1573826

stackSize = llvm.ConstInt(b.uintptrType, b.DefaultStackSize, false)

This can be done by aligning to 16 bytes, I believe that's nowadays a pretty standard alignment (even on riscv32 and i386, for example).

src/internal/task/task_asyncify.go

targets/wasi.json

testdata/channel.go

aykevl · 2021-10-14T03:19:27Z

For some reason I thought chocolatey had binaryen. I guess I will need to switch over the install then.

scoop has it, not choco.

niaow · 2021-10-14T14:30:04Z

I have created a fix for the WASM scheduler recursion failure: #2178

dgryski · 2021-10-14T20:44:30Z

I think this branch still has an odd failure. Trying to build the following program with
tinygo build -scheduler=asyncify -gc=leaking -target=wasi -o m.wasm main.go

package main

import (
	"context"
)

func main() {
	ctx := context.Background()

	select {
	case <-ctx.Done():
	}
}

produces the following build failure:

Call parameter type does not match function signature!
%runtime.channel.56* null
 %runtime.channel.80*  %1 = call i1 bitcast (i1 (%runtime.channel*, i8*, %runtime.channelBlockedList*, i8*, i8*)* @runtime.chanRecv to i1 (%runtime.channel.80*, i8*, %runtime.channelBlockedList.79*, i8*, i8*)*)(%runtime.channel.56* null, i8* null, %runtime.channelBlockedList.79* %chan.blockedList, i8* undef, i8* null), !dbg !1627

niaow · 2021-10-21T16:29:03Z

I have been poking around, and it currently appears that the interface lowering pass is the first pass introducing type errors when building that code.

niaow · 2021-10-21T17:06:33Z

I identified the issue and seperated it into #2197

niaow · 2021-10-30T16:43:49Z

Hmm, it looks like LLVM 10 may have issues with the not-yet-resolved symbols for asyncify. I am not really sure how we want to handle this. I guess we could disable this on LLVM 10? I am not sure of a better option.

dgryski · 2021-10-30T17:16:57Z

Given that llvm 13 is out now, how many versions behind is tinygo committed to supporting?

aykevl · 2021-10-30T23:19:32Z

Hmm, it looks like LLVM 10 may have issues with the not-yet-resolved symbols for asyncify. I am not really sure how we want to handle this. I guess we could disable this on LLVM 10? I am not sure of a better option.

Don't let that be a blocker. If LLVM 10 is an issue, we'll drop LLVM 10 support. That's a really old version by now. (Usually we support the last two LLVM versions or so, to make things easier for distributions). Even more so because this PR is a blocker to add support for newer LLVM versions.

niaow · 2021-11-11T15:32:32Z

It looks like the version of GCC used by Debian Stretch is too old to build binaryen correctly, and that the cache was hiding this fact.

niaow · 2021-11-11T16:36:06Z

I managed to work around that by using the copy of clang that we build.
Now the only remaining issue is Windows.

niaow · 2021-11-11T16:56:16Z

If my debugging attempts are correct, it looks like binaryen is still trying to dynamically link against libstdc++ and libgcc from mingw, and failing (error code 0xc0000139).

[niaow@finch bin]$ objdump -p wasm-opt.exe | grep "DLL Name:"
	DLL Name: libgcc_s_seh-1.dll
	DLL Name: KERNEL32.dll
	DLL Name: msvcrt.dll
	DLL Name: libwinpthread-1.dll
	DLL Name: USER32.dll
	DLL Name: libstdc++-6.dll

aykevl · 2021-11-11T17:18:52Z

For the TinyGo build I use -static -static-libgcc -static-libstdc++, I think you might need those for binaryen too.

niaow · 2021-11-11T17:20:38Z

Yeah I just realized that. Adding now.

aykevl · 2021-11-11T17:21:54Z

I can try rebooting to Windows to test some build options, would that help?

niaow · 2021-11-11T17:22:46Z

I suspect that I just fixed it. If not, I finally have a Windows VM setup so I can test there.

niaow · 2021-11-11T17:47:18Z

All tests are now passing.

This change implements a new "scheduler" for WebAssembly using binaryen's asyncify transform. This is more reliable than the current "coroutines" transform, and works with non-Go code in the call stack. runtime (js/wasm): handle scheduler nesting If WASM calls into JS which calls back into WASM, it is possible for the scheduler to nest. The event from the callback must be handled immediately, so the task cannot simply be deferred to the outer scheduler. This creates a minimal scheduler loop which is used to handle such nesting.

niaow · 2021-11-11T17:50:15Z

I did a quick squash/cleanup, so I think this is ready for a final review.

deadprogram · 2021-11-11T18:32:18Z

Do we need to modify the dev Dockerfile if you want to use this scheduler? https://github.com/tinygo-org/tinygo/blob/dev/Dockerfile#L23-L28

Dockerfile

Makefile

Oops I forgot to install cmake and ninja.

deadprogram

All tests are now passing, and seems good to me.

deadprogram · 2021-11-12T13:03:29Z

@aykevl any last comments before merge?

gedw99 · 2021-11-12T13:11:22Z

Is this the fix to get WASM compilation working again ?

deadprogram · 2021-11-12T13:22:34Z

Is this the fix to get WASM compilation working again ?

WASM compilation is working afaik. This PR is to add asyncify as an option for task scheduling for WASM programs.

gedw99 · 2021-11-13T13:45:16Z

Is this the fix to get WASM compilation working again ?

WASM compilation is working afaik. This PR is to add asyncify as an option for task scheduling for WASM programs.

@deadprogram

ok thank for the response.

#1972 is blocking one . I believe is for gio when building to Web ( wasm ). I need to try again...

aykevl

This looks good to me.
I can't really comment on the Dockerfile changes, is the issue there fixed now? I don't know a whole lot about Docker.

aykevl · 2021-11-14T01:30:03Z

goenv/goenv.go

 	default:
 		return ""
 	}
 }

+// Find wasm-opt, or exit with an error.
+func findWasmOpt() string {


[this comment was intended to be part of the review but I forgot to click the "start review" button]

I think this function can be simplified. In short: the bundled wasm-opt binaries don't need to be checked for a version.

I would suggest the following logic:

if $TINYGOROOT/bin/wasm-opt[.exe] exists: return this path if $TINYGOROOT/build/wasm-opt[.exe] exists: return this path look for wasm-opt using LookPath if not found: print or return error check for version if version too old: return error return this path

This is not a blocker to me.

I think we should still be doing a version check regardless for now, until we can actually link it in properly.

This has already caught one issue, so it is still helpful.

niaow · 2021-11-14T01:54:52Z

The Dockerfile issues have been fixed.

deadprogram · 2021-11-14T09:48:46Z

OK merge time! Great work on this @niaow thank you. Also thanks to everyone who helped review/test.

niaow force-pushed the asyncify branch from a4e95f9 to 6121f72 Compare Sep 25, 2021

dkegel-fastly mentioned this pull request Oct 7, 2021

builder: run binaryen after the linker when generating wasm #2035

Closed

niaow force-pushed the asyncify branch from 0d0f3fa to 6d1ede2 Compare Oct 12, 2021

dgryski mentioned this pull request Oct 13, 2021

infinite recursion trying to create recursive function type #2172

Closed

niaow mentioned this pull request Oct 13, 2021

runtime/compiler/transform (gc): generated code includes many 0-sized heap allocation calls #2173

Closed

aykevl reviewed Oct 14, 2021

View changes

niaow force-pushed the asyncify branch from cb2023d to 99e8d50 Compare Oct 30, 2021

niaow force-pushed the asyncify branch from 8a2a3a9 to 5657fb1 Compare Nov 11, 2021

aykevl mentioned this pull request Nov 11, 2021

runtime timers are unimplemented #1037

Closed

niaow force-pushed the asyncify branch from 86dd4a5 to 0b7ccb1 Compare Nov 11, 2021

niaow requested a review from aykevl Nov 11, 2021

deadprogram reviewed Nov 11, 2021

View changes

Dockerfile Show resolved Hide resolved

deadprogram reviewed Nov 11, 2021

View changes

Makefile Outdated Show resolved Hide resolved

fix binaryen build in docker

ff18b3f

Oops I forgot to install cmake and ninja.

deadprogram approved these changes Nov 11, 2021

View changes

aykevl reviewed Nov 14, 2021

View changes

deadprogram approved these changes Nov 14, 2021

View changes

deadprogram merged commit 539e132 into tinygo-org:dev Nov 14, 2021
6 checks passed

aykevl mentioned this pull request Nov 25, 2021

Add LLVM 12 support #2300

Merged

internal/task: use asyncify on WASM #2011

internal/task: use asyncify on WASM #2011

Conversation

niaow commented Jul 21, 2021

dgryski commented Sep 24, 2021

niaow commented Sep 24, 2021

dgryski commented Sep 24, 2021

aykevl commented Sep 24, 2021

niaow commented Sep 25, 2021

niaow commented Sep 25, 2021

dgryski commented Sep 25, 2021

niaow commented Oct 12, 2021 • edited

dgryski commented Oct 12, 2021

niaow commented Oct 12, 2021

dgryski commented Oct 12, 2021

dgryski commented Oct 12, 2021

dgryski commented Oct 13, 2021 • edited

niaow commented Oct 13, 2021 • edited

dgryski commented Oct 13, 2021

aykevl left a comment

aykevl Oct 14, 2021

Choose a reason for hiding this comment

aykevl commented Oct 14, 2021

niaow commented Oct 14, 2021

dgryski commented Oct 14, 2021 • edited

niaow commented Oct 21, 2021 • edited

niaow commented Oct 21, 2021 • edited

niaow commented Oct 30, 2021

dgryski commented Oct 30, 2021

aykevl commented Oct 30, 2021 • edited

niaow commented Nov 11, 2021

niaow commented Nov 11, 2021 • edited

niaow commented Nov 11, 2021

aykevl commented Nov 11, 2021

niaow commented Nov 11, 2021

aykevl commented Nov 11, 2021

niaow commented Nov 11, 2021

niaow commented Nov 11, 2021

niaow commented Nov 11, 2021

deadprogram commented Nov 11, 2021

deadprogram left a comment

deadprogram commented Nov 12, 2021

gedw99 commented Nov 12, 2021

deadprogram commented Nov 12, 2021

gedw99 commented Nov 13, 2021 • edited

aykevl left a comment

aykevl Nov 14, 2021

Choose a reason for hiding this comment

niaow Nov 14, 2021 • edited

Choose a reason for hiding this comment

niaow Nov 14, 2021

Choose a reason for hiding this comment

niaow commented Nov 14, 2021

deadprogram commented Nov 14, 2021

niaow commented Oct 12, 2021 •

edited

dgryski commented Oct 13, 2021 •

edited

niaow commented Oct 13, 2021 •

edited

dgryski commented Oct 14, 2021 •

edited

niaow commented Oct 21, 2021 •

edited

niaow commented Oct 21, 2021 •

edited

aykevl commented Oct 30, 2021 •

edited

niaow commented Nov 11, 2021 •

edited

gedw99 commented Nov 13, 2021 •

edited

niaow Nov 14, 2021 •

edited