Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Refactor thread #118

Closed
wants to merge 2 commits into from
Closed

Conversation

MartinNowak
Copy link
Member

  • pimpl core.thread
  • pimpl core.sys.windows.threadaux

Not much refactoring yet but this is a requirement to
integrate threads with other internal druntime components.

@complexmath
Copy link
Member

The thread code belongs in core, not rt/. The rt/ directory is for compiler support code. If we want to hide the implementation, why not make core/internal to match Phobos' std/internal? Also, why make ThreadImpl a struct instead of a class? I can see having a struct Thread wrapper that's passed around by value, but since ThreadImpl would always be dynamically allocated, why a struct? To avoid GC allocation?

@MartinNowak
Copy link
Member Author

core/internal sounds fine

Your probably right about making ThreadImpl a class, won't hurt to have two more.

@MartinNowak
Copy link
Member Author

Any further objections?
It's rather difficult to maintain that pull as I have to
balance the rest of the shared library support on top of it.

@MartinNowak
Copy link
Member Author

Seems like I messed up the merge. Will clean this up tomorrow.

 - It is one of the most interdependent modules in the whole runtime
   but at the same time it is implemented completely in the runtime
   interface.

 - Deriving Thread and Fiber is part of the specified API.  By
   reducing the member to a single pointer we'll remain ABI compatible
   with implementation changes.

 - This adds another level of indirection for every Thread and Fiber
   method which is neglectable given the complexity of their methods.

 - Use a header file and an implementation file to avoid a pseudo
   namespace using dozens of extern(C) functions

 - Compilation of the implementation needs to be separated from header
   usage because the compiler currently doesn't support this.
 - fix module clash of core.thread header
   and implementation

 - removed struct scope (thread_aux)
@MartinNowak
Copy link
Member Author

Bump.

This is needed to merge the rest of the shared library support for ELF.
I need to wire up the thread module with rt.lifetime and the to be
added shared library registry.
Even so this should be a useful change on it's own as it stabilizes the ABI
and promotes future change.

If there is any disagreement with that please get back to me so that we can sort it out.

@complexmath
Copy link
Member

Rather than pimpl, what if core/thread.di were hand-written to hide all implementation details? Is there really a need for the double indirection here?

@MartinNowak
Copy link
Member Author

The double indirection buys you a stable ABI, which will be an increasing concern once
druntime/phobos are shared libraries. But this isn't required now.

I was initially trying to further break down the whole module but that failed.
So I think your proposal of a di file is adequate for now.

JohanEngelen added a commit to weka/druntime that referenced this pull request Feb 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants