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

Issue 12090 - Make std.concurrency compatible with fibers as threads #1910

Merged
merged 2 commits into from Oct 22, 2014

Conversation

complexmath
Copy link
Member

@DmitryOlshansky
Copy link
Member

@complexmath Needs rebase

@John-Colvin
Copy link
Contributor

ping. Still needs rebasing.

@andralex
Copy link
Member

test failures, rebase

@complexmath
Copy link
Member Author

rebased

On May 19, 2014, at 5:15 PM, Andrei Alexandrescu notifications@github.com wrote:

test failures, rebase


Reply to this email directly or view it on GitHub.

@mihails-strasuns
Copy link

Seems to include some unrelated commits right now.

@complexmath
Copy link
Member Author

Well how the heck did that happen? Should I submit a new pull request, or does someone know how to remove those?

@yebblies
Copy link
Member

Try this: #1153 (comment)

@complexmath
Copy link
Member Author

That worked. Thanks!

@mihails-strasuns
Copy link

Now it has two commits with identical title (first one being trivial changes) ;) You probably want to squash it all together later. Anyway, going to review this soon!

Also pinging @s-ludwig


interface Scheduler
{
void start( void delegate() op );
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be used anywhere but in the implementations of spawn, maybe it should be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what it's for. The Scheduler allows different concurrency mechanisms to back the threading in std.concurrency. The start function is for creating these threads.

Copy link
Member

Choose a reason for hiding this comment

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

I mean the implementations of ThreadScheduler.spawn and FiberScheduler.spawn. If it was used in std.concurrency.spawn then that might serve as an example of its utility, but as it is, it looks like Scheduler.spawn is the only required primitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, this definitely needs to be documented because it isn't evident from the code. start() isn't strictly required in many cases. That's intended to be called in Dmain so everything, even the main thread, is run by the scheduler. This is necessary for vibe.d, for example, but many other types of schedulers will work without it.

@JakobOvrum
Copy link
Member

This adds public but undocumented types along with the apparently user-facing scheduler variable. Is there a rationale for them being undocumented? It makes the intention of the PR slightly unclear.

@complexmath
Copy link
Member Author

Scheduler should really be documented. Though the two implemented examples were meant to serve as documentation of their own.

@complexmath
Copy link
Member Author

I've added some documentation. Let me know how it looks.

@mihails-strasuns
Copy link

Can you please add some examples of using FiberScheduler? After reading docs and API I have tried something like this and failed:

import std.concurrency;
import core.thread;
import std.stdio;

void spawned(Tid tid)
{
    for (;;)
    {   
        writeln("waiting for numbers");

        scheduler.yield();

        receive(
            (int i) { writefln("received number %s", i); }
        );
    }   
}

void main()
{
    scheduler = new FiberScheduler();

    auto tid = spawn(&spawned, thisTid);

    writefln("main after spawn");

    foreach (i; 0..5)
    {
        writefln("sending %s", i); 
        tid.send(i);

        scheduler.yield();
    }   
}

It works as I expected it to work when ThreadScheduler is used.

Another thing is that I am worried about scheduler being null by default. That means task implementation needs always to check its state before yielding which results in unnecessary boilerplate. Should be probably hidden behind free function, similar to spawn.

Sorry for "soon" being so long :)

@complexmath
Copy link
Member Author

Try

import std.concurrency;
import core.thread;
import std.stdio;

void spawned(Tid tid)
{
    for (;;)
    {   
        writeln("waiting for numbers");

        scheduler.yield(); // explicit yield is unnecessary as receive will yield if it blocks, but can't hurt

        receive(
            (int i) { writefln("received number %s", i); }
        );
    }   
}

void main()
{
    scheduler = new FiberScheduler();
    scheduler.start({
        auto tid = spawn(&spawned, thisTid);

        writefln("main after spawn");

        foreach (i; 0..5)
        {
            writefln("sending %s", i); 
            tid.send(i);

            scheduler.yield();
        }
    });   
}

scheduler.start() starts the dispatcher, which is necessary for FiberScheduler. yield() will start it as well, but that could lead to weird behavior as execution won't (currently) return to the yielding thread until all fibers terminate. I'll see about adding an example.

@mihails-strasuns
Copy link

as execution won't (currently) return to the yielding thread until all fibers terminate

Ah this is what has confused me. is this because of implementation difficulties or intentional? Anyway, should be mentioned in FiberScheduler docs clearly.

This works and should be probably added as documented unittest to module description as it makes obvious difference between fiber and thread schedulers:

import core.thread;
import std.stdio;

void spawned(Tid tid)
{
    bool finished = false;

    for (;;)
    {
        receive(
            (int i) { writefln("received number %s", i); },
            (Tid t) { finished = true; }
        );

        if (finished)
            return;
    }
}

