Improved blocking

[niaow]

This adds buffered channels, changing the channel implementation as little as possible.

[niaow]

fixes #510

[niaow]

After some discussion, we decided that we should refactor how we handle blocking before we add any more complexity to this.

[niaow]

The improved blocking has been added

[aykevl]

@jadr2ddude can you rebase on the dev branch? There are some conflicts.
Also, I just enabled building pull requests, which will show that the current PR cannot build for ARM microcontrollers (duplicate function definition).

[aykevl]

I did some measurements, and the changes in d6f7e54...dcd49ef caused a massive file size increase for WebAssembly targets. All tests increased significantly in size, with some of the worst offenders (in bytes) being binop.go (2747 -> 5740), channel.go (24884 -> 29216), float.go (4558 -> 7212), and init_multi.go (1043 -> 3702).

I'm afraid that increase in size is too much to be mergeable. It's not a problem if a refactor does increase output file size if it's small and the maintainability benefits are big, but such a huge increase in file size is simply not worth it.

On the other hand, the first commit d6f7e54 only increases the channel.go test case in size by a somewhat significant size: 23022 -> 24884. But this is expected. The only other file size change I see is in reflect.go and is only a single byte so not a problem.
What do you think, maybe we can do just the buffered channel commit and look at refactoring the scheduler later?

[niaow]

@aykevl I think I might know why that is occurring, and I think I might be able to fix it. It most likely has something to do with my changes to deadlock. Unfortunately, the code LLVM was generating before was not necessarily correct. However, I should be able to come up with something that is both correct and efficient.

[niaow]

The growth in fully synchronous code has now been fixed. There are still a few things remaining to be fixed.

[deadprogram]

In addition to the other things you mention @jadr2ddude there are also now merge conflicts that need to be resolved.

[niaow]

Yes I know. I am actually trying an alternate approach now to see if it works better.

[niaow]

I am working on the merge conflicts now

[niaow]

The reason that the tests are failing is because the test data was updated incorrectly. I am not quite sure how to fix it - @aykevl is there something I did wrong with the test data?

[niaow]

So at this point, this is probably ready for review. There are a bunch of optimizations I want to do at some point, but I think this just needs to get in first.

Note: I still have no idea how to update testdata/channel.txt

[niaow]

Despite the failure of that one build I think this is working correctly. The testdata/channel.go test is just flaky.

[niaow]

I squashed this down to one commit.

[niaow]

fixes #526

[deadprogram]

@jadr2ddude this need the merge conflict from #532 resolved now, if you please.

[aykevl]

I skimmed through it and here are some comments. Note that I haven't yet done a full review.

Also, I did a new size test (at 217f5c2) and it still significantly increases the size of some builds that do not use any blocking calls. In particular, testdata/float.go, testdata/binop.go, testdata/init_multi.go, etc. Did I miss something?

The way I measured was with this command (using the fish shell):

for t in testdata/*.go; echo $t; tinygo build -o wasm.wasm -no-debug $t; /bin/ls -l wasm.wasm; end

[aykevl]

Good one! I hadn't thought about that and although it costs a bit in performance, it should hopefully reduce false positives in the GC.

[niaow]

@aykevl the size regression should be fixed in 60bdf4f

[aykevl]

I confirmed that the size increase on wasm is no longer present. However, I still see a rather significant increase in code size for code that doesn't need a scheduler (~450 bytes flash and ~90 bytes RAM) - in this case on the micro:bit. Is it really necessary to enable the scheduler in all cases?
Code to test (fish shell):

for t in testdata/*.go; echo $t; tinygo build -o test.elf -target microbit -no-debug -size=short $t; end

testdata/binop.go size increase:

testdata/binop.go
   code    data     bss |   flash     ram
   2148       0    2196 |    2148    2196
testdata/binop.go
   code    data     bss |   flash     ram
   3252      56    2228 |    3308    2284

Remember, the whole purpose of TinyGo is small binaries. A lot of features would be simpler if this requirement would be dropped - for example reflect would be a whole lot easier to implement. So the benefits have to be big to allow such a size increase.

@deadprogram what's your view on this?

[niaow]

@aykevl When I fixed scheduler inclusion logic earlier, it seems that I did not update the logic for the tasks scheduler. That should be now fixed.

[aykevl]

Great, the size increase for cortex-m appears to be solved!

[aykevl]

I did some more size comparisons, and it turns out that this PR reduces binary size in many cases (make smoketest RISCV=0). I'm not sure why, but it might need some extra testing in case it is caused by a bug (see #507 for example). I couldn't find any unexpected increases in size, so it's good on that front 😄

[niaow]

@aykevl We do most certainty have correct size decreases in some cases. On WASM, I actually changed it so the scheduler is not always included. It is also entirely possible that some of this is easier for LLVM to optimize.

[deadprogram]

I tried to test this branch using the "apa102" example that is part of the drivers using the "Bluepill". It did not work as expected, although it does work in the dev branch.

The issue is that LEDs lightup but do not cycle. I also tested with the ItsyBitsy-M4 and did not work as expected either, with the same issue..

I did merge in the latest SPI fix for STM32 into this branch before testing, so seems like something else wrong with the time on the STM32 possibly.

I also tested both the apa102 and the echo example using this branch on the ItsyBity-M4 and neither worked as expected.

I tested the "flightstick" example using the Arduino Nano33 and it worked perfectly using this branch, so it might be some platform specific or related to that specific example that allows it to work. I suspect that it has to do with the flightstick uses a go routine, and the other examples do not.

[niaow]

@deadprogram It appears that we hit a false positive, and that may not be using the scheduler even though it needs it. The large gap between the coroutines implementation and the tasks implementation seems to be a larger problem than I anticipated. . .

[aykevl]

I looked at the last commit, it looks good to me. I checked file sizes again, and there is nothing unexpected (only the known 500b flash, 56b RAM increase on Cortex-M, and obviously the channels test has grown bigger). So from my POV, nothing is blocking this PR anymore.

@deadprogram can you run your tests again, to make sure it breaks none of your code? This is a really large PR so there's a decent chance it breaks something.

[deadprogram]

I ran thru a group of tests on several different physical hardware devices, including code against a couple of different processors.

Everything worked exactly as it had previously.

Based on all that I am going to squash/merge this into dev.

Impressive work @jadr2ddude thank you for this contribution.