Thanks for the additional info Coleen!
Just to add a bit more to the initialization history. The ServiceThread
is a generalization of the LowMemoryDetectorThread that was part of the
management API, and so it was initialized in Management::initialize.
When it turned into the ServiceThread - to process JVMTI deferred events
in addition to the low-memory-detector events - the initialization
placement remained the same. Then later the INCLUDE_MANAGEMENT guards
were added (JDK-7189254, October 2012). Later still we started adding
other items of work for the ServiceThread. The earliest was the
AllocationContextService notification in September 2014 but as that no
longer exists I can't tell if that was the first non-management related
use. Then the StringTable use was added 18 months ago - which definitely
was outside the realm of the management API. So that is when the
MinimalVM was first "broken". So it is good that is fixed.
With regard to the placement in the initialization order, my remaining
concern was with JVMTI event processing that might happen via events
generated very early in the init sequence. But you have now modified
things so that we will only process events in the LIVE phase, which only
activates after all the class library initialization is complete.
So overall I'm no longer significantly concerned about the change to the
initialization order as I think you have it all covered. Thanks for
bearing with me and all the off-list discussion.
Cheers,
David
-----
On 18/12/2019 1:27 am, coleen.phillim...@oracle.com wrote:
On 12/16/19 11:04 PM, David Holmes wrote:
Clarification ...
On 17/12/2019 12:40 pm, coleen.phillim...@oracle.com wrote:
Short answer below.
On 12/16/19 5:51 PM, David Holmes wrote:
Hi Coleen,
A quick initial response ...
On 16/12/2019 11:26 pm, coleen.phillim...@oracle.com wrote:
On 12/16/19 8:04 AM, David Holmes wrote:
Hi Coleen,
On 16/12/2019 9:41 pm, coleen.phillim...@oracle.com wrote:
Summary: Start ServiceThread before compiler threads, and run
nmethod barriers for zgc before adding to the service thread
queue, or posting the events on the java thread queue.
I can't comment on most of this but the earlier starting of the
service thread has some concerns:
- there is a lot of JDK level initialization which now will not
have happened before the service thread is started and it is far
from obvious that all possible initialization dependencies will be
satisfied
I agree that the order of initialization is very sensitive. From
the actions that the service thread does, the one that I found was
a problem was that events were posted before the LIVE phase (see
comment in has_events()), which could have happened with the
existing code, but the window for the race is a lot smaller. The
other actions can be run if there's a GC before initialization but
would be a bug in the initialization code, and I didn't find these
bugs in all my testing. There are some ordering dependencies that
do have odd side effects (between the compiler thread startup and
initialization jsr292 classes) which have comments. This patch
doesn't touch those.
- current starting of the service thread in Management::initialize
is guarded by "#if INCLUDE_MANAGEMENT", but now you are starting
the service thread unconditionally for all builds. Hmm just saw
your latest comment to the bug report - so the service thread is
now (for quite some time?) being used for other than management
tasks and so should always be present even if INCLUDE_MANAGEMENT
is not enabled. Is that sufficient or are there likely to be other
changes needed to actually ensure that all works correctly e.g.
any code the service thread executes that is only defined for
INCLUDE_MANAGEMENT will need to be compiled out explicitly.
I asked Jie offline to check the minimal build. I don't think
there are other INCLUDE_MANAGEMENT actions in the service thread
and I'm not sure why it was initialized there in the first place.
The minimal vm would have been broken ie. hashtables would not have
been cleaned up, etc, but I'm not sure how well that is tested or
if one would notice.
- the service thread and the notification thread are (were?)
closely related but now started at completely different times
The notification thread is limited to "services" so it makes sense
where it is. The ServiceThread does lots of other things. Maybe
it needs renaming in 15.
The bug report states the problem as:
"The graal crash is because compiled_method_load events are added
to the ServiceThread's deferred event queue before the
ServiceThread is created so are not walked to keep them from being
zombied."
so why isn't the solution to ensure the deferred event queue is
walked? I'm not clear how starting the service thread relates to
walking the queue.
The service thread is responsible for walking the deferred event
queue. See ServiceThread::oops_do/nmethods_do. The design could
be changed to have some global walk somewhere of this queue, but
essentially this queue is processed by the service thread.
Sorry I don't follow. I thought "oops_do" and friends are for the GC
threads and/or VMThread to call to process oops when GC updates them.
The oops_do and nmethods_do() can be called by a thread walk in
handshakes (by the sweeper thread) and by parallel GC thread walks.
There isn't a single entry to do the thread-specific closures that we
need to do for these deferred event queues. I tried a version that
walked the queues with a static call but missed some places where it
would be needed to make this call (didn't work). Keeping this
associated with the ServiceThread simplifies a lot.
Just to clarify that further, the thread walk requires the thread
appears in ALL_JAVA_THREADS but that only happens after the
ServiceThread has been started. So in essence we don't really need the
ServiceThread to have commenced execution earlier, but we need it to
have been created. Those two steps are combined in practice.
Yes. Then the ServiceThread waits on the Service_lock until notified by
these events:
while (((sensors_changed = (!UseNotificationThread &&
LowMemoryDetector::has_pending_requests())) |
(has_jvmti_events = _jvmti_service_queue.has_events()) |
(has_gc_notification_event = (!UseNotificationThread &&
GCNotifier::has_event())) |
(has_dcmd_notification_event = (!UseNotificationThread &&
DCmdFactory::has_pending_jmx_notification())) |
(stringtable_work = StringTable::has_work()) |
(symboltable_work = SymbolTable::has_work()) |
(resolved_method_table_work =
ResolvedMethodTable::has_work()) |
(thread_id_table_work = ThreadIdTable::has_work()) |
(protection_domain_table_work =
SystemDictionary::pd_cache_table()->has_work()) |
(oopstorage_work = OopStorage::has_cleanup_work_and_reset())
) == 0) {
The first, third and fourth events are from management.cpp events that
were initialized after the ServiceThread was started.
The second event I have changed, to wait until LIVE phase to return true.
The stringtable, symboltable, resolved_method_table, thread_id and pd
table have static _has_work variables initialized to false.
The oopstorage_work has similar, but may update a time-based counter a
bit earlier with the service thread starting earlier. I think this is
harmless.
It is possible that after the service thread starts and before the
compiler thread starts, there could be a GC that notifies the
stringtable to clean up. This seems like a good thing that the GC would
clean up these tables with this order. I ran the tier4 graal tests and
there were no failures.
Thanks,
Coleen
Cheers,
David
thanks,
Coleen
David
-----
I had an additional change to make the queue non-static but want to
limit the change at this point.
Thanks,
Coleen
Thanks,
David
See bug for description of the problems found with the new
Zombie.java test.
open webrev at
http://cr.openjdk.java.net/~coleenp/2019/8235829.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8235829
Ran tier1 all platforms, and tier2-8 testing, as well as
rerunning original test failure from bug
https://bugs.openjdk.java.net/browse/JDK-8173361.
Thanks,
Coleen