void main()
{
//    scheduler = new FiberScheduler();
    scheduler = new ThreadScheduler();

    scheduler.start({
        auto tid = spawn(&spawned, thisTid);

        foreach (i; 0..5)
        {
            writefln("sending %s", i); 

            tid.send(i);

            scheduler.yield();
        }

        tid.send(thisTid);
    });
}

@mihails-strasuns
Copy link

Also, @s-ludwig - your feedback is still necessary :P

@complexmath
Copy link
Member Author

Your comment about having a standalone yield got me thinking. I have another pull request blocked by this one that was going to add a standalone yield for another purpose, and I've realized that for it to work correctly, the user should never call scheduler.yield() explicitly. So I think it's a good idea to get the standalone yield in right now, and I may just merge the two pull requests for this reason even though they aren't strictly otherwise related. I'll see about improving the documentation for Scheduler as well. I think scheduler.start() should always be called in main even if some schedulers don't require it just for the sake of simplicity.

@s-ludwig
Copy link
Member

s-ludwig commented Jun 6, 2014

I'll try to find some time to build a custom DMD/Phobos and make a demo integration for vibe.d over the next days, as that's the only definitive way to find out the last possible issues, I guess.

Just a little technical documentation comment: There are some doc comments without a short summary paragraph (see "Sections" at http://dlang.org/ddoc.html), which will result in some crowded overview tables in the DDOX documentation layout.

@complexmath
Copy link
Member Author

Thanks @s-ludwig. From previous discussion, I've also merged in my other pull request with this one. This seemed appropriate since it affects the yield() implementation. I've also added a unittest that tests both the FiberScheduler and this new Generator type.

@mihails-strasuns
Copy link

Test failure because of "statement is not reachable" warning
(new stuff looks awesome on the first glance)

@complexmath
Copy link
Member Author

Random Win64 breakage? That errorlevel seems weird.

@complexmath
Copy link
Member Author

I guess this means that fibers are broken on win64? Is there anyone that can look into this?

@s-ludwig
Copy link
Member

s-ludwig commented Jun 8, 2014

They definitely were broken until recently: http://issues.dlang.org/show_bug.cgi?id=12800

@complexmath
Copy link
Member Author

Hrm. Then I'm at a loss to explain two successive test failures on Win64 only. It doesn't appear to be displaying any output at all from the test run on that platform. Could the failure be elsewhere?

@s-ludwig
Copy link
Member

s-ludwig commented Jun 8, 2014

Getting vibe.d to compile on DMD master was a bit more involved than I had hoped for due to some regressions, but now I've got something that seems to work (start and yield are just implemented as no-ops). Now there is just one major issue, assuming I didn't misunderstand something. In vibe.d, there are two flavors of spawn aka runTask and runWorkerTask. The former starts a new task in a fiber of the current thread, while the latter starts it in a fiber of a different thread.

But If I see it right, tasks currently have to be started using spawn, if they are supposed to be usable for message passing (for populating the ThreadInfo of the new task). In that case, there would be no possibility to choose between the two forms of starting a task. What about adding a way to perform the setup* manually from the outside, so that runTask could also be made to work? The question would just be how to ideally avoid creating another public API method that is only really useful for Scheduler implementations and not for the normal API user...

*:

auto spawnTid = Tid( new MessageBox ); // just this line would be enough in theory
auto ownerTid = thisTid;
thisInfo.ident = spawnTid;
thisInfo.owner = ownerTid;

BTW, I'll try to reproduce the Win64 issue tomorrow.

s-ludwig added a commit to vibe-d/vibe.d that referenced this pull request Jun 9, 2014
@s-ludwig
Copy link
Member

s-ludwig commented Jun 9, 2014

For Win64, I'm getting a different error code (-1073741701, which corresponds to 0xc000007b aka STATUS_INVALID_IMAGE_FORMAT), maybe that's related to the curl.lib that I had to get from somewhere. But -1073741819 should correspond to a STATUS_ACCESS_VIOLATION.

@s-ludwig
Copy link
Member

s-ludwig commented Jun 9, 2014

Got it to run finally. The crash is in Fiber.call of the FiberScheduler, but there is no obvious relation to the scheduler code. Looks like #809 probably was only a partial fix.

@mihails-strasuns
Copy link

@complexmath I have rebased your PR against current master and squashed everything in two commits : https://github.com/Dicebot/phobos/tree/complexmath-concurrency-pr

You can reset your PR to that branch by doing this:

git remote add dicebot https://github.com/Dicebot/phobos.git
git fetch dicebot
git checkout addConcurrencyScheduler
git reset --hard dicebot/complexmath-concurrency-pr
git push -f origin addConcurrencyScheduler

@complexmath
Copy link
Member Author

Done. Thanks for the help!

@DmitryOlshansky
Copy link
Member

Hm, it can't merge again. Needs yet another git pull --rebase upstream master ...

@complexmath
Copy link
Member Author

Oh FFS. So I did this:

git fetch upstream master
git rebase upstream master
git checkout addConcurrencyScheduler
git merge master
... resolved conflict in std/concurrency.d
git push origin addConcurrencyScheduler

And this created a million changes. Instead of the "git merge master" step I guess I should have done something else?

@DmitryOlshansky
Copy link
Member

@complexmath Yeah.. instead just this would have worked:

git checkout addConcurrencyScheduler
git pull --rebase upstream master

No need to re-fetch or anything.
Now to reset back to your old state to try the above, do:

git checkout addConcurrencyScheduler
git reset --hard 1b73a4e

That should put your branch at the right commit as it was.

complexmath and others added 2 commits October 21, 2014 11:47
https://d.puremagic.com/issues/show_bug.cgi?id=12090

With this commit fibers can have own message boxes and act similar
to threads in that regard.
Generator is an extension to a Fiber that yields return values of specific type
@complexmath
Copy link
Member Author

Thanks. For some reason I thought "git pull" would always result in the huge merge we were trying to avoid.

@DmitryOlshansky
Copy link
Member

@complexmath You are welcome. Going to auto-merge it unless something spooky happens.

@dnadlinger
Copy link
Member

So Generator is accepted now? (I personally don't care too much either way.)

@mihails-strasuns
Copy link

I don't see any truly compelling reason to not merge it. If there are any issues with it will be easier to create separate PR's for those before release and figure those out on case by case basis. This has taken wa too long already, some sense of accomplishment is necessary :)

@mihails-strasuns
Copy link

Auto-merge toggled on

mihails-strasuns pushed a commit that referenced this pull request Oct 22, 2014
Issue 12090 - Make std.concurrency compatible with fibers as threads
@mihails-strasuns mihails-strasuns merged commit b956f68 into dlang:master Oct 22, 2014
@mihails-strasuns
Copy link

Couldn't resists to press the button myself :P

@mihails-strasuns
Copy link

For some reason I thought "git pull" would always result in the huge merge we were trying to avoid

It is other way around :) git merge will always result in huge merge commit. git pull may or may not result in merge commit depending on branch state and flags used (with --rebase it never will)

