issue-#156 adding Coroutines to BlackBox

Merged to the master branch

Re: issue-#156 adding Coroutines to BlackBox

Postby Josef Templ » Mon Apr 10, 2017 8:17 am

handling of very small stack sizes has been improved.
see diffs at https://redmine.blackboxframework.org/projects/blackbox/repository/diff?utf8=%E2%9C%93&rev=d2e29f078214383d4d1d0cf910c832635894ffe6&rev_to=f56119eb4d1cf320603dd78013d8bed82221a0b9.

Ivan, the normal procedure would be to create an alternative branch if you want to show
an alternative solution. Can we agree at least on that?

- Josef
User avatar
Josef Templ
 
Posts: 2012
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#156 adding Coroutines to BlackBox

Postby Ivan Denisov » Mon Apr 10, 2017 11:18 am

I made separate branch and added last fix there.
http://blackboxframework.org/unstable/i ... a1.843.zip

I had worked with your branch because I hope on cooperative effect. Why there should be a competition? I think, that such competition will be unnecessary, because my code is very similar to your's. It is just organized in the BlackBox way and it's more easy to port and support.

Coroutines — is the logics of coroutines.
Cross-platform Kernel modification for multiple stacks support.
HostCoroutines — it's realization with Windows Fibers and the new Kernel features.
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#156 adding Coroutines to BlackBox

Postby Josef Templ » Mon Apr 10, 2017 2:01 pm

small improvement in Kernel.AddCoroutine:
stackBase reset to 0.
This enables one to reuse the object for several coroutines.

See diffs at https://redmine.blackboxframework.org/projects/blackbox/repository/diff?utf8=%E2%9C%93&rev=97d6cf15fe06438a50bb095fa275bfaf1d3bf2c0&rev_to=d2e29f078214383d4d1d0cf910c832635894ffe6.

The WinApi calls have been moved to Kernel originally in order to do Ivan a favor.
In the meantime it turned out to be a good move. Why?
Because it is easy this way to guarantee consistency.
If half of the coroutine support is done in the Kernel and the other half is done outside,
it is easily possible to tell the Kernel to switch to coroutine A and via the other half
to switch to coroutine B, creating occasional system crashes in the garbage collector, for example.
By keeping them together this consistency is guaranteed by design.
It also gives the coroutine support routines more flexibility in their implementation
because all aspects are handled at one place.
There is also no runtime overhead by that organization.

The exchangeability need is hard to follow for me.
If there is a much better implementation available it should be used as the default implementation.
Does anybody need multiple implementations at the same time?
I doubt it (and it may create problems if they are mixed at runtime).

The Co_ package has been transformed to use the Kernel's coroutine services now.
There were no surprises, as I expected. A straightforward 1:1 replacement
of WinApi fiber calls by the corresponding Kernel coroutine procedures.
The only surprise was that the Co_ author previously thought that it is not possible.

- Josef
User avatar
Josef Templ
 
Posts: 2012
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#156 adding Coroutines to BlackBox

Postby Josef Templ » Tue Apr 11, 2017 3:11 pm

A bug in the Kernel's coroutine garbage collection support has been fixed.
The latest build is http://blackboxframework.org/unstable/issue-%23156/blackbox-1.7.1-a1.847.zip.

- Josef
User avatar
Josef Templ
 
Posts: 2012
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#156 adding Coroutines to BlackBox

Postby Ivan Denisov » Tue Apr 11, 2017 5:05 pm

Adopted last Josef's fixes for the branch issue-#156host
http://blackboxframework.org/unstable/i ... a1.848.zip

Josef Templ wrote:The WinApi calls have been moved to Kernel originally in order to do Ivan a favor.
In the meantime it turned out to be a good move. Why?
Because it is easy this way to guarantee consistency.
If half of the coroutine support is done in the Kernel and the other half is done outside,
it is easily possible to tell the Kernel to switch to coroutine A and via the other half
to switch to coroutine B, creating occasional system crashes in the garbage collector, for example.
By keeping them together this consistency is guaranteed by design.
It also gives the coroutine support routines more flexibility in their implementation
because all aspects are handled at one place.

