Skip to content

internal/task: remove coroutines #2448

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

Merged
merged 3 commits into from
Jan 19, 2022
Merged

Conversation

niaow
Copy link
Member

@niaow niaow commented Dec 30, 2021

No description provided.

@niaow niaow force-pushed the remove-coroutines branch from 58d2bd9 to 2b609e2 Compare December 30, 2021 17:37
@niaow
Copy link
Member Author

niaow commented Dec 30, 2021

This is a bit bigger than I expected, so I don't think it is necessarily a good idea to include this for this release. Next release cycle I would like to refactor a lot of the things that used to depend on coroutines.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phew! That's indeed a big PR. I did expect it to be pretty big though...

I think I would have tried to split this into multiple commits (remove coroutines scheduler, remove switch implementation of functions, and then remove the parentHandle parameter). But I'm not going to complain.

We might want to test AVR some more just to be sure, see below.

Anyway, giving a preliminary LGTM now.

Comment on lines -174 to -181
case "none", "coroutines":
// As "doubleword", but with the function pointer replaced by a unique
// ID per function signature. Function values are called by using a
// switch statement and choosing which function to call.
// Pick the switch implementation with the coroutines scheduler, as it
// allows the use of blocking inside a function that is used as a func
// value.
return "switch"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: this does have an effect on AVR which used -scheduler=none by default. This used to be necessary because function pointers used to be buggy in AVR but I believe it should work now (except a very small increase in binary size sometimes which I think is acceptable).

@niaow
Copy link
Member Author

niaow commented Dec 30, 2021

I think I would have tried to split this into multiple commits

Yeah, I can try to do that

@niaow niaow force-pushed the remove-coroutines branch from 2b609e2 to ef2fec3 Compare December 31, 2021 20:13
@niaow
Copy link
Member Author

niaow commented Dec 31, 2021

This PR has now been split into 3 less-huge commits.

@niaow niaow marked this pull request as ready for review December 31, 2021 20:33
@niaow niaow changed the title WIP: remove coroutines internal/task: remove coroutines Dec 31, 2021
@niaow niaow force-pushed the remove-coroutines branch from ef2fec3 to 66a98ef Compare January 1, 2022 17:58
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
Feel free to fix (probably trivial) merge conflicts and merge the PR.

niaow added 3 commits January 19, 2022 08:28
The switched func lowering was mainly necessary for coroutines.
With coroutines removed, this is no longer necessary.
This removes the parentHandle argument from the internal calling convention.
It was formerly used to implment coroutines.
Now that coroutines have been removed, it is no longer necessary.
@niaow niaow force-pushed the remove-coroutines branch from 66a98ef to d239909 Compare January 19, 2022 13:41
@niaow
Copy link
Member Author

niaow commented Jan 19, 2022

I need to look into why there is an issue with the closure on Windows.

@niaow
Copy link
Member Author

niaow commented Jan 19, 2022

It seems to be a random error. . . which is concerning. I also distinctly remember running into that exact error before (so I don't think it is caused by this PR). I strongly suspect that the actual fix is #2433, which I now realize I forgot to finish debugging.

@niaow
Copy link
Member Author

niaow commented Jan 19, 2022

I created #2551 for now

@niaow
Copy link
Member Author

niaow commented Jan 19, 2022

IDK, does anyone object to merging this now?

@dkegel-fastly
Copy link
Contributor

I'm all for merging it if it will prevent bad user experiences. We can always resurrect coroutines later...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants