RFR: 8225702(13): Safepoint counter can't be used for safepoint detection

[Daniel D. Daugherty]

On 6/14/19 4:26 AM, Robbin Ehn wrote:
> Hi all, please review.
>
> This bug fix introduced a new useage of the safepoint counter.
> Which does not work with the later pushed faster safepoint code.
> https://bugs.openjdk.java.net/browse/JDK-8215889
> http://hg.openjdk.java.net/jdk/jdk12/rev/4b469f5f4bf2
>
> In faster safepoint we must publish the new safepoint counter before 
> arming
> and publish the inactive counter after. Reading the safepoint counter 
> without
> having first looked at your local poll can causes faulty assumption about
> safepoint state.
>
> We add a new safepoint id instead which work reliable and we hide the 
> safepoint
> counter getter. (JNI fast get can still use safepoint counter, so it's 
> not
> completely hidden) And provide a simple class to track safepoint state.
> Also it makes no sense for a normal user that JFR/JMC safepoint id are 
> only odd,
> we instead use this new id which is just incremented for each safepoint.
>
> Code:
> http://cr.openjdk.java.net/~rehn/8225702/v1/webrev/index.html

src/hotspot/share/runtime/safepoint.hpp
     L52:   uint64_t _id;
         Please consider: 's/_id/_safepoint_id/'; see below for more 
renames.

     L55:   SafepointStateTracker(uint64_t current_id, bool at_safepoint);
         Please consider: 's/current_id/safepoint_id/'; see below for 
more renames.

     L103:   // Basic counter which increase inside each safepoint.
         nit - typo: s/increase/increases once/

         Perhaps add a bit more to the comment:

             // A change in this counter or a change in the result of
             // is_at_safepoint() are used by SafepointStateTracker::
             // safepoint_state_changed() to determine its answer.

     L165:     return SafepointStateTracker(_safepoint_id, _state == 
_synchronized);
         Should the second param be "is_at_safepoint()" instead?

src/hotspot/share/runtime/safepoint.cpp
     L122: SafepointStateTracker::SafepointStateTracker(uint64_t 
current, bool in_safepoint) {
     L123:   _id = current;
     L124:   _at_safepoint = in_safepoint;
         Please consider:
          SafepointStateTracker::SafepointStateTracker(uint64_t 
safepoint_id, bool at_safepoint) {
            _safepoint_id = safepoint_id;
            _at_safepoint = is_safepoint;

     L128:   return _id != SafepointSynchronize::_safepoint_id ||
         Please consider: 's/_id/_safepoint_id/'

src/hotspot/share/runtime/vmThread.cpp
     No comments.

src/hotspot/share/ci/ciMethodData.cpp
     No comments.

src/hotspot/share/code/dependencyContext.hpp
     No comments.

src/hotspot/share/runtime/biasedLocking.cpp
     Should this file include safepoint.hpp?

Dan

> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8225702
>
> Passes JFR test and t1-3.
>
> Thanks, Robbin