I mentioned the attention on some requirements to any basic feature. It should be split in BlackBox way to cross-platform logics including abstract interface and the realization. You do it some "lazy" way and move the realization to Kernel (mixed stacks manipulations feature with Windows fibers manipulations). It was better then the initial suggestion. However this starts wrong tradition to make Kernel more platform-dependent and "fat". It will be more hard to support it in multiple platforms. Your argument about consistency is good. I understand some benefits. But if all BlackBox will be in one module it will be very consistent AND it will be impossible to deal with it and support it in future. We should always fight with complexity and make decomposition of the complex tasks. Coroutines task is complicated. It includes stack manipulations, fibers manipulations and the logic with some traditional design patterns (Iterator, Task etc.). From my point of view we should split this task:
Coroutines — is the logics of coroutines.
Cross-platform Kernel modification for multiple stacks support.
HostCoroutines — it's realization with Windows Fibers and the new Kernel features.

Josef Templ wrote:There is also no runtime overhead by that organization.

I should mention that there is no runtime overhead by this organization. I had mentioned even 10% faster execution of ObxCoroutines.RunPrimes. This should be checked carefully.
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#156 adding Coroutines to BlackBox

Postby Zinn » Tue Apr 11, 2017 5:18 pm

Ivan Denisov wrote:I mentioned the attention on some requirements to any basic feature. It should be split in BlackBox way to cross-platform logics including abstract interface and the realization...


Ivan, there are more than one design pattern inside BlackBox.
The philosophy of Oberon is "Everything should be as simple as possible, but not simpler" (Einstein).

I am not an expert in coroutines but Josef's 'consistency argument' is convincing to me.
HostCoroutines only makes it more complicated and dangerous to use.

- Helmut
Zinn
 
Posts: 470
Joined: Tue Mar 25, 2014 5:56 pm
Location: Frankfurt am Main

Re: issue-#156 adding Coroutines to BlackBox

Postby Ivan Denisov » Tue Apr 11, 2017 5:39 pm

Zinn wrote:
Ivan Denisov wrote:I mentioned the attention on some requirements to any basic feature. It should be split in BlackBox way to cross-platform logics including abstract interface and the realization...


Ivan, there are more than one design pattern inside BlackBox.
The philosophy of Oberon is "Everything should be as simple as possible, but not simpler" (Einstein).

I am not an expert in coroutines but Josef's 'consistency argument' is convincing to me.
HostCoroutines only makes it more complicated and dangerous to use.

I understands that everyone can not dive into details. Both versions are worthwhile and it is good that Center members has some choice.

For me this discussion is critical, because after 1.7.1 will be released I have to sync and release version for Linux, OpenBSD and FreeBSD. It will be much easier to concentrate only on the system part. The kernel patch will be trivial, because it can be cross-platform.

To make it easier to distinguish these versions of sources compositions I will past few sources parts here:

