Refactor Tasks #828

niaow · 2020-01-03T05:25:45Z

No description provided.

niaow · 2020-01-21T21:45:40Z

This PR attempts to create a clean API for interacting with tasks for implementing low level concurrency in the standard library. In the process, coroutine lowering was overhauled and moved into the transform package.

Main changes:

In the coroutines scheduler, rather than each async function making its own task structure, there is now one task structure created at coroutine start. All coroutine handles are stored into this task structure. This is more convenient to work with, and eliminates the need for the old "fake coroutine" mechanism.
Coroutine lowering was completely rebuilt, and is now far more readable. The new version was built in the transform package, and has tests. More kinds of tail calls are now supported, so fewer functions should need to be transformed into coroutines. Therefore, this should reduce code sizes in large applications.
The new internal/task package has a more organized API, with types for common structures like task stacks and queues. These are now used in the runtime package for the runqueue, and will be used later in the sync package to implement concurrency primitives.
On avr, we are not able to use any scheduler. Previously, there was a workaround in the coroutines scheduler for this. Instead, this PR adds a scheduler option called none which avoids scheduling. This is only currently supported on avr, but a future PR will make it available everywhere else.
The mechanism for invoking main has been changed slightly. Rather than implementing a special lowering mechanism in a pass, the runtime entry point now spawns callMain() in a goroutine before invoking the scheduler explicitly. This simplified the compiler substantially, and now all that it has to do is replace all references to callMain with the actual program main function. Additionally, this means that no lowering pass is required for the tasks scheduler.
The IR generator now emits the same IR for starting a goroutine on all schedulers. This eliminates some logic in the compiler and means that we do not need to worry about as many variations in the IR.
The scheduler is always included. At the moment, LLVM cannot optimize it completely away (although this will change once the termination condition is corrected to the termination of main).

deadprogram · 2020-01-27T22:38:32Z

I tried to merge this locally into current dev. Was able to build compiler using go install, but then got this when I tried to use it:

$ tinygo build -o test.hex -size short -target arduino examples/blinky1
# runtime
src/runtime/gc_stack_raw.go:13:12: undeclared name: getSystemStackPointer

niaow · 2020-01-27T22:39:35Z

There is probably another silent merge conflict. I guess I can re-fix that in a bit.

niaow · 2020-01-27T23:40:35Z

Fixed.

deadprogram · 2020-01-28T07:12:07Z

So far I have done testing with the following programs/boards:

digispark - my upcoming ws2812 code
samd21 - ran the Gopherbot demo, also ran the wifinina mqttsub demo
samd51 - ran the gobadge demo
arduino - ran both blinky1 and echo

All of the above programs worked exactly as expected. 👍

aykevl

Thanks a lot for the high-level overview! It definitely sounds like a good improvement.

I still need to take a deep dive in the code, but here are some superficial remarks while scrolling through it.

compiler/goroutine.go

src/internal/task/task_coroutine.go

src/internal/task/queue.go

src/internal/task/task_coroutine.go

src/internal/task/task_stack_cortexm.go

aykevl

The IR generator now emits the same IR for starting a goroutine on all schedulers. This eliminates some logic in the compiler and means that we do not need to worry about as many variations in the IR.

Are you referring to internal/task.start? The IR for it looks a lot simpler now. Apparently internal/task.start creates/starts the new coroutine instead of starting it wherever that call appears.

compiler/compiler.go

src/internal/task/queue.go

src/internal/task/task_none.go

src/internal/task/task_stack.go

src/runtime/scheduler_coroutines.go

transform/goroutine.go

niaow · 2020-02-26T19:24:02Z

The IR generator now emits the same IR for starting a goroutine on all schedulers. This eliminates some logic in the compiler and means that we do not need to worry about as many variations in the IR.

Are you referring to internal/task.start? The IR for it looks a lot simpler now. Apparently internal/task.start creates/starts the new coroutine instead of starting it wherever that call appears.

Yes, that is what I was referring to.

aykevl

I've looked mainly at the tasks scheduler. Hopefully I can look at the coroutines scheduler soon. So far, it looks good but I have a few nits here and there to make the code easier to understand and read.

