RFR: 8189276: Make SuspendibleThreadSet and related code available to other GCs

[Erik Österlund]

Hi Roman,

On 2017-10-17 23:34, Roman Kennke wrote:
> Am 17.10.2017 um 23:10 schrieb Roman Kennke:
>>
>>> The SuspendibleThreadSet API for synchronizing any non-Java thread 
>>> with safepoints currently resides under gc/g1. It is very useful for 
>>> other GCs too (in particular, Shenandoah does use it too), so I 
>>> wanted to move it to a common location like gc/common. Then Kim 
>>> Barrett commented that it might actually be useful for other threads 
>>> outside GC land and to put it under runtime/. So I did:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8189276/webrev.00/ 
>>> <http://cr.openjdk.java.net/%7Erkennke/8189276/webrev.00/>
>>>
>>> I also added a generic hook to call the STS from safepoint 
>>> sync/desync, which is consequently used by G1 now. In other words, 
>>> the CollectedHeap API that Erik Ö introduced is no longer used by 
>>> G1. Only CMS still uses that API because it has its own way to sync 
>>> with safepoints. I filed another bug 
>>> <https://bugs.openjdk.java.net/browse/JDK-8189364> for this. 
>>> Although I have my doubt it will ever be fixed. This seems to have 
>>> been very carefully evolved (to put it positive), and the risk of 
>>> breaking it is relatively high, and thus doesn't seem worth the 
>>> struggle to make it fit the STS.
>>>
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8189276
>>>
>>> What do you think? Ok to go in?
>>>
>> Replying to myself here.
>> I must admit that I am a bit reluctant to expose it to runtime/ 
>> unless there's a specific need for it. So maybe go back to the 
>> original plan to move it into gc/common and leave all the rest alone 
>> for now? What do others think?
>
> Sorry, forgot to add something. One reason why we originally wanted to 
> move it to gc/common (a new directory) instead of the existing 
> gc/shared was that Erik H objected that gc/shared would be built into 
> the minimal VM, and we probably wouldn't want that unless needed. 
> However, moving it to runtime/ (without actual need), would achieve 
> just that. So unless we can name an actual thread that would benefit 
> from synchronizing using the STS, I suggest to leave the STS code in 
> gc/common for now?
>
> That would be:
> http://cr.openjdk.java.net/~rkennke/8189276/webrev.01/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189276/webrev.01/>

We had some internal discussions, and the conclusion seems to be that we 
would like to not have a new gc/common directory for build-only 
purposes, but rather put it in gc/shared and explicitly mark files we do 
not want to have compiled in minimal VM in the 
make/hotspot/lib/JvmFeatures.gmk build file. You can remove files from 
the minimal VM build by explicitly adding files to an exclude list in 
that file.

133:  JVM_EXCLUDE_FILES += \
134:      concurrentGCThread.cpp \
+++ add your files here
135:      plab.cpp

I tend to agree that avoiding the new gc/common directory is probably a 
win right now. Hope you agree with this.

Thanks,
/Erik

> Roman

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20171018/aa886877/attachment.htm>