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

Added functions to get and set the stackbottom of each thread #277

Closed
wants to merge 1 commit into from

Conversation

@bcardiff
Copy link
Contributor

commented Apr 22, 2019

The added functions allow the implementation of lightweight threads without imposing new concepts to the user of the GC.

Hopefully, this PR is the beginning of a conversation on how to add this feature. I suspect there are things you will want to discuss before merging.

In Crystal, we are starting to use this extension as a preview feature in the path to support multi-threading.

@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2019

Relates to #274

@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2019

Notes on pull requests:

  • It should be based on master
  • After testing it could be later back-ported to the release branches (but typically new functionality is not back-ported)
  • The committed code should not refer to non-existing entities (e.g. GC_switch_to_coroutine).
@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2019

I was considering such solution but the issue with it that we completely forget to scan old stack. E.g. you have the only object pointer in the primordial thread's stack then you switch to a coroutine, then GC occurs which won't scan the the primordial thread's stack thus causing the live object to be collected.
How do you deal with such situations?

@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2019

Currently I'm working on another solution described in #274. It has some drawbacks too.

@ivmai ivmai closed this Apr 22, 2019
@bcardiff

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I'm interested in whatever built-in support bdw-gc can have that can be used in Crystal.

In Crystal:

  • we already need to keep a list of all the lightweight threads in order to switch/restore their execution context.
  • The initial lightweight threads of each thread are also in that list.
  • Each element knows if it is currently bound to a thread or not.
  • Before a collect, thanks to GC_set_push_other_roots, all the stacks of the lightweight threads that are not currently running are pushed as roots.

Although having an abstraction can be useful, at the end of the day a scheduler would need to know about the lightweight threads, so there is no real gain to have some else that the proposed feature.

A final note is that the proposed extension is only called before the collect, there is no need to let the GC know about all the context switches the runtime is doing.

@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2019

OK. The concept looks good.
@nickwanninger, what do you think of it?

@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

Hopefully, this PR is the beginning of a conversation on how to add this feature. I suspect there are things you will want to discuss before merging.

What to improve (some items already mentioned above):

  • rebase to master
  • don't mention functions that are missing in GC (e.g. GC_switch_to_coroutine)
  • don't duplicate comments in .h and .c (API function should be documented in .h, adding the same commit to the function definition is redundant)
  • Use GC_stack_base to retrieve and update thread's stack bottom (i.e. GC_get_stackbottom should have void return type)
  • The API should be cross-platform (single-threaded, pthreads, win32) - for the single-threaded case we can add a comment that 0 could be passed as a thread-id, the type for thread id is GC_SUSPEND_THREAD_ID (this is defined in javaxfc.h, so we could move GC_set_stackbottom there, the type should be DWORD if win32); to simplify cross-platform usage, I think it would be good to allow 0 as thread id to specify the current thread
  • GC_set_stackbottom (i.e. user-level context switching based on GC_set_stackbottom) is not compatible GC_do_blocking and GC_call_with_gc_active currently - it is OK but there should be the relevant comment in .h and there should be assertions that GC_blocked_sp and GC_traced_stack_sect (or their multi-threaded equivalents) are null on GC_set_stackbottom entrance
  • "MUST be called with the GC disabled": do you mean the caller should acquire the GC lock (by GC_call_with_alloc_lock)? We should add GC_ASSERT(I_HOLD_LOCK()) to GC_set_stackbottom in this case
  • There should be some testing of get/set functionality in gctest (the easiest (minimum) thing is to get stack bottom and immediately set it to the same thread)

A final note is that the proposed extension is only called before the collect, there is no need to let the GC know about all the context switches the runtime is doing.

Also, is my understanding correct that you listen GC events and perform GC_set_stackbottom for all threads? Is it correct that we could also call GC_set_stackbottom on each context switch (w/o listening GC events)?

@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

I'm closing this PR (new version to be in another PR). But discussion is OK to continue here or in #274

@ivmai ivmai closed this Apr 23, 2019
@nickwanninger

This comment has been minimized.

Copy link

commented Apr 24, 2019

Does this PR still mark the old stack? When you switch to another stack base, is the old stack still being scanned for references?

@ivmai

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

No, you should do bookkeeping of the coroutines yourself and handle marking of inactive coroutines (including the initial one) using GC_set_push_other_roots. To fetch the bottom of the current stack you can use one of the proposed functions.

ivmai added a commit that referenced this pull request May 6, 2019
Issue #277 (bdwgc).

This API is useful to support coroutines.

* include/gc.h (GC_get_my_stackbottom, GC_set_stackbottom): New API
function declaration.
* misc.c [!THREADS] (GC_set_stackbottom, GC_get_my_stackbottom): New
function definition.
* pthread_support.c [GC_PTHREADS && !GC_WIN32_THREADS]
(GC_set_stackbottom, GC_get_my_stackbottom): Likewise.
* win32_threads.c [GC_WIN32_THREADS] (GC_set_stackbottom,
GC_get_my_stackbottom): Likewise.
* tests/test.c (struct thr_hndl_sb_s): Define.
* tests/test.c (set_stackbottom): New function (which calls
GC_set_stackbottom).
* tests/test.c (run_one_test): Define thr_hndl_sb local variable;
call GC_get_my_stackbottom() and set_stackbottom().
* win32_threads.c [GC_WIN32_THREADS && I386] (struct GC_Thread_Rep):
Add initial_stack_base field.
* win32_threads.c [GC_WIN32_THREADS && I386] (GC_record_stack_base,
GC_call_with_gc_active): Set initial_stack_base field.
* win32_threads.c [GC_WIN32_THREADS && I386] (GC_push_stack_for): Handle
the case when GetThreadContext() might return stale register values,
thread stack_base != initial_stack_base but the stack is not inside
the TIB stack (use context.Esp but call WARN); add TODO.
@jhass jhass referenced this pull request Aug 30, 2019
@bcardiff bcardiff referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.