One thing I'm missing is some sort of high level overview of the new/improved/refactored coroutines scheduler. It has changed in a few ways, but I'm finding it hard to see how exactly (and thus how it works now). There used to be one in src/runtime/scheduler_coroutines.go but it is now gone. I'm not sure what the best place would be, perhaps src/internal/task/task_coroutine.go? Some questions it should probably answer: how are parameters passed to newly started goroutines? How does call/return work? What happens when you do a blocking operation (such as time.Sleep)? Such documentation is super valuable not just to me, but also to people new to the project that want to improve the scheduler.

compiler/compiler.go

compiler/optimizer.go

src/internal/task/task_coroutine.go

src/internal/task/task_none.go

src/internal/task/task_stack.go

src/internal/task/task_stack_cortexm.go

deadprogram · 2020-03-15T00:08:31Z

So would it be correct to say that this PR needs some docs, and then can be merged?

In either case, I suggest adding a separate page for "Task Scheduler" in https://github.com/tinygo-org/tinygo-site/tree/master/content/compiler-internals

niaow · 2020-03-15T00:11:29Z

@deadprogram I added the docs requested in @aykevl 's comment. I could add some additional documentation later.

aykevl

I have reviewed all the code now. It looks very clean, just a few nits here and there.

In either case, I suggest adding a separate page for "Task Scheduler" in https://github.com/tinygo-org/tinygo-site/tree/master/content/compiler-internals

That might be useful but at the same time I'd rather avoid duplicating documentation. You could perhaps put a link to https://github.com/tinygo-org/tinygo/tree/master/transform/coroutines.go where the canonical documentation is kept.

src/internal/task/task_stack_cortexm.go

transform/coroutines.go

aykevl · 2020-03-15T21:36:29Z

transform/coroutines.go

+}
+
+// supplyTaskOperands fills in the task operands of async calls.
+func (c *coroutineLoweringPass) supplyTaskOperands() {


I think this could eventually be done by the compiler directly. It would avoid the issue of possibly optimized out parameters etc. But not in this PR.

transform/coroutines.go

niaow · 2020-03-16T16:04:22Z

I fixed those issues and rebased on dev.

aykevl

I checked code size and in nearly all cases (including WebAssembly!) programs get bigger. Usually not enormously, but still significantly (typically 300-1000 bytes). Do you know why this is the case?

A few examples:

./testdata/alias.go went from 1204 to 2276 bytes on microbit.
./testdata/interface.go stayed at 14760 bytes on microbit, as one of very few tests that stays the same size.
./testdata/stdlib.go went from 72896 to 74404 bytes on WebAssembly.

It looks like something doesn't get optimized out properly when not using the scheduler, because in very small tests (testdata/zeroalloc.go) the size increase is pretty big (+1148 bytes).

I used the following script to compare a whole lot of code at the same time:

#!/bin/sh
for t in testdata/*.go; do echo $t; tinygo build -o test.elf -no-debug -size=short $t; done
for t in testdata/*.go; do echo $t; tinygo build -o wasm.wasm -no-debug $t; /bin/ls -l wasm.wasm; done
for t in testdata/*.go; do echo $t; tinygo build -o test.elf -target microbit -no-debug -size=short $t; done
make smoketest

aykevl · 2020-03-16T17:56:38Z

transform/coroutines.go

+		c.builder.SetInsertPointBefore(user)
+		raw := user.Operand(1)
+		if !raw.IsAUndefValue().IsNil() || raw.IsNull() {
+			return errors.New("undefined task")


This one can also be changed to errorAt.

aykevl · 2020-03-17T10:25:06Z

src/internal/task/task_stack_cortexm.go

@@ -0,0 +1,83 @@
+// +build scheduler.tasks, cortexm


I discovered an issue here while playing around with -scheduler=none: this file is still included. Removing the extra space (to make it scheduler.tasks,cortexm) fixes the issue.

aykevl · 2020-03-17T10:31:16Z

I tested #895 rebased on top of this branch and the size is very similar to what it is now when I add -scheduler=none. In many cases the code size is slightly bigger (usually by 16 bytes) and in some cases it is significantly lower, apparently when the scheduler was needed previously due to time.Sleep but is not needed now. Therefore, I consider the size increase issue to be resolved.
It would be nice however if this could somehow be detected by the compiler - perhaps that can be investigated at a later time.

aykevl · 2020-03-17T11:16:28Z

Merged! 🎉

deadprogram · 2020-03-17T12:18:34Z

@jaddr2line thank you very much for your incredible work on this contribution, and thanks @aykevl for getting it reviewed, this was a big one!

niaow closed this Jan 3, 2020

niaow reopened this Jan 5, 2020

niaow force-pushed the internalcoro branch 2 times, most recently from 19b7638 to 25de588 Compare January 21, 2020 20:41

niaow force-pushed the internalcoro branch from 25de588 to 6945f44 Compare January 23, 2020 02:02

niaow marked this pull request as ready for review January 23, 2020 12:19

niaow force-pushed the internalcoro branch from 6945f44 to 3a0cb77 Compare January 26, 2020 22:54

niaow changed the title ~~refactor tasks WIP~~ Refactor Tasks Jan 26, 2020

niaow force-pushed the internalcoro branch from 1aface0 to 9242668 Compare January 27, 2020 21:26

niaow mentioned this pull request Jan 27, 2020

Implement sync #881

Merged

niaow force-pushed the internalcoro branch 2 times, most recently from 3c0a5a9 to d5c997a Compare January 27, 2020 23:24

niaow requested a review from aykevl January 27, 2020 23:29

niaow force-pushed the internalcoro branch 2 times, most recently from 9dabba2 to 95ada00 Compare February 5, 2020 21:29

niaow force-pushed the internalcoro branch from 95ada00 to f5ea079 Compare February 7, 2020 21:06

niaow mentioned this pull request Feb 7, 2020

Generalize -scheduler=none to support most platforms #895

Merged

aykevl reviewed Feb 13, 2020

View reviewed changes

niaow force-pushed the internalcoro branch from f5ea079 to f8bbf17 Compare February 14, 2020 14:07

aykevl reviewed Feb 20, 2020

View reviewed changes

niaow force-pushed the internalcoro branch 2 times, most recently from 08648f3 to 48c4fac Compare February 21, 2020 23:01

niaow added this to the v0.13 milestone Feb 26, 2020

niaow mentioned this pull request Mar 1, 2020

LLVM ERROR: Coroutines cannot handle non static allocas yet #234

Closed

niaow mentioned this pull request Mar 1, 2020

Possibly spurious error: LLVM ERROR: Coroutines cannot handle non static allocas yet #421

Closed

aykevl reviewed Mar 7, 2020

View reviewed changes

niaow force-pushed the internalcoro branch from 48c4fac to de792bd Compare March 9, 2020 00:37

aykevl reviewed Mar 15, 2020

View reviewed changes

niaow force-pushed the internalcoro branch 3 times, most recently from 52f76ce to b049521 Compare March 16, 2020 15:31

aykevl reviewed Mar 16, 2020

View reviewed changes

refactor coroutine lowering and tasks

70f6062

niaow force-pushed the internalcoro branch from b049521 to 70f6062 Compare March 16, 2020 21:57

aykevl reviewed Mar 17, 2020

View reviewed changes

aykevl merged commit 6a50f25 into tinygo-org:dev Mar 17, 2020

Refactor Tasks #828

Refactor Tasks #828

Uh oh!

Conversation

niaow commented Jan 3, 2020

Uh oh!

niaow commented Jan 21, 2020

Uh oh!

deadprogram commented Jan 27, 2020

Uh oh!

niaow commented Jan 27, 2020

Uh oh!

niaow commented Jan 27, 2020

Uh oh!

deadprogram commented Jan 28, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niaow commented Feb 26, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deadprogram commented Mar 15, 2020

Uh oh!

niaow commented Mar 15, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aykevl Mar 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niaow commented Mar 16, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

aykevl Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

aykevl Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aykevl commented Mar 17, 2020

aykevl Mar 17, 2020 •

edited

Loading