Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Remove coordinator and support forking #1067
Remove coordinator and support forking #1067
Changes from all commits
3b61fad
18f7bb6
ffe8f33
b64e2a7
4fa8fac
8f8c3e0
845eea6
2d52f00
fa2aa50
867eac0
2230c3f
5bda9ee
70d7b95
72e6eb9
742c3a2
90cb6df
2a25331
1f9d905
029bfda
d328859
0d79d8d
44f22ba
e243776
54f586b
da19f04
822f6f5
7cb0b2c
176ad47
53309d9
d3ddf7d
5c125fb
41ee28c
c4ab8b4
d9b451a
7a2074e
eb1d8df
54d798a
f0e2755
6e907e0
ea53ce1
2c15ecd
9aacb2d
351d982
9f743a8
fd2f9c3
991314a
eaa00bf
b791aa7
f89db28
f049f73
a6d834a
e9da4c2
d293c04
b526ec1
d9ef6a3
a0b2f0b
31a660c
49c979e
a56e38d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fork
is unix specific. We should either avoid using the term, or make it specific to certain targets. As we are not doing anything specific to forking in MMTk (we just stop GC threads), I think it may be a good idea to avoid using the termfork
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
fork
is unix-specific. But I think we should be very explicit about forking because it has very unusual requirements. For example, when callingfork()
, the process must have only one thread. If the process has multiple threads, the only thing safe to do is callingexec()
or other async-signal-safe functions (see https://man7.org/linux/man-pages/man2/fork.2.html). The implementation ofprepare_to_fork
is centered around those requirements. TheGCThreadJoinHandle
trait which I just introduced yesterday is one example. It waits until the underlying native thread of a GC thread quits. It may be very different if we simply want to, for example, grow or shrink the number of GC threads at run time. In that case, it is sufficient to let the GC thread give up their context, but the VM may repurpose those threads as other threads (such as used as mutators).And the name should be exposed through the API. If a VM needs forking (Android ART and CRuby), it knows precisely what it is doing. CRuby carefully brings down other helper threads, too, before forking.
I am OK with making it platform-specific. VMs treat forking as platform-specific, too. CRuby only supports
Kernel#fork
if the underlying OS supportsfork()
, and Andorid only runs on its own modified Linux kernel. But I don't know how to do it well now. We need some kind of platform abstraction layer. Then we can label platforms that supportfork()
. That includes Linux, Mac, some UNIX variants, and CYGWIN, too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this API function, does MMTk core do anything specific to forking other than just stopping GC threads? Is it the binding rather than MMTk core that needs to know the semantics of forking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCWorker
struct instances so that GC workers can be re-spawn later, reusing those structs. If we are adjusting the number of GC workers, we probably would drop those structs. This probably doesn't matter because the GC workers are almost stateless when not running any work packets.memory_manager::start_worker
. Of course the binding needs to cooperate by implementing theGCThreadJoinHandle
.There is an alternative design. MMTk core provides a function to ask all GC threads to return from their entry-point functions, such as
memory_manager::start_worker
. Then the VM bindings implement their own mechanisms to wait until their underlying OS threads exited. By doing this, bindings no longer need to implementGCThreadJoinHandle
and give it to mmtk-core.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds to me that both are design/implementation choices rather than some requirements of
fork
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is OK to destruct
GCWorker
when forking. We will need to construct them again later. It is fair to say this is a design choice. It is an appropriate choice for forking.It is OK to move the responsibility of waiting for the OS threads to the binding. But either mmtk-core or the binding must wait for OS threads to exit. This is a hard requirement for
fork
.