Josef's composition:
Kernel
Code: Select all
   PROCEDURE RemoveCoroutine*(this: Coroutine);
      VAR t: TrapCleaner; chk: Handler;
   BEGIN
      IF this.next # NIL THEN
         this.prev.next := this.next; this.next.prev := this.prev;
         this.next := NIL; this.prev := NIL
      END;
      IF (this # currentCoroutine) & (this.fiber # 0) THEN
         WinApi.DeleteFiber(this.fiber); this.fiber := 0;
         WHILE this.trapStack # NIL DO
            t := this.trapStack; this.trapStack := t.next; t.Cleanup
         END;
         chk := this.trapChecker; this.trapChecker := NIL;
         IF (chk # NIL) & (err # 128) THEN chk() END
      END;
   END RemoveCoroutine;


Ivan's composition:
HostCoroutines
Code: Select all
   PROCEDURE (h: Hook) RemoveCoroutine*(this: Coroutines.Impl);
   BEGIN
      WITH this: FiberCoroutine DO
         IF (this # currentCoroutine) & (this.fiber # 0) THEN
            WinApi.DeleteFiber(this.fiber); this.fiber := 0;
            Kernel.RemoveStackInfo(this.stackInfo)
         END
      END
   END RemoveCoroutine;
Kernel
Code: Select all
   PROCEDURE RemoveStackInfo*(s: StackInfo);
   VAR t: TrapCleaner; chk: Handler;
   BEGIN
      IF s.next # NIL THEN
         s.prev.next := s.next; s.next.prev := s.prev; s.next := NIL; s.prev := NIL
      END;
      WHILE s.trapStack # NIL DO
         t := s.trapStack; s.trapStack := t.next; t.Cleanup
      END;
      chk := s.trapChecker; s.trapChecker := NIL;
      IF (chk # NIL) & (err # 128) THEN chk() END
   END RemoveStackInfo;


Josef's composition:
Kernel
Code: Select all
   PROCEDURE TransferCoroutine*(target: Coroutine);
   BEGIN
      ASSERT(target.next # NIL, 20);
      IF target # currentCoroutine THEN
         currentCoroutine.trapStack := trapStack; currentCoroutine.trapChecker := trapChecker;
         S.GETREG(FP, currentCoroutine.stackTop);
         trapStack := target.trapStack; trapChecker := target.trapChecker;
         currentCoroutine := target;
         WinApi.SwitchToFiber(target.fiber)
      END
   END TransferCoroutine;


Ivan's composition:
HostCoroutines
Code: Select all
   PROCEDURE (h: Hook) TransferCoroutine*(target: Coroutines.Impl);
   BEGIN
      WITH target: FiberCoroutine DO
         IF currentCoroutine # target THEN
            Kernel.SwitchStackInfo(currentCoroutine.stackInfo, target.stackInfo);
            currentCoroutine := target;
            WinApi.SwitchToFiber(target.fiber)
         END
      ELSE END
   END TransferCoroutine;
Kernel
Code: Select all
   PROCEDURE SwitchStackInfo*(from, to: StackInfo);
   BEGIN
      ASSERT(to.next # NIL, 20);
      ASSERT(to # currentStack, 21);
      from.trapStack := trapStack; from.trapChecker := trapChecker;
      S.GETREG(FP, from.stackTop);
      trapStack := to.trapStack; trapChecker := to.trapChecker;
      currentStack := to
   END SwitchStackInfo;


The main advantage of my composition that we have no extra WinApi calls in the System modules. Second benefit is that the solution is decomposed so it easier to understand the sources.
The third advantage, that you can improve this HostCoroutines for your aims. For example add some logging of fibers statistics.
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#156 adding Coroutines to BlackBox

Postby Josef Templ » Wed Apr 12, 2017 9:12 am

Ivan Denisov wrote:For me this discussion is critical, because after 1.7.1 will be released I have to sync and release version for Linux, OpenBSD and FreeBSD. It will be much easier to concentrate only on the system part. The kernel patch will be trivial, because it can be cross-platform.


Firstly, porting coroutines is not something that is done every day.
Secondly, for the port to Linux etc. it doesn't make a difference if you edit it here or there.
The real work (99%) is finding the best implementation for Linux etc. and this is the same either way.
The WinApi calls are clearly visible in the source and the compiler marks
every occurrence. It is not possible to overlook it.
This is the same as with other WinApi calls in the Kernel and this is IMHO not a design mistake
that needs to be fixed but a necessary compromise.

Calling anything indirectly with some hook plus additional code can never be faster
than calling directly. If your measurements show something else this must be caused
by something else, e.g. garbage collector invocation or cache effects or other
Windows background activities.

Your RemoveCoroutine version has a different behaviour than mine.

- Josef
User avatar
Josef Templ
 
Posts: 2012
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#156 adding Coroutines to BlackBox

Postby DGDanforth » Wed Apr 12, 2017 9:23 am

Your RemoveCoroutine version has a different behaviour than mine.


Picky English language comment: A large proportion of American talkers do
say "different than" but that is not correct. One should say
"different from". Its easy to remember since "different" has "f" in it.

-Doug
User avatar
DGDanforth
 
Posts: 1061
Joined: Tue Sep 17, 2013 1:16 am
Location: Palo Alto, California, USA

Re: issue-#156 adding Coroutines to BlackBox

Postby Ivan Denisov » Wed Apr 12, 2017 9:33 am

Josef Templ wrote:Your RemoveCoroutine version has a different behaviour...

I just realize that your version is inconsistent... it can remove StackInfo from the list and then does not remove Fiber. Why did you do it this way?
Ivan Denisov
 
Posts: 1694
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

PreviousNext

Return to Resolved (Features)

Who is online

Users browsing this forum: No registered users and 1 guest

cron