Decouple Stack Context by baziotis · Pull Request #2797 · dlang/druntime

baziotis · 2019-09-14T15:14:08Z

Continuing from #2689

This PR decouples Context (now StackContext) from Thread / Fiber in the form of a super class for both.
Also, it adds a GlobalStackContext for commonly used global data.

@thewilsonator I did it kind of quickly, tell me what you think. I did not follow your diff precisely as some things seemed not necessary. One example is push/popContext etc. which seem to be relevant only to Thread.
Also, you seem to want a LockGuardedBlock for which I found no use and so it's not added yet.
Finally, it may be good to alias GlobalStackContext as it is used very much and it is a little verbose.

dlang-bot · 2019-09-14T15:14:09Z

Thanks for your pull request and interest in making D better, @baziotis! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2797"

thewilsonator · 2019-09-15T00:32:26Z

One example is push/popContext etc. which seem to be relevant only to Thread.

Yes, but the operations only require the data members of StackContextExecutor. Also osthread.d is (still) a massive file anything that can logically be removed from it is a good thing.

Also, you seem to want a LockGuardedBlock for which I found no use and so it's not added yet.

I think I was trying to factorise the horrible mess of globals somewhat. See the low level thread stuff at the bottom of osthread.d, although tbh it might be better to split that off completely in a separate PR and refactoring it there (making a struct LowLevelThread { ThreadID tid; } with the free functions as methods and the locks as static members. Be sure to ping Rainer for that one.

Finally, it may be good to alias GlobalStackContext as it is used very much and it is a little verbose.

I don't think so, as its kind of the point to make it obvious that the variable being accessed is global (and therefore synchronisation may be required, and should be suspicious if absent).

baziotis · 2019-09-15T00:54:05Z

One example is push/popContext etc. which seem to be relevant only to Thread.

Yes, but the operations only require the data members of StackContextExecutor.

Yes, but if we put them in StackContextExecutor, then it seems like Fiber needs them, while
it doesn't seem to. If that's ok, I'll put them there.

Also osthread.d is (still) a massive file anything that can logically be removed from it is a good thing.

Yes, I'll see if I have something when the tests pass. Which btw, fail currently. If you happen to have any suggestion, in this unittest calling the derived.call or composed.call (and even having everything else commented out), yields a seg fault. GDB shows that it tries to call m_dg or m_fn and they're null - here.
Which is weird if you consider that to reach this point (because of the switch) , m_call has
to be set to Call.DG, which is set in reset(), which should set m_dg as well.

So.. the hard part on these refactorings is that something can break with 0 chances of finding why. :p

Also, you seem to want a LockGuardedBlock for which I found no use and so it's not added yet.

I think I was trying to factorise the horrible mess of globals somewhat. See the low level thread stuff at the bottom of osthread.d, although tbh it might be better to split that off completely in a separate PR and refactoring it there (making a struct LowLevelThread { ThreadID tid; } with the free functions as methods and the locks as static members. Be sure to ping Rainer for that one.

Ok great, interesting.

Finally, it may be good to alias GlobalStackContext as it is used very much and it is a little verbose.

I don't think so, as its kind of the point to make it obvious that the variable being accessed is global (and therefore synchronisation may be required, and should be suspicious if absent).

Ok, clear.

-- Edit --
Fixed it (at least what I was referring to).

Due to changing the number of fields in Fiber (and moving them in a base class), the indexing on .tupleof had to change.

thewilsonator · 2019-09-15T02:49:44Z

Yes, but if we put them in StackContextExecutor, then it seems like Fiber needs them, while
it doesn't seem to.

Even if Fiber doesn't need it, having the stuff related solely to StackContext management all in one place is good practice.

If that's ok, I'll put them there.

Please do.

src/core/thread/context.d

thewilsonator · 2019-09-15T03:15:27Z

src/core/thread/context.d

+
+    // Thread / Fiber entry point.  Invokes the function or delegate passed on
+    // construction (if any).
+    final void run()


note that somehow this has triggered

../.dub/packages/gtk-d-3.3.1/gtk-d/src/glib/Spawn.d(204,15): Error: function `glib.Spawn.Spawn.ReadFile.run` cannot override `final` function `core.thread.context.StackContextExecutor.run`

ae:sys-net-test 0.0.2444: building configuration "ae-sys-net-test-test-library"... -- | sys/file.d(2020,8): Error: function `ae.sys.file.writeFileAsync.Writer.run` cannot override `final` function `core.thread.context.StackContextExecutor.run`

in build kite, why I'm not sure.

I can't find the source of either of these files but the error most probably is something like here. Note that the private was not there before. I put it to fix an identical error (following the example of other unittests).
So, the point is that this is user code and it's not a problem of Fiber's code.
But this may lead to breaking change in existing code.

Yeah its probably a visibility/protection issue, you can ask on the forums for an explanation if you can't find a solution. (Alternately you could call it something else that won't cause a collision)

Well, the problem is that a final function cannot be overriden, unless it is also private.
Previously: run was member of Fiber but it was private so overriding in e.g. DerivedFiber was ok.
Now: run is a member of StackContextExecutor and it can't be private as it has to be used by Fiber. Because of that, overriding it causes a problem.

I think of a couple solutions to that, of which the simplest is the one you said - chaning the name.

Hmm, perhaps doing #2797 (comment) and having it as the opCall in this PR would be worth doing then. Saves having to come up with a name that won't collide. Or just move run back to Fiber for the time being and do that when you address #2797 (comment) in a future PR. Up to you.

Ok, I'll see if there are any simple things left and do that.

src/core/thread/context.d

thewilsonator · 2019-09-16T00:47:57Z

src/core/thread/fiber.d

 */
-class Fiber
+
+import core.thread.context : StackContext, StackContextExecutor;


You have multiple import of the same module just collapse them into one, and the public import for Rethrow just do alias Rethrow = .Rethow; (or alias Rethrow = core.thread.context.Rethrow; if that doesn't work)

Yes, the imports are a little bad at the moment, along with the versioning. The last one is good thought, thanks.

baziotis · 2019-10-07T19:17:03Z

This was supposed to be a follow-up to #2821,
continuing the core.thread refactoring by decoupling the stack context.
I don't have time at the moment to continue it, but I'm leaving this for reference.

Decouple Stack Context

ceb9bcf

baziotis requested review from andralex and wilzbach as code owners September 14, 2019 15:14

baziotis added 3 commits September 14, 2019 18:16

Update context.d doc

06e160d

Update /mak files

47e671b

Small fixes

757bc24

baziotis added 2 commits September 15, 2019 04:48

Fixed duplicate standard fiber data

3bed5d9

Fixed fiber_guard_page.d

8fbcf31

Due to changing the number of fields in Fiber (and moving them in a base class), the indexing on .tupleof had to change.

thewilsonator reviewed Sep 15, 2019

View reviewed changes

baziotis added 2 commits September 15, 2019 14:54

Decoupling 3 member variables

9b921ae

Decouple *Context functions

93dde48

thewilsonator reviewed Sep 15, 2019

View reviewed changes

src/core/thread/context.d Outdated Show resolved Hide resolved

thewilsonator reviewed Sep 15, 2019

View reviewed changes

src/core/thread/context.d Outdated Show resolved Hide resolved

baziotis added 2 commits September 15, 2019 16:05

Other simple fixes

de73069

Decouple getStackTop/Bottom

dc8b6a3

baziotis commented Sep 15, 2019

View reviewed changes

src/core/thread/context.d Show resolved Hide resolved

baziotis added 3 commits September 15, 2019 17:14

Decouple Rethrow and small refactorings

0b69e98

Temporarily duplicating version stuff

24d9540

Generic posix import removed

1b9a532

thewilsonator reviewed Sep 16, 2019

View reviewed changes

baziotis closed this Oct 7, 2019

Uh oh!

Conversation

baziotis commented Sep 14, 2019

Uh oh!

dlang-bot commented Sep 14, 2019

Bugzilla references

Testing this PR locally

Uh oh!

thewilsonator commented Sep 15, 2019

Uh oh!

baziotis commented Sep 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thewilsonator commented Sep 15, 2019

Uh oh!

Uh oh!

Uh oh!

thewilsonator Sep 15, 2019 • 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!

baziotis Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

thewilsonator Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

baziotis Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

thewilsonator Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

baziotis Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thewilsonator Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

baziotis Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

baziotis commented Oct 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

baziotis commented Sep 15, 2019 •

edited

Loading

thewilsonator Sep 15, 2019 •

edited

Loading