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

[Roman Kennke]

Am 18.10.2017 um 16:48 schrieb Roman Kennke:
> Am 18.10.2017 um 10:18 schrieb 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.
> Sure. This sounds like the best solution so far. Basically back to my 
> original plan :-)
>
> http://cr.openjdk.java.net/~rkennke/8189276/webrev.02/ 
> <http://cr.openjdk.java.net/%7Erkennke/8189276/webrev.02/>
>
Sorry, forget that patch. I uploaded the wrong webrev. Here's the 
correct one:

http://cr.openjdk.java.net/~rkennke/8189276/webrev.03/ 
<http://cr.openjdk.java.net/%7Erkennke/8189276/webrev.03/>

Roman