@complexmath
Copy link
Member Author

Yay!

@DmitryOlshansky
Copy link
Member

Awesome. Oh, the strange pleasure of having 6+ month old PR merged :)

@IgorStepanov
Copy link
Contributor

@DmitryOlshansky May be my new AA implementation will be merged somewhen=)
Remaining about 4 months? :o)

@DmitryOlshansky
Copy link
Member

@IgorStepanov other common durations are 8 month, and a year ;)
Sadly I still lack knowledge of druntime to merge big pulls there.

@IgorStepanov
Copy link
Contributor

@DmitryOlshansky

other common durations are 8 month, and a year ;)

Ok, I have to be patient:)

Sadly I still lack knowledge of druntime to merge big pulls there.
BTW, I you have any time and energy, you may see the only dlang/druntime#934.
This PR implements AA backend, it haven't any dependence and it may be seen simply as the implementation of the abstract container.

@MartinNowak
Copy link
Member

Awesome. Oh, the strange pleasure of having 6+ month old PR merged :)

Write smaller pull requests and discuss design decisions upfront.
I think it was a failure to merge this and disregarding outstanding design concerns.
#1910 (comment)
#1910 (comment)
#1910 (comment)
#1910 (comment)
It remains open how FiberScheduler can be integrated with event loops and it's somewhat unclear what the exact design goals are.

Please never start a complex feature by implementing a pull request.
The whole discussion is littered with Fiber bug reports, git noise and style comments, but very little is concerned with the API and how this integrates with the rest of D's infrastructure.

So for the next time, please start with a DIP or a newsgroup discussion telling people what you want to achieve and how you intend to implement it.

auto t = new Thread( &exec ); t.start();
links[spawnTid] = linked;
if( scheduler !is null )
scheduler.spawn( &exec );
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you simply initialize scheduler with a default 1:1 threading scheduler?
It's now possible to create and store classes in globals at CT.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this would add unnecessary overhead for the default case.

Copy link
Member

Choose a reason for hiding this comment

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

Just a vtable right? More a matter of taste.

@mihails-strasuns
Copy link

@MartinNowak I disagree that this specific feature is complex enough to keep bashing it again and again. Most of the linked issues are not really related to adding fiber support to std.concurrency but to possible system built on top of this. This specific addition was small and was working as intended, I see no point in keeping it stalling for 1 more year while discussing more abstract things.

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