Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)

[Alan Bateman]

On Mon, 2 Nov 2020 11:26:51 GMT, Maurizio Cimadamore  
wrote:

>> I looked through the changes in this update.
>> 
>> The shared memory segment support looks sound and the mechanism to close a 
>> shared memory segment is clever (albeit a bit surprising at first that it 
>> does global handshake to look for a frame in a scoped region. Also 
>> surprising that close can cause failure at both ends - it took me a while to 
>> see that this is pragmatic approach).
>> 
>> The share method specifies NPE if thread == null but there is no thread 
>> parameter, is this a cut 'n paste error? Another one in registerCleaner 
>> where it should be NPE if the cleaner is null.
>> 
>> I think the javadoc for the close method needs to be a bit clearer on the 
>> state of the memory segment when IllegalStateException is thrown. Will it be 
>> marked "not alive" when it fails? Does this mean there is a resource leak? I 
>> think an apiNote to explain the rational for why close is not idempotent is 
>> also needed, or maybe it should be re-visited so that close is a no-op when 
>> the memory segment is not alive.
>> 
>> Now that MemorySegment is AutoCloseable then maybe the term "alive" should 
>> be replaced with "open" or  "closed" and isAlive replaced with isOpen is 
>> isClosed.
>> 
>> FileDescriptor can be attraction nuisance and forced reference counting 
>> everywhere that it is used. Is it needed? Could an isMapped method work 
>> instead?
>> 
>> mapFromPath was in the second preview but I think the method name should be 
>> re-examined as it maps a file, the path just locates the file.  Naming is 
>> subjectives but in this case using "map" or "mapFile" would fit beside the 
>> allocateNative methods.
>> 
>> MappedMemorySegments. The force method specifies a write back guarantee but 
>> at the same time, the implNote in the class description suggests that the 
>> methods might be a no-op. You might want to adjust the wording to avoid any 
>> suggestion that force might be a no-op.
>> 
>> The javadoc for copyFrom isn't changed in this update but I notice it 
>> specifies IndexOutOfBoundException when the source segment is larger than 
>> the receiver, have other exceptions been examined?
>> 
>> I don't have any any comments on MemoryAccess except that it's not 
>> immediately clear why there are "Byte" methods that take a ByteOrder. Make 
>> sense for the multi-byte types of course.
>> 
>> The updates the java/nio sources look okay but it would be helpful if the 
>> really long lines could be chopped down as it's just too hard to do 
>> side-by-side reviews when the lines are so long. A minor nit but the changes 
>> X-Buffer.java.template mess up the alignment of the parameters to 
>> copyMemory/copySwapMemory methods.
>
>> The javadoc for copyFrom isn't changed in this update but I notice it 
>> specifies IndexOutOfBoundException when the source segment is larger than 
>> the receiver, have other exceptions been examined?
> 
> This exception is consistent with other uses of this exception throughout 
> this API (e.g. when writing a segment out of bounds).

I assume the CSR needs to be updated so that it's in sync with the API changes 
in the latest round.

-

PR: https://git.openjdk.java.net/jdk/pull/548

Re: RFR: JDK-8255711: Fix and unify hotspot signal handlers

[David Holmes]

Hi Thomas,

Some initial comments as this is quite big - but thanks for all the 
detailed explanations.


On 4/11/2020 1:57 am, Thomas Stuefe wrote:

Hi all,

may I please have opinions and reviews on this cleanup-fix-patch for the 
hotspot signal handlers.

Its main intent is to simplify coding and to commonize some of it across all 
Posix platforms where possible. Also to fix a number of smaller issues.

This will have a number of benefits, mainly easing maintenance pain for porters 
and reducing bitrot for platform dependent code.

This all builds upon the work @gerard-ziemski did with 
https://bugs.openjdk.java.net/browse/JDK-8252324.



This cleanup was made more complicated by the fact that there exists a 
non-obvious and undocumented way for a third party app to chain signal handlers 
(beside the documented one of using libjsig). It seems that the 
JVM_handle_xxx_functions() are in fact interfaces for third party coding to 
invoke hotspot signal handling. This only makes sense in combination with 
`-XX:+AllowUserSignalHandlers`. A cursory github search revealed that this flag 
is used quite a bit.


Thanks for doing that search.


See a more in-depth discussion here: [4]. Thanks to @dholmes-ora for untangling 
this bit of history.

Unfortunately there is no official documentation from Sun or Oracle, and zero 
regression tests. So I try to preserve this interface as best as I can. I plan 
to add a proper regression test with a later change, but for now I don't have 
the time for that.


On the documentation front we could at least explain what the flag 
really does. Rather than saying:


 product(bool, AllowUserSignalHandlers, false, 
   \
  "Do not complain if the application installs signal handlers 
"


we could say something like:

 product(bool, AllowUserSignalHandlers, false, 
   \
  "Allow the application to install the primary signal handlers 
instead of the JVM." \


and we (I?) could update the java manpage.



---

The fixed issues:

1) `PosixSignals::_are_signal_handlers_installed` is used inside the platform 
handlers to guard a part of the platform handlers against execution in case the 
signal handlers are not yet installed.

Initially this confused me since when this handler is called it would of course 
be installed. So that boolean would always be true. The only explanation I 
found was that since these handlers can be invoked directly from outside, this 
is some (ineffective) form of guard against calling this handler too early.
But that guard can be left out and that boolean removed. Our signal handlers 
are safe to call before VM initialization is completed.


The code that is guarded by this check is implicitly safe as it is 
trivial and early in VM init the current thread will be null anyway and 
so we won't execute the guarded code.



2) The return code of JVM_handle_xxx_signal() was inconsistently set (some as 
bool, some as int) as well as unused in normal code paths (excluding outside 
calls).


Ok.


3) JVM_handle_xxx_signal are supposed to be exported, but on AIX there is a 
day-zero bug which caused it to not be exported.

4) Every platform handler has this section:

   JavaThread* thread = NULL;
   VMThread* vmthread = NULL;
   if (PosixSignals::are_signal_handlers_installed()) {
 if (t != NULL ){
   if(t->is_Java_thread()) {
 thread = t->as_Java_thread();
   }
   else if(t->is_VM_thread()){
 vmthread = (VMThread *)t;
   }
 }
   }

`vmthread` is unused on all platforms and can be removed.


Ok. Once upon a time we probably did something different if in the 
vmThread versus some other non-JavaThread.



5) Every platform handler has some variant of this section, to ignore SIGPIPE, 
SIGXFSZ (whose default actions are to terminate the VM):

   if (sig == SIGPIPE || sig == SIGXFSZ) {
 // allow chained handler to go first
 if (PosixSignals::chained_handler(sig, info, ucVoid)) {
   return true;
 } else {
   // Ignoring SIGPIPE/SIGXFSZ - see bugs 4229104 or 6499219
   return true;
 }
   }

- On s390 and ppc, we miss SIGXFSZ handling
_Update: Fixed separately for easier backport, see 
[https://bugs.openjdk.java.net/browse/JDK-8255734](JDK-8255734)_.
- both paths return true - section can be shortened

Side note: having handlers for those signals may be unnecessary. We could just 
set the signal handler to `SIG_IGN`. We would have to tiptoe around any third 
party handlers for those signals, but it still may be simpler.

6) At the end of every platform header, before calling into fatal error 
handling, we unblock the signal:


  // unmask current signal
  sigset_t newset;
  sigemptyset();
  sigaddset(, sig);
  sigprocmask(SIG_UNBLOCK, , NULL);



- Use of `sigprocmask()` is UB in a multithreaded program.
- but then, this section is unnecessary anyway, since 
[https://bugs.openjdk.java.net/browse/JDK-8252533](JDK-8252533) we unmask error 
signals at the start of the signal handler.

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v5]

[Serguei Spitsyn]

On Wed, 4 Nov 2020 02:15:52 GMT, Serguei Spitsyn  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code review comments from Kim and Albert.
>
> Hi Coleen,
> 
> Wow, there are a lot of simplifications and code removal with this fix!
> It looks great in general, just some nits below.
> I also wanted to suggest renaming the 'set_needs_processing' to 
> 'set_needs_rehashing'. :)
> 
> src/hotspot/share/prims/jvmtiTagMap.hpp:
> 
> Nit: Would it better to use a plural form 'post_dead_objects_on_vm_thread'? :
> `+  void post_dead_object_on_vm_thread();`
> 
> src/hotspot/share/prims/jvmtiTagMap.cpp:
> 
> Nit: It'd be nice to add a short comment before the check_hashmap similar to 
> L143 also explaining a difference (does check and post just for one env) with 
> the check_hashmaps_for_heapwalk:
> 122 void JvmtiTagMap::check_hashmap(bool post_events) {
>   . . .
> 143 // This checks for posting and rehashing and is called from the heap 
> walks.
>  144 void JvmtiTagMap::check_hashmaps_for_heapwalk() {
> 
> I'm just curious how this fragment was added. Did you get any failures in 
> testing? :
> 1038   // skip if object is a dormant shared object whose mirror hasn't been 
> loaded
> 1039   if (obj != NULL &&   obj->klass()->java_mirror() == NULL) {
> 1040 log_debug(cds, heap)("skipped dormant archived object " 
> INTPTR_FORMAT " (%s)", p2i(obj),
> 1041  obj->klass()->external_name());
> 1042 return;
> 1043   }
> 
> Nit: Can we rename this field to something like '_some_dead_found' or 
> '_dead_found'? :
> `1186   bool _some_dead;`
> 
> Nit: The lines 2997-3007 and 3009-3019 do the same but in different contexts. 
> 2996   if (!is_vm_thread) {
> 2997 if (num_dead_entries != 0) {
> 2998   JvmtiEnvIterator it;
> 2999   for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
> 3000 JvmtiTagMap* tag_map = env->tag_map_acquire();
> 3001 if (tag_map != NULL) {
> 3002   // Lock each hashmap from concurrent posting and cleaning
> 3003   tag_map->unlink_and_post_locked();
> 3004 }
> 3005   }
> 3006   // there's another callback for needs_rehashing
> 3007 }
> 3008   } else {
> 3009 assert(SafepointSynchronize::is_at_safepoint(), "must be");
> 3010 JvmtiEnvIterator it;
> 3011 for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
> 3012   JvmtiTagMap* tag_map = env->tag_map_acquire();
> 3013   if (tag_map != NULL && !tag_map->is_empty()) {
> 3014 if (num_dead_entries != 0) {
> 3015   tag_map->hashmap()->unlink_and_post(tag_map->env());
> 3016 }
> 3017 // Later GC code will relocate the oops, so defer rehashing 
> until then.
> 3018 tag_map->_needs_rehashing = true;
> 3019   }
> It feels like it can be refactored/simplified, at least, a little bit.
> Is it possible to check and just return if (num_dead_entries == 0)?
> If not, then, at least, it can be done the same way (except of locking).
> Q: Should the _needs_rehashing be set in both contexts?
> 
> Also, can we have just one (static?) lock for the whole gc_notification (not 
> per JVMTI env/JvmtiTagMap)? How much do we win by locking per each 
> env/JvmtiTagMap? Note, that in normal case there is just one agent. It is 
> very rare to have multiple agents requesting object tagging and ObjectFree 
> events. It seems, this can be refactored to more simple code with one 
> function doing work in both contexts.
> 
> src/hotspot/share/utilities/hashtable.cpp:
> 
> Nit: Need space after the '{' :
>   `+const int _small_table_sizes[] = {107, 1009, 2017, 4049, 5051, 10103, 
> 20201, 40423 } ;`
> 
> src/hotspot/share/prims/jvmtiTagMapTable.cpp:
> 
> Nit: Extra space after assert:
>   `119   assert (find(index, hash, obj) == NULL, "shouldn't already be 
> present");`
> 
> Thanks,
> Serguei

More about possible refactoring of the JvmtiTagMap::gc_notification().
I'm thinking about something like below:

void JvmtiTagMap::unlink_and_post_for_all_envs() {
  if (num_dead_entries == 0) {
return; // nothing to unlink and post
  }
  JvmtiEnvIterator it;
  for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
JvmtiTagMap* tag_map = env->tag_map_acquire();
if (tag_map != NULL && !tag_map->is_empty()) {
  tag_map->unlink_and_post();
}
  }
}

void JvmtiTagMap::gc_notification(size_t num_dead_entries) {
  if (Thread::current()->is_VM_thread()) {
assert(SafepointSynchronize::is_at_safepoint(), "must be");
unlink_and_post_for_all_envs();
set_needs_rehashing();
  } else {
MutexLocker ml(JvmtiTagMap_lock(), Mutex::_no_safepoint_check_flag);
unlink_and_post_for_all_envs();
// there's another callback for needs_rehashing
  }
}

If we still need a lock per each JvmtiTagMap then it is possible to add this 
fragment to the unlink_and_post_for_all_envs:
   bool is_vm_thread = 

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v5]

[Serguei Spitsyn]

On Wed, 4 Nov 2020 00:08:10 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comments from Kim and Albert.

Hi Coleen,

Wow, there are a lot of simplifications and code removal with this fix!
It looks great in general, just some nits below.
I also wanted to suggest renaming the 'set_needs_processing' to 
'set_needs_rehashing'. :)

src/hotspot/share/prims/jvmtiTagMap.hpp:

Nit: Would it better to use a plural form 'post_dead_objects_on_vm_thread'? :
`+  void post_dead_object_on_vm_thread();`

src/hotspot/share/prims/jvmtiTagMap.cpp:

Nit: It'd be nice to add a short comment before the check_hashmap similar to 
L143 also explaining a difference (does check and post just for one env) with 
the check_hashmaps_for_heapwalk:
122 void JvmtiTagMap::check_hashmap(bool post_events) {
  . . .
143 // This checks for posting and rehashing and is called from the heap walks.
 144 void JvmtiTagMap::check_hashmaps_for_heapwalk() {

I'm just curious how this fragment was added. Did you get any failures in 
testing? :
1038   // skip if object is a dormant shared object whose mirror hasn't been 
loaded
1039   if (obj != NULL &&   obj->klass()->java_mirror() == NULL) {
1040 log_debug(cds, heap)("skipped dormant archived object " INTPTR_FORMAT 
" (%s)", p2i(obj),
1041  obj->klass()->external_name());
1042 return;
1043   }

Nit: Can we rename this field to something like '_some_dead_found' or 
'_dead_found'? :
`1186   bool _some_dead;`

Nit: The lines 2997-3007 and 3009-3019 do the same but in different contexts. 
2996   if (!is_vm_thread) {
2997 if (num_dead_entries != 0) {
2998   JvmtiEnvIterator it;
2999   for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
3000 JvmtiTagMap* tag_map = env->tag_map_acquire();
3001 if (tag_map != NULL) {
3002   // Lock each hashmap from concurrent posting and cleaning
3003   tag_map->unlink_and_post_locked();
3004 }
3005   }
3006   // there's another callback for needs_rehashing
3007 }
3008   } else {
3009 assert(SafepointSynchronize::is_at_safepoint(), "must be");
3010 JvmtiEnvIterator it;
3011 for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
3012   JvmtiTagMap* tag_map = env->tag_map_acquire();
3013   if (tag_map != NULL && !tag_map->is_empty()) {
3014

Re: RFR: 8255853: Update all nroff manpages for JDK 16 release

[David Holmes]

Hi Magnus,

On 4/11/2020 6:50 am, Magnus Ihse Bursie wrote:

The man pages in src/$module/man/*.1 is generated from the (unfortunately 
still) closed markdown sources.

These needs to be updated for JDK 16. During this updating, a typo in the GPL 
header is also fixed (which caused Oracle's internal header verification tool 
to complain) -- a double space had been collapsed into a single space.

There is a certain possibility that man page content can still change for JDK 
16. If that is the case, the export needs to be re-done. (In contrast to prior 
exports, we now have a standard way of doing this, so it's trivial to update.)


I'd say there is a certainty that some man page content will still be 
updated before FC. Last time we at least waited until after RDP1 before 
doing the bulk update.


And I'm not sure this is really something that should be classified as a 
build task that only folk on build-dev will see.


Cheers,
David


-

Commit messages:
  - 8255853: Update all nroff manpages for JDK 16 release

Changes: https://git.openjdk.java.net/jdk/pull/1043/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1043=00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8255853
   Stats: 334 lines in 28 files changed: 202 ins; 25 del; 107 mod
   Patch: https://git.openjdk.java.net/jdk/pull/1043.diff
   Fetch: git fetch https://git.openjdk.java.net/jdk pull/1043/head:pull/1043

PR: https://git.openjdk.java.net/jdk/pull/1043

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v5]

[Coleen Phillimore]

> This change turns the HashTable that JVMTI uses for object tagging into a 
> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
> table to follow oops and then to rehash the table, this table points to 
> WeakHandle.  GC walks the backing OopStorages concurrently.
> 
> The hash function for the table is a hash of the lower 32 bits of the 
> address.  A flag is set during GC (gc_notification if in a safepoint, and 
> through a call to JvmtiTagMap::needs_processing()) so that the table is 
> rehashed at the next use.
> 
> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
> to post ObjectFree events.  In concurrent GCs there can be a window of time 
> between weak oop marking where the oop is unmarked, so dead (the phantom load 
> in peek returns NULL) but the gc_notification hasn't been done yet.  In this 
> window, a heap walk or GetObjectsWithTags call would not find an object 
> before the ObjectFree event is posted.  This is dealt with in two ways:
> 
> 1. In the Heap walk, there's an unconditional table walk to post events if 
> events are needed to post.
> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
> required, we use the VM thread to post the event.
> 
> Event posting cannot be done in a JavaThread because the posting needs to be 
> done while holding the table lock, so that the JvmtiEnv state doesn't change 
> before posting is done.  ObjectFree callbacks are limited in what they can do 
> as per the JVMTI Specification.  The allowed callbacks to the VM already have 
> code to allow NonJava threads.
> 
> To avoid rehashing, I also tried to use object->identity_hash() but this 
> breaks because entries can be added to the table during heapwalk, where the 
> objects use marking.  The starting markWord is saved and restored.  Adding a 
> hashcode during this operation makes restoring the former markWord (locked, 
> inflated, etc) too complicated.  Plus we don't want all these objects to have 
> hashcodes because locking operations after tagging would have to always use 
> inflated locks.
> 
> Much of this change is to remove serial weak oop processing for the 
> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
> jvmti code.
> 
> It has also been tested with tier1-6.
> 
> Thank you to Stefan, Erik and Kim for their help with this change.

Coleen Phillimore has updated the pull request incrementally with one 
additional commit since the last revision:

  Code review comments from Kim and Albert.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/967/files
  - new: https://git.openjdk.java.net/jdk/pull/967/files/1c3f2e1e..f66ea839

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=967=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=967=03-04

  Stats: 10 lines in 3 files changed: 0 ins; 4 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/967.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/967/head:pull/967

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Coleen Phillimore]

On Tue, 3 Nov 2020 21:12:49 GMT, Albert Mingkun Yang  wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into jvmti-table
>>  - Merge branch 'master' into jvmti-table
>>  - More review comments from Stefan and ErikO
>>  - Code review comments from StefanK.
>>  - 8212879: Make JVMTI TagMap table not hash on oop address
>
> src/hotspot/share/utilities/hashtable.cpp line 164:
> 
>> 162: }
>> 163:   }
>> 164:   return newsize;
> 
> It is existing code, but could be made clearer as part of this PR:
>   int newsize;
>   for (int i=0; i newsize = primelist[i];
> if (newsize >= requested)
>   break;
>   }
>   return newsize;
> Additionally, this method could be made `const`, right?
> 
> PS: not a review, just a comment in passing

Yes, that is a lot simpler and better.  I'd copied that code from another file 
without changing it that much.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Coleen Phillimore]

On Tue, 3 Nov 2020 21:47:24 GMT, Coleen Phillimore  wrote:

>> src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 50:
>> 
>>> 48: };
>>> 49: 
>>> 50: typedef uint WeakProcessorPhase;
>> 
>> This was originally written with the idea that WeakProcessorPhases::Phase 
>> (and WeakProcessorPhase) should be a scoped enum (but we didn't have that 
>> feature yet). It's possible there are places that don't cope with a scoped 
>> enum, since that feature wasn't available when the code was written, so 
>> there might have be mistakes.
>> 
>> But because of that, I'd prefer to keep the WeakProcessorPhases::Phase type 
>> and the existing definition of WeakProcessorPhase. Except this proposed 
>> change is breaking that at least here:
>> 
>> src/hotspot/share/gc/shared/weakProcessor.inline.hpp
>> 116 uint oopstorage_index = WeakProcessorPhases::oopstorage_index(phase);
>> 117 StorageState* cur_state = 
>> _storage_states.par_state(oopstorage_index);
>> =>
>> 103 StorageState* cur_state = _storage_states.par_state(phase);
>> 
>> I think eventually (as in some future RFE) this could all be collapsed to 
>> something provided by OopStorageSet.
>> enum class : uint WeakProcessorPhase {}; 
>> 
>> ENUMERATOR_RANGE(WeakProcessorPhase,
>>  static_cast(0), 
>>  static_cast(OopStorageSet::weak_count));
>> and replacing all uses of WeakProcessorPhases::Iterator with 
>> EnumIterator (which involves more than a type alias).
>> 
>> Though it might be possible to go even further and eliminate 
>> WeakProcessorPhases as a thing separate from OopStorageSet.
>
> Ok, so I'm not sure what to do with this:
> 
>  enum Phase { 
> // Serial phase.
> JVMTI_ONLY(jvmti)
> // Additional implicit phase values follow for oopstorages.
>   `};`
> 
> I've removed the only thing in this enum.

>Though it might be possible to go even further and eliminate 
>WeakProcessorPhases as a thing separate from OopStorageSet.

This makes sense.  Can we file another RFE for this? I was sort of surprised by 
how much code was involved so I tried to find a place to stop deleting.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Integrated: JDK-8255850: Hotspot recompiled on first incremental build

[Erik Joelsson]

On Tue, 3 Nov 2020 20:06:35 GMT, Erik Joelsson  wrote:

> After building the JDK from clean, the first incremental build of hotspot 
> will recompile all of it. This is caused by a difference in the CFLAGS 
> generated on the second go. The difference is generated in 
> JdkNativeCompilation.gmk, where the module specific java header dir is always 
> added to the list of include dirs. When compiling hotspot the first time, 
> there is no such dir, and so nothing is added, but the second go, later 
> compilation steps have created the base headers dir 
> ($(SUPPORT_OUTPUTDIR)/headers), which is found and picked up in CFLAGS. This 
> difference is then detected by the DependOnVariable construct for libjvm.
> 
> The fix here is to make sure SetupJdkLibrary is able to work in a context 
> without a MODULE defined, since that is how libjvm is built. In that case, no 
> java header dir should be added.
> 
> While fixing this I decided to go through the whole file and make sure all 
> uses of MODULE were protected when needed.

This pull request has now been integrated.

Changeset: 76fa974c
Author:Erik Joelsson 
URL:   https://git.openjdk.java.net/jdk/commit/76fa974c
Stats: 30 lines in 1 file changed: 23 ins; 0 del; 7 mod

8255850: Hotspot recompiled on first incremental build

Reviewed-by: ihse

-

PR: https://git.openjdk.java.net/jdk/pull/1041

Re: RFR: 8253892: Disable misleading-indentation on clang as well as gcc

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 22:36:34 GMT, John Paul Adrian Glaubitz 
 wrote:

>> With clang 10.0, the compiler now detects a new class of warnings. The 
>> `misleading-indentation` warning has previously been disabled on gcc for 
>> hotspot and libfdlibm.  Now we need to disable it for clang as well.
>
> Why do we disable the warning instead of fixing the incorrect indentations?

@glaubitz Good question. :) If you want to start fixing code to get rid of 
disabled warnings, I will not stand in your way!

For example, in hotspot and gcc, we have `parentheses comment unknown-pragmas 
address delete-non-virtual-dtor char-subscripts array-bounds 
int-in-bool-context ignored-qualifiers missing-field-initializers 
implicit-fallthrough empty-body strict-overflow sequence-point 
maybe-uninitialized misleading-indentation cast-function-type 
shift-negative-value`. I believe many of these should be fixed and removed from 
the list of disabled warnings.

But this bug is about disabling a warning in one compiler that we have already 
decided to disable in another.

-

PR: https://git.openjdk.java.net/jdk/pull/1044

Re: RFR: 8253892: Disable misleading-indentation on clang as well as gcc

[John Paul Adrian Glaubitz]

On Tue, 3 Nov 2020 22:25:08 GMT, Magnus Ihse Bursie  wrote:

> With clang 10.0, the compiler now detects a new class of warnings. The 
> `misleading-indentation` warning has previously been disabled on gcc for 
> hotspot and libfdlibm.  Now we need to disable it for clang as well.

Why do we disable the warning instead of fixing the incorrect indentations?

-

PR: https://git.openjdk.java.net/jdk/pull/1044

RFR: 8253892: Disable misleading-indentation on clang as well as gcc

[Magnus Ihse Bursie]

With clang 10.0, the compiler now detects a new class of warnings. The 
`misleading-indentation` warning has previously been disabled on gcc for 
hotspot and libfdlibm.  Now we need to disable it for clang as well.

-

Commit messages:
 - 8253892: Disable misleading-indentation on clang as well as gcc

Changes: https://git.openjdk.java.net/jdk/pull/1044/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1044=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8253892
  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1044.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1044/head:pull/1044

PR: https://git.openjdk.java.net/jdk/pull/1044

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Serguei Spitsyn]

On Tue, 3 Nov 2020 21:41:55 GMT, Coleen Phillimore  wrote:

> > @coleenp - please make sure you hear from someone on the Serviceability team
> > for this PR...
> 
> I've asked @sspitsyn to review this.

Yes, I'm reviewing this. Still need another pass.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Integrated: 8255853: Update all nroff manpages for JDK 16 release

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 20:43:01 GMT, Magnus Ihse Bursie  wrote:

> The man pages in src/$module/man/*.1 is generated from the (unfortunately 
> still) closed markdown sources.
> 
> These needs to be updated for JDK 16. During this updating, a typo in the GPL 
> header is also fixed (which caused Oracle's internal header verification tool 
> to complain) -- a double space had been collapsed into a single space.
> 
> There is a certain possibility that man page content can still change for JDK 
> 16. If that is the case, the export needs to be re-done. (In contrast to 
> prior exports, we now have a standard way of doing this, so it's trivial to 
> update.)

This pull request has now been integrated.

Changeset: 622f72bc
Author:Magnus Ihse Bursie 
URL:   https://git.openjdk.java.net/jdk/commit/622f72bc
Stats: 334 lines in 28 files changed: 202 ins; 25 del; 107 mod

8255853: Update all nroff manpages for JDK 16 release

Reviewed-by: erikj

-

PR: https://git.openjdk.java.net/jdk/pull/1043

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 20:37:07 GMT, Jorn Vernee  wrote:

>> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
>> 
>> This removes the reported warning.
>> 
>> Note that the _LP64 macro was not being propagated to the benchmark native 
>> libraries on Windows. The comment says that this is due to pack200, but 
>> since this has been removed [1], it seemed safe to propagate the macro now 
>> (backed up by testing).
>> 
>> CC'ing hotspot-runtime since I know some people there were looking forward 
>> to having this fixed.
>> 
>> Testing: tier1-3
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232022
>
> Jorn Vernee has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Collapse both _LP64 if blocks in flags-cflags.m4
>  - Use jlong.h macros instead of spinning new ones

Looks good to me now.

-

Marked as reviewed by ihse (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255853: Update all nroff manpages for JDK 16 release

[Erik Joelsson]

On Tue, 3 Nov 2020 20:43:01 GMT, Magnus Ihse Bursie  wrote:

> The man pages in src/$module/man/*.1 is generated from the (unfortunately 
> still) closed markdown sources.
> 
> These needs to be updated for JDK 16. During this updating, a typo in the GPL 
> header is also fixed (which caused Oracle's internal header verification tool 
> to complain) -- a double space had been collapsed into a single space.
> 
> There is a certain possibility that man page content can still change for JDK 
> 16. If that is the case, the export needs to be re-done. (In contrast to 
> prior exports, we now have a standard way of doing this, so it's trivial to 
> update.)

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1043

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Coleen Phillimore]

On Tue, 3 Nov 2020 13:45:57 GMT, Kim Barrett  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   More review comments from Stefan and ErikO
>
> src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 50:
> 
>> 48: };
>> 49: 
>> 50: typedef uint WeakProcessorPhase;
> 
> This was originally written with the idea that WeakProcessorPhases::Phase 
> (and WeakProcessorPhase) should be a scoped enum (but we didn't have that 
> feature yet). It's possible there are places that don't cope with a scoped 
> enum, since that feature wasn't available when the code was written, so there 
> might have be mistakes.
> 
> But because of that, I'd prefer to keep the WeakProcessorPhases::Phase type 
> and the existing definition of WeakProcessorPhase. Except this proposed 
> change is breaking that at least here:
> 
> src/hotspot/share/gc/shared/weakProcessor.inline.hpp
> 116 uint oopstorage_index = WeakProcessorPhases::oopstorage_index(phase);
> 117 StorageState* cur_state = _storage_states.par_state(oopstorage_index);
> =>
> 103 StorageState* cur_state = _storage_states.par_state(phase);
> 
> I think eventually (as in some future RFE) this could all be collapsed to 
> something provided by OopStorageSet.
> enum class : uint WeakProcessorPhase {}; 
> 
> ENUMERATOR_RANGE(WeakProcessorPhase,
>  static_cast(0), 
>  static_cast(OopStorageSet::weak_count));
> and replacing all uses of WeakProcessorPhases::Iterator with 
> EnumIterator (which involves more than a type alias).
> 
> Though it might be possible to go even further and eliminate 
> WeakProcessorPhases as a thing separate from OopStorageSet.

Ok, so I'm not sure what to do with this:

 enum Phase { 
// Serial phase.
JVMTI_ONLY(jvmti)
// Additional implicit phase values follow for oopstorages.
  `};`

I've removed the only thing in this enum.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Coleen Phillimore]

On Mon, 2 Nov 2020 13:19:08 GMT, Coleen Phillimore  wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into jvmti-table
>>  - Merge branch 'master' into jvmti-table
>>  - More review comments from Stefan and ErikO
>>  - Code review comments from StefanK.
>>  - 8212879: Make JVMTI TagMap table not hash on oop address
>
> I think I addressed your comments, retesting now.  Thank you!

> @coleenp - please make sure you hear from someone on the Serviceability team
> for this PR...

I've asked @sspitsyn to review this.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Coleen Phillimore]

On Tue, 3 Nov 2020 13:43:32 GMT, Kim Barrett  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   More review comments from Stefan and ErikO
>
> src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 41:
> 
>> 39:   class Iterator;
>> 40: 
>> 41:   typedef void (*Processor)(BoolObjectClosure*, OopClosure*);
> 
> I think this typedef is to support serial phases and that it is probably no 
> longer used.

ok, removed.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Coleen Phillimore]

On Tue, 3 Nov 2020 16:17:58 GMT, Kim Barrett  wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into jvmti-table
>>  - Merge branch 'master' into jvmti-table
>>  - More review comments from Stefan and ErikO
>>  - Code review comments from StefanK.
>>  - 8212879: Make JVMTI TagMap table not hash on oop address
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 127:
> 
>> 125:   // The table cleaning, posting and rehashing can race for
>> 126:   // concurrent GCs. So fix it here once we have a lock or are
>> 127:   // at a safepoint.
> 
> I think this comment and the one below about locking are confused, at least 
> about rehashing.  I _think_ this is referring to concurrent num-dead 
> notification?  I've already commented there about it being a problem to do 
> the unlink  in the GC pause (see later comment).  It also seems like a 
> bad idea to be doing this here and block progress by a concurrent GC because 
> we're holding the tagmap lock for a long time, which is another reason to not 
> have the num-dead notification do very much (and not require a lock that 
> might be held here for a long time).

The comment is trying to describe the situation like:
1. mark-end pause (WeakHandle.peek() returns NULL because object A is unmarked)
2. safepoint for heap walk
   2a. Need to post ObjectFree event for object A before the heap walk doesn't 
find object A.
3. gc_notification - would have posted an ObjectFree event for object A if the 
heapwalk hadn't intervened

The check_hashmap() function also checks whether the hash table needs to be 
rehashed before the next operation that uses the hashtable.

Both operations require the table to be locked.

The unlink and post needs to be in a GC pause for reasons that I stated above.  
The unlink and post were done in a GC pause so this isn't worse for any GCs.  
The lock can be held for concurrent GC while the number of entries are 
processed and this would be a delay for some applications that have requested a 
lot of tags, but these applications have asked for this and it's not worse than 
what we had with GC walking this table in safepoints.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Coleen Phillimore]

On Tue, 3 Nov 2020 16:12:21 GMT, Kim Barrett  wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into jvmti-table
>>  - Merge branch 'master' into jvmti-table
>>  - More review comments from Stefan and ErikO
>>  - Code review comments from StefanK.
>>  - 8212879: Make JVMTI TagMap table not hash on oop address
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 2979:
> 
>> 2977: 
>> 2978: // Concurrent GC needs to call this in relocation pause, so after the 
>> objects are moved
>> 2979: // and have their new addresses, the table can be rehashed.
> 
> I think the comment is confusing and wrong. The requirement is that the 
> collector must call this before exposing moved objects to the mutator, and 
> must provide the to-space invariant. (This whole design would not work with 
> the old Shenandoah barriers without additional work. I don't know if tagmaps 
> ever worked at all for them? Maybe they added calls to Access<>::resolve 
> (since happily deceased) to deal with that?)  I also think there are a bunch 
> of missing calls; piggybacking on the num-dead callback isn't correct (see 
> later comment about that).

So the design is that when the oops have new addresses, we set a flag in the 
table to rehash it.  Not sure why this is wrong and why wouldn't it work for 
shenandoah? @zhengyu123 ?  When we call WeakHandle.peek()/resolve() after the 
call, the new/moved oop address should be returned.  Why wouldn't this be the 
case?

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Daniel D . Daugherty]

On Mon, 2 Nov 2020 13:19:08 GMT, Coleen Phillimore  wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into jvmti-table
>>  - Merge branch 'master' into jvmti-table
>>  - More review comments from Stefan and ErikO
>>  - Code review comments from StefanK.
>>  - 8212879: Make JVMTI TagMap table not hash on oop address
>
> I think I addressed your comments, retesting now.  Thank you!

@coleenp - please make sure you hear from someone on the Serviceability team
for this PR...

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Coleen Phillimore]

On Tue, 3 Nov 2020 14:50:36 GMT, Kim Barrett  wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into jvmti-table
>>  - Merge branch 'master' into jvmti-table
>>  - More review comments from Stefan and ErikO
>>  - Code review comments from StefanK.
>>  - 8212879: Make JVMTI TagMap table not hash on oop address
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:
> 
>> 3013:   if (tag_map != NULL && !tag_map->is_empty()) {
>> 3014: if (num_dead_entries != 0) {
>> 3015:   tag_map->hashmap()->unlink_and_post(tag_map->env());
> 
> Why are we doing this in the callback, rather than just setting a flag?  I 
> thought part of the point of this change was to get tagmap processing out of 
> GC pauses.  The same question applies to the non-safepoint side.  The idea 
> was to be lazy about updating the tagmap, waiting until someone actually 
> needed to use it.  Or if more prompt ObjectFree notifications are needed then 
> signal some thread (maybe the service thread?) for followup.

The JVMTI code expects the posting to be done quite eagerly presumably during 
GC, before it has a chance to disable the event or some other such operation.  
So the posting is done during the notification because it's as soon as 
possible.  Deferring to the ServiceThread had two problems.  1. the event came 
later than the caller is expecting it, and in at least one test the event was 
disabled before posting, and 2. there's a comment in the code why we can't post 
events with a JavaThread.  We'd have to transition into native while holding a 
no safepoint lock (or else deadlock).  The point of making this change was so 
that the JVMTI table does not need GC code to serially process the table.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address

[Daniel D. Daugherty]

On 11/2/20 8:22 AM, Coleen Phillimore wrote:

On Mon, 2 Nov 2020 08:34:17 GMT, Stefan Karlsson  wrote:


src/hotspot/share/prims/jvmtiTagMap.cpp line 126:


124:   // concurrent GCs. So fix it here once we have a lock or are
125:   // at a safepoint.
126:   // SetTag and GetTag should not post events!

I think it would be good to explain why. Otherwise, this just leaves the 
readers wondering why this is the case.

Maybe even move this comment to the set_tag/get_tag code.

I was trying to explain why there's a boolean there but I can put this comment 
at both get_tag and set_tag.

   // Check if we have to processing for concurrent GCs.


Typo: s/we have to processing/we have to do processing/



   // GetTag should not post events because the JavaThread has to
   // transition to native for the callback and this cannot stop for
   // safepoints with the hashmap lock held.
   check_hashmap(false);

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Albert Mingkun Yang]

On Tue, 3 Nov 2020 14:53:12 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into jvmti-table
>  - Merge branch 'master' into jvmti-table
>  - More review comments from Stefan and ErikO
>  - Code review comments from StefanK.
>  - 8212879: Make JVMTI TagMap table not hash on oop address

src/hotspot/share/utilities/hashtable.cpp line 164:

> 162: }
> 163:   }
> 164:   return newsize;

It is existing code, but could be made clearer as part of this PR:
  int newsize;
  for (int i=0; i= requested)
  break;
  }
  return newsize;
Additionally, this method could be made `const`, right?

PS: not a review, just a comment in passing

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Coleen Phillimore]

On Tue, 3 Nov 2020 14:47:35 GMT, Kim Barrett  wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into jvmti-table
>>  - Merge branch 'master' into jvmti-table
>>  - More review comments from Stefan and ErikO
>>  - Code review comments from StefanK.
>>  - 8212879: Make JVMTI TagMap table not hash on oop address
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 3018:
> 
>> 3016: }
>> 3017: // Later GC code will relocate the oops, so defer rehashing 
>> until then.
>> 3018: tag_map->_needs_rehashing = true;
> 
> This is wrong for some collectors. I think all collectors ought to be calling 
> set_needs_rehashing in appropriate places, and it can't be be correctly 
> piggybacked on the num-dead callback. (See discussion above for that 
> function.)
> 
> For example, G1 remark pause does weak processing (including weak oopstorage) 
> and will call the num-dead callback, but does not move objects, so does not 
> require tagmap rehashing.
> 
> (I think CMS oldgen remark may have been similar, for what that's worth.)

Ok, so I'm going to need help to know where in all the different GCs to make 
this call.

-

PR: https://git.openjdk.java.net/jdk/pull/967

RFR: 8255853: Update all nroff manpages for JDK 16 release

[Magnus Ihse Bursie]

The man pages in src/$module/man/*.1 is generated from the (unfortunately 
still) closed markdown sources.

These needs to be updated for JDK 16. During this updating, a typo in the GPL 
header is also fixed (which caused Oracle's internal header verification tool 
to complain) -- a double space had been collapsed into a single space.

There is a certain possibility that man page content can still change for JDK 
16. If that is the case, the export needs to be re-done. (In contrast to prior 
exports, we now have a standard way of doing this, so it's trivial to update.)

-

Commit messages:
 - 8255853: Update all nroff manpages for JDK 16 release

Changes: https://git.openjdk.java.net/jdk/pull/1043/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1043=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255853
  Stats: 334 lines in 28 files changed: 202 ins; 25 del; 107 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1043.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1043/head:pull/1043

PR: https://git.openjdk.java.net/jdk/pull/1043

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]

[Coleen Phillimore]

On Tue, 3 Nov 2020 20:37:07 GMT, Jorn Vernee  wrote:

>> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
>> 
>> This removes the reported warning.
>> 
>> Note that the _LP64 macro was not being propagated to the benchmark native 
>> libraries on Windows. The comment says that this is due to pack200, but 
>> since this has been removed [1], it seemed safe to propagate the macro now 
>> (backed up by testing).
>> 
>> CC'ing hotspot-runtime since I know some people there were looking forward 
>> to having this fixed.
>> 
>> Testing: tier1-3
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232022
>
> Jorn Vernee has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Collapse both _LP64 if blocks in flags-cflags.m4
>  - Use jlong.h macros instead of spinning new ones

This looks better.

-

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]

[Jorn Vernee]

On Tue, 3 Nov 2020 19:08:05 GMT, Jorn Vernee  wrote:

>> I knew this looks familiar. Look at [existing macros in 
>> jlong_md.h](https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjava/jlong_md.h#L67-L81)
>>  and use/match them? There is a little difference in casting through `jint` 
>> in your code, while `jlong_md.h` does it via `int`.
>
> @shipilev Now that I look at the file you linked, it does look familiar to me 
> as well. Must have copy-pasted it from my subconscious ;)
> 
> Looks like this file is usable from the benchmark lib code as well, will try 
> to switch.

I've updated the PR with the following 2 changes:
- Use the pre-exsiting macros from jlong.h to convert between jlong and pointer
- Collapse the 2 if-blocks in flags-cflags.m4 for setting _LP64 into one

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]

[Jorn Vernee]

> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
> 
> This removes the reported warning.
> 
> Note that the _LP64 macro was not being propagated to the benchmark native 
> libraries on Windows. The comment says that this is due to pack200, but since 
> this has been removed [1], it seemed safe to propagate the macro now (backed 
> up by testing).
> 
> CC'ing hotspot-runtime since I know some people there were looking forward to 
> having this fixed.
> 
> Testing: tier1-3
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8232022

Jorn Vernee has updated the pull request incrementally with two additional 
commits since the last revision:

 - Collapse both _LP64 if blocks in flags-cflags.m4
 - Use jlong.h macros instead of spinning new ones

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1017/files
  - new: https://git.openjdk.java.net/jdk/pull/1017/files/18e2507d..6309e2ae

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1017=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1017=00-01

  Stats: 23 lines in 2 files changed: 1 ins; 12 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1017.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1017/head:pull/1017

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: JDK-8255850: Hotspot recompiled on first incremental build

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 20:06:35 GMT, Erik Joelsson  wrote:

> After building the JDK from clean, the first incremental build of hotspot 
> will recompile all of it. This is caused by a difference in the CFLAGS 
> generated on the second go. The difference is generated in 
> JdkNativeCompilation.gmk, where the module specific java header dir is always 
> added to the list of include dirs. When compiling hotspot the first time, 
> there is no such dir, and so nothing is added, but the second go, later 
> compilation steps have created the base headers dir 
> ($(SUPPORT_OUTPUTDIR)/headers), which is found and picked up in CFLAGS. This 
> difference is then detected by the DependOnVariable construct for libjvm.
> 
> The fix here is to make sure SetupJdkLibrary is able to work in a context 
> without a MODULE defined, since that is how libjvm is built. In that case, no 
> java header dir should be added.
> 
> While fixing this I decided to go through the whole file and make sure all 
> uses of MODULE were protected when needed.

Looks good to me. Thanks for fixing this, it has been highly annoying!

-

Marked as reviewed by ihse (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1041

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 19:41:49 GMT, Jorn Vernee  wrote:

>> Yes, that sounds good. I did not notice this (still not used to github 
>> reviews, which I think has too little context by default).
>
> Ok, will do. (FWIW, you can expand the context of the diff with the arrow 
> buttons on the left side of the view. Above or below the line numbers)

(Yes, I know.  I just didn't think that doing so would reveal anything about 
AIX. I just wish I had gotten a bit more context automatically.)

-

PR: https://git.openjdk.java.net/jdk/pull/1017

RFR: JDK-8255850: Hotspot recompiled on first incremental build

[Erik Joelsson]

After building the JDK from clean, the first incremental build of hotspot will 
recompile all of it. This is caused by a difference in the CFLAGS generated on 
the second go. The difference is generated in JdkNativeCompilation.gmk, where 
the module specific java header dir is always added to the list of include 
dirs. When compiling hotspot the first time, there is no such dir, and so 
nothing is added, but the second go, later compilation steps have created the 
base headers dir ($(SUPPORT_OUTPUTDIR)/headers), which is found and picked up 
in CFLAGS. This difference is then detected by the DependOnVariable construct 
for libjvm.

The fix here is to make sure SetupJdkLibrary is able to work in a context 
without a MODULE defined, since that is how libjvm is built. In that case, no 
java header dir should be added.

While fixing this I decided to go through the whole file and make sure all uses 
of MODULE were protected when needed.

-

Commit messages:
 - Handle a MODULE free context better in SetupJdk* macros

Changes: https://git.openjdk.java.net/jdk/pull/1041/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1041=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255850
  Stats: 30 lines in 1 file changed: 23 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1041.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1041/head:pull/1041

PR: https://git.openjdk.java.net/jdk/pull/1041

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Jorn Vernee]

On Tue, 3 Nov 2020 19:16:02 GMT, Magnus Ihse Bursie  wrote:

>> Thanks for the suggestion.
>> 
>> Note the code that adds _LP64 for the JVM (below):
>> 
>> if test "x$FLAGS_OS" != xaix; then
>>   # xlc on AIX defines _LP64=1 by default and issues a warning if we 
>> redefine it.
>>   $1_DEFINES_CPU_JVM="${$1_DEFINES_CPU_JVM} -D_LP64=1"
>> fi
>> 
>> So, it seems xlc/aix explicitly does _not_ want this macro defined?
>> 
>> I think we could reuse that if-block to add `-D_LP64=` to both 
>> `1_DEFINES_CPU_JDK`, and `1_DEFINES_CPU_JVM` though, and remove the first 
>> one that checks for linux/mac/windows.
>
> Yes, that sounds good. I did not notice this (still not used to github 
> reviews, which I think has too little context by default).

Ok, will do. (FWIW, you can expand the context of the diff with the arrow 
buttons on the left side of the view. Above or below the line numbers)

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 18:52:52 GMT, Jorn Vernee  wrote:

>> make/autoconf/flags-cflags.m4 line 667:
>> 
>>> 665: 
>>> 666:   if test "x$FLAGS_CPU_BITS" = x64; then
>>> 667: if test "x$FLAGS_OS" = xlinux || test "x$FLAGS_OS" = xmacosx || 
>>> test "x$FLAGS_OS" = xwindows; then
>> 
>> At this point, you're almost testing for all supported OSes.  :) I can only 
>> think of AIX that does not match this if clause. I think it would be better 
>> to just remove the if and always define _LP64=1 on 64-bit platforms. 
>> 
>> @simonis Would that be OK for AIX?
>
> Thanks for the suggestion.
> 
> Note the code that adds _LP64 for the JVM (below):
> 
> if test "x$FLAGS_OS" != xaix; then
>   # xlc on AIX defines _LP64=1 by default and issues a warning if we 
> redefine it.
>   $1_DEFINES_CPU_JVM="${$1_DEFINES_CPU_JVM} -D_LP64=1"
> fi
> 
> So, it seems xlc/aix explicitly does _not_ want this macro defined?
> 
> I think we could reuse that if-block to add `-D_LP64=` to both 
> `1_DEFINES_CPU_JDK`, and `1_DEFINES_CPU_JVM` though, and remove the first one 
> that checks for linux/mac/windows.

Yes, that sounds good. I did not notice this (still not used to github reviews, 
which I think has too little context by default).

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Jorn Vernee]

On Tue, 3 Nov 2020 18:30:46 GMT, Aleksey Shipilev  wrote:

>> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
>> 
>> This removes the reported warning.
>> 
>> Note that the _LP64 macro was not being propagated to the benchmark native 
>> libraries on Windows. The comment says that this is due to pack200, but 
>> since this has been removed [1], it seemed safe to propagate the macro now 
>> (backed up by testing).
>> 
>> CC'ing hotspot-runtime since I know some people there were looking forward 
>> to having this fixed.
>> 
>> Testing: tier1-3
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232022
>
> I knew this looks familiar. Look at [existing macros in 
> jlong_md.h](https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjava/jlong_md.h#L67-L81)
>  and use/match them? There is a little difference in casting through `jint` 
> in your code, while `jlong_md.h` does it via `int`.

@shipilev Now that I look at the file you linked, it does look familiar to me 
as well. Must have copy-pasted it from my subconscious ;)

Looks like this file is usable from the benchmark lib code as well, will try to 
switch.

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Integrated: 8254827: JVMCI: Enable it for Windows+AArch64

[Bernhard Urban-Forster]

On Thu, 15 Oct 2020 15:00:47 GMT, Bernhard Urban-Forster  
wrote:

> Use r18 as allocatable register on Linux only.
> 
> A bootstrap works now (it has been crashing before due to r18 being 
> allocated):
> $ ./windows-aarch64-server-fastdebug/bin/java.exe 
> -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI 
> -version
> Bootstrapping JVMCI. in 17990 ms
> (compiled 3330 methods)
> openjdk version "16-internal" 2021-03-16
> OpenJDK Runtime Environment (fastdebug build 
> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk)
> OpenJDK 64-Bit Server VM (fastdebug build 
> 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode)
> 
> Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well.

This pull request has now been integrated.

Changeset: 88ee9733
Author:Bernhard Urban-Forster 
Committer: Tom Rodriguez 
URL:   https://git.openjdk.java.net/jdk/commit/88ee9733
Stats: 24 lines in 4 files changed: 15 ins; 0 del; 9 mod

8254827: JVMCI: Enable it for Windows+AArch64

Reviewed-by: ihse, never, kvn

-

PR: https://git.openjdk.java.net/jdk/pull/685

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Jorn Vernee]

On Tue, 3 Nov 2020 18:46:29 GMT, Magnus Ihse Bursie  wrote:

>> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
>> 
>> This removes the reported warning.
>> 
>> Note that the _LP64 macro was not being propagated to the benchmark native 
>> libraries on Windows. The comment says that this is due to pack200, but 
>> since this has been removed [1], it seemed safe to propagate the macro now 
>> (backed up by testing).
>> 
>> CC'ing hotspot-runtime since I know some people there were looking forward 
>> to having this fixed.
>> 
>> Testing: tier1-3
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232022
>
> make/autoconf/flags-cflags.m4 line 667:
> 
>> 665: 
>> 666:   if test "x$FLAGS_CPU_BITS" = x64; then
>> 667: if test "x$FLAGS_OS" = xlinux || test "x$FLAGS_OS" = xmacosx || 
>> test "x$FLAGS_OS" = xwindows; then
> 
> At this point, you're almost testing for all supported OSes.  :) I can only 
> think of AIX that does not match this if clause. I think it would be better 
> to just remove the if and always define _LP64=1 on 64-bit platforms. 
> 
> @simonis Would that be OK for AIX?

Thanks for the suggestion.

Note the code that adds _LP64 for the JVM (below):

if test "x$FLAGS_OS" != xaix; then
  # xlc on AIX defines _LP64=1 by default and issues a warning if we 
redefine it.
  $1_DEFINES_CPU_JVM="${$1_DEFINES_CPU_JVM} -D_LP64=1"
fi

So, it seems xlc/aix explicitly does _not_ want this macro defined?

I think we could reuse that if-block to add `-D_LP64=` to both 
`1_DEFINES_CPU_JDK`, and `1_DEFINES_CPU_JVM` though, and remove the first one 
that checks for linux/mac/windows.

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Magnus Ihse Bursie]

On Mon, 2 Nov 2020 18:36:32 GMT, Jorn Vernee  wrote:

> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
> 
> This removes the reported warning.
> 
> Note that the _LP64 macro was not being propagated to the benchmark native 
> libraries on Windows. The comment says that this is due to pack200, but since 
> this has been removed [1], it seemed safe to propagate the macro now (backed 
> up by testing).
> 
> CC'ing hotspot-runtime since I know some people there were looking forward to 
> having this fixed.
> 
> Testing: tier1-3
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8232022

Changes requested by ihse (Reviewer).

make/autoconf/flags-cflags.m4 line 667:

> 665: 
> 666:   if test "x$FLAGS_CPU_BITS" = x64; then
> 667: if test "x$FLAGS_OS" = xlinux || test "x$FLAGS_OS" = xmacosx || test 
> "x$FLAGS_OS" = xwindows; then

At this point, you're almost testing for all supported OSes.  :) I can only 
think of AIX that does not match this if clause. I think it would be better to 
just remove the if and always define _LP64=1 on 64-bit platforms. 

@simonis Would that be OK for AIX?

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Aleksey Shipilev]

On Mon, 2 Nov 2020 18:36:32 GMT, Jorn Vernee  wrote:

> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
> 
> This removes the reported warning.
> 
> Note that the _LP64 macro was not being propagated to the benchmark native 
> libraries on Windows. The comment says that this is due to pack200, but since 
> this has been removed [1], it seemed safe to propagate the macro now (backed 
> up by testing).
> 
> CC'ing hotspot-runtime since I know some people there were looking forward to 
> having this fixed.
> 
> Testing: tier1-3
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8232022

I knew this looks familiar. Look at [existing macros in 
jlong_md.h](https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjava/jlong_md.h#L67-L81)
 and use/match them? There is a little difference in casting through `jint` in 
your code, while `jlong_md.h` does it via `int`.

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Kim Barrett]

On Tue, 3 Nov 2020 14:53:12 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into jvmti-table
>  - Merge branch 'master' into jvmti-table
>  - More review comments from Stefan and ErikO
>  - Code review comments from StefanK.
>  - 8212879: Make JVMTI TagMap table not hash on oop address

src/hotspot/share/prims/jvmtiTagMap.cpp line 3018:

> 3016: }
> 3017: // Later GC code will relocate the oops, so defer rehashing 
> until then.
> 3018: tag_map->_needs_rehashing = true;

This is wrong for some collectors. I think all collectors ought to be calling 
set_needs_rehashing in appropriate places, and it can't be be correctly 
piggybacked on the num-dead callback. (See discussion above for that function.)

For example, G1 remark pause does weak processing (including weak oopstorage) 
and will call the num-dead callback, but does not move objects, so does not 
require tagmap rehashing.

(I think CMS oldgen remark may have been similar, for what that's worth.)

src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:

> 3013:   if (tag_map != NULL && !tag_map->is_empty()) {
> 3014: if (num_dead_entries != 0) {
> 3015:   tag_map->hashmap()->unlink_and_post(tag_map->env());

Why are we doing this in the callback, rather than just setting a flag?  I 
thought part of the point of this change was to get tagmap processing out of GC 
pauses.  The same question applies to the non-safepoint side.  The idea was to 
be lazy about updating the tagmap, waiting until someone actually needed to use 
it.  Or if more prompt ObjectFree notifications are needed then signal some 
thread (maybe the service thread?) for followup.

src/hotspot/share/prims/jvmtiTagMap.cpp line 2979:

> 2977: 
> 2978: // Concurrent GC needs to call this in relocation pause, so after the 
> objects are moved
> 2979: // and have their new addresses, the table can be rehashed.

I think the comment is confusing and wrong. The requirement is that the 
collector must call this before exposing moved objects to the mutator, and must 
provide the to-space invariant. (This whole design would not work with the old 
Shenandoah barriers without additional work. I don't know if tagmaps ever 
worked at all for them? Maybe they added calls to Access<>::resolve (since 
happily deceased) to deal with that?)  I 

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Kim Barrett]

On Tue, 3 Nov 2020 12:58:22 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More review comments from Stefan and ErikO

src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 41:

> 39:   class Iterator;
> 40: 
> 41:   typedef void (*Processor)(BoolObjectClosure*, OopClosure*);

I think this typedef is to support serial phases and that it is probably no 
longer used.

src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 50:

> 48: };
> 49: 
> 50: typedef uint WeakProcessorPhase;

This was originally written with the idea that WeakProcessorPhases::Phase (and 
WeakProcessorPhase) should be a scoped enum (but we didn't have that feature 
yet). It's possible there are places that don't cope with a scoped enum, since 
that feature wasn't available when the code was written, so there might have be 
mistakes.

But because of that, I'd prefer to keep the WeakProcessorPhases::Phase type and 
the existing definition of WeakProcessorPhase. Except this proposed change is 
breaking that at least here:

src/hotspot/share/gc/shared/weakProcessor.inline.hpp
116 uint oopstorage_index = WeakProcessorPhases::oopstorage_index(phase);
117 StorageState* cur_state = _storage_states.par_state(oopstorage_index);
=>
103 StorageState* cur_state = _storage_states.par_state(phase);

I think eventually (as in some future RFE) this could all be collapsed to 
something provided by OopStorageSet.
enum class : uint WeakProcessorPhase {}; 

ENUMERATOR_RANGE(WeakProcessorPhase,
 static_cast(0), 
 static_cast(OopStorageSet::weak_count));
and replacing all uses of WeakProcessorPhases::Iterator with 
EnumIterator (which involves more than a type alias).

Though it might be possible to go even further and eliminate 
WeakProcessorPhases as a thing separate from OopStorageSet.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Coleen Phillimore]

On Mon, 2 Nov 2020 18:36:32 GMT, Jorn Vernee  wrote:

> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
> 
> This removes the reported warning.
> 
> Note that the _LP64 macro was not being propagated to the benchmark native 
> libraries on Windows. The comment says that this is due to pack200, but since 
> this has been removed [1], it seemed safe to propagate the macro now (backed 
> up by testing).
> 
> CC'ing hotspot-runtime since I know some people there were looking forward to 
> having this fixed.
> 
> Testing: tier1-3
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8232022

Marked as reviewed by coleenp (Reviewer).

test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/libJNIPoint.c 
line 32:

> 30: #define PTR_TO_JLONG(value) ((jlong) (value))
> 31: #else
> 32: #define JLONG_TO_PTR(value, type) ((type*) (jint) (value))

Maybe the jlong thisPoint argument comes from a pointer so it's ok.  Not nice, 
but if you say so, I'll go along.

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Jorn Vernee]

On Tue, 3 Nov 2020 17:42:08 GMT, Jorn Vernee  wrote:

>> test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/libJNIPoint.c
>>  line 32:
>> 
>>> 30: #define PTR_TO_JLONG(value) ((jlong) (value))
>>> 31: #else
>>> 32: #define JLONG_TO_PTR(value, type) ((type*) (jint) (value))
>> 
>> Maybe the jlong thisPoint argument comes from a pointer so it's ok.  Not 
>> nice, but if you say so, I'll go along.
>
> Yes, it's the same pointer that is returned from allocate. It's just stored 
> in a `jlong` on the Java side (this would be a requirement on x64), but it's 
> not expected that the high-order bits are used.
> 
> Do you have a suggestion for handling this otherwise?

Hmm, now that I think about it, we could add overloads that work with `int` for 
32-bit platforms.

But, for now 32-bit usage of these benchmarks seems unlikely. Since the build 
passes, and the benchmarks run on both platforms already, I'm reluctant to put 
much more effort into this at the moment.

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Jorn Vernee]

On Tue, 3 Nov 2020 17:40:32 GMT, Coleen Phillimore  wrote:

>> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
>> 
>> This removes the reported warning.
>> 
>> Note that the _LP64 macro was not being propagated to the benchmark native 
>> libraries on Windows. The comment says that this is due to pack200, but 
>> since this has been removed [1], it seemed safe to propagate the macro now 
>> (backed up by testing).
>> 
>> CC'ing hotspot-runtime since I know some people there were looking forward 
>> to having this fixed.
>> 
>> Testing: tier1-3
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232022
>
> test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/libJNIPoint.c
>  line 32:
> 
>> 30: #define PTR_TO_JLONG(value) ((jlong) (value))
>> 31: #else
>> 32: #define JLONG_TO_PTR(value, type) ((type*) (jint) (value))
> 
> Maybe the jlong thisPoint argument comes from a pointer so it's ok.  Not 
> nice, but if you say so, I'll go along.

Yes, it's the same pointer that is returned from allocate. It's just stored in 
a `jlong` on the Java side (this would be a requirement on x64), but it's not 
expected that the high-order bits are used.

Do you have a suggestion for handling this otherwise?

-

PR: https://git.openjdk.java.net/jdk/pull/1017

Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Coleen Phillimore]

On Tue, 3 Nov 2020 17:40:38 GMT, Coleen Phillimore  wrote:

>> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
>> 
>> This removes the reported warning.
>> 
>> Note that the _LP64 macro was not being propagated to the benchmark native 
>> libraries on Windows. The comment says that this is due to pack200, but 
>> since this has been removed [1], it seemed safe to propagate the macro now 
>> (backed up by testing).
>> 
>> CC'ing hotspot-runtime since I know some people there were looking forward 
>> to having this fixed.
>> 
>> Testing: tier1-3
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232022
>
> Marked as reviewed by coleenp (Reviewer).

I was anxious for this to compile so I reviewed it. Thanks!

-

PR: https://git.openjdk.java.net/jdk/pull/1017

RFR: 8255128: linux x86 build failure with libJNIPoint.c

[Jorn Vernee]

Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c

This removes the reported warning.

Note that the _LP64 macro was not being propagated to the benchmark native 
libraries on Windows. The comment says that this is due to pack200, but since 
this has been removed [1], it seemed safe to propagate the macro now (backed up 
by testing).

CC'ing hotspot-runtime since I know some people there were looking forward to 
having this fixed.

Testing: tier1-3

[1] https://bugs.openjdk.java.net/browse/JDK-8232022

-

Commit messages:
 - Add 32-bit safe version of jlong <-> casts to libJNIPoint.c

Changes: https://git.openjdk.java.net/jdk/pull/1017/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1017=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255128
  Stats: 17 lines in 2 files changed: 8 ins; 2 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1017.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1017/head:pull/1017

PR: https://git.openjdk.java.net/jdk/pull/1017

RFR: JDK-8255711: Fix and unify hotspot signal handlers

[Thomas Stuefe]

Hi all,

may I please have opinions and reviews on this cleanup-fix-patch for the 
hotspot signal handlers.

Its main intent is to simplify coding and to commonize some of it across all 
Posix platforms where possible. Also to fix a number of smaller issues.

This will have a number of benefits, mainly easing maintenance pain for porters 
and reducing bitrot for platform dependent code.

This all builds upon the work @gerard-ziemski did with 
https://bugs.openjdk.java.net/browse/JDK-8252324.



This cleanup was made more complicated by the fact that there exists a 
non-obvious and undocumented way for a third party app to chain signal handlers 
(beside the documented one of using libjsig). It seems that the 
JVM_handle_xxx_functions() are in fact interfaces for third party coding to 
invoke hotspot signal handling. This only makes sense in combination with 
`-XX:+AllowUserSignalHandlers`. A cursory github search revealed that this flag 
is used quite a bit.

See a more in-depth discussion here: [4]. Thanks to @dholmes-ora for untangling 
this bit of history.

Unfortunately there is no official documentation from Sun or Oracle, and zero 
regression tests. So I try to preserve this interface as best as I can. I plan 
to add a proper regression test with a later change, but for now I don't have 
the time for that.

---

The fixed issues:

1) `PosixSignals::_are_signal_handlers_installed` is used inside the platform 
handlers to guard a part of the platform handlers against execution in case the 
signal handlers are not yet installed. 

Initially this confused me since when this handler is called it would of course 
be installed. So that boolean would always be true. The only explanation I 
found was that since these handlers can be invoked directly from outside, this 
is some (ineffective) form of guard against calling this handler too early. 
But that guard can be left out and that boolean removed. Our signal handlers 
are safe to call before VM initialization is completed.

2) The return code of JVM_handle_xxx_signal() was inconsistently set (some as 
bool, some as int) as well as unused in normal code paths (excluding outside 
calls).

3) JVM_handle_xxx_signal are supposed to be exported, but on AIX there is a 
day-zero bug which caused it to not be exported.

4) Every platform handler has this section:

  JavaThread* thread = NULL;
  VMThread* vmthread = NULL;
  if (PosixSignals::are_signal_handlers_installed()) {
if (t != NULL ){
  if(t->is_Java_thread()) {
thread = t->as_Java_thread();
  }
  else if(t->is_VM_thread()){
vmthread = (VMThread *)t;
  }
}
  }

`vmthread` is unused on all platforms and can be removed.

5) Every platform handler has some variant of this section, to ignore SIGPIPE, 
SIGXFSZ (whose default actions are to terminate the VM):

  if (sig == SIGPIPE || sig == SIGXFSZ) {
// allow chained handler to go first
if (PosixSignals::chained_handler(sig, info, ucVoid)) {
  return true;
} else {
  // Ignoring SIGPIPE/SIGXFSZ - see bugs 4229104 or 6499219
  return true;
}
  }

- On s390 and ppc, we miss SIGXFSZ handling 
   _Update: Fixed separately for easier backport, see 
[https://bugs.openjdk.java.net/browse/JDK-8255734](JDK-8255734)_.
- both paths return true - section can be shortened

Side note: having handlers for those signals may be unnecessary. We could just 
set the signal handler to `SIG_IGN`. We would have to tiptoe around any third 
party handlers for those signals, but it still may be simpler.

6) At the end of every platform header, before calling into fatal error 
handling, we unblock the signal:

>  // unmask current signal
>  sigset_t newset;
>  sigemptyset();
>  sigaddset(, sig);
>  sigprocmask(SIG_UNBLOCK, , NULL);
>

- Use of `sigprocmask()` is UB in a multithreaded program. 
- but then, this section is unnecessary anyway, since 
[https://bugs.openjdk.java.net/browse/JDK-8252533](JDK-8252533) we unmask error 
signals at the start of the signal handler.

7) the JFR crash protection is not consistently checked in all platform 
handlers.

8) On Zero, when entering fatal error handling, we do so via fatal() instead of 
VMError::report_and_die(), thereby discarding the real crash context and 
obfuscating the register content in the hs-err file (we still see registers, 
but those stem from the assertion-poison-page mechanism).

9) on Linux ppc64 and AIX, we have this section:

>  if (sig == SIGILL && (pc < (address) 0x200)) {
>goto report_and_die;
>  }

which is related to the fact that the zero page on AIX is readable, filled with 
0, and reading instructions from it will yield us a SIGILL, not a SIGSEGV (0 is 
not a noop on PPC, so we don't nop-slide).

This coding is irrelevant on Linux. On AIX, it can also be removed, since this 
SIGILL would be unrecognized by the hotspot and later count as fatal error 
anyway.

10) When invoking the fatal error handler, we extract the pc from the 

Integrated: 8255798: Remove dead headless code in CompileJavaModules.gmk

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 08:21:22 GMT, Magnus Ihse Bursie  wrote:

> The variable BUILD_HEADLESS_ONLY is no longer set. And sun/applet does not 
> exist anymore.

This pull request has now been integrated.

Changeset: 64a98112
Author:Magnus Ihse Bursie 
URL:   https://git.openjdk.java.net/jdk/commit/64a98112
Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod

8255798: Remove dead headless code in CompileJavaModules.gmk

Reviewed-by: shade, erikj

-

PR: https://git.openjdk.java.net/jdk/pull/1031

Integrated: 8255801: Race when building ct.sym build tools

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 13:39:19 GMT, Magnus Ihse Bursie  wrote:

> There is a race when compiling build.tools.symbolgenerator.CreateSymbols:
> 
> [2020-11-03T07:21:12,118Z] Error: LinkageError occurred while loading main 
> class build.tools.symbolgenerator.CreateSymbols
> [2020-11-03T07:21:12,119Z] java.lang.ClassFormatError: Truncated class file
> [2020-11-03T07:21:12,125Z] Gendata.gmk:69: recipe for target 
> '/.../build/macosx-x64-open/support/javadoc-symbols/symbols' failed
> [2020-11-03T07:21:12,125Z] make[3]: *** 
> [/.../build/macosx-x64-open/support/javadoc-symbols/symbols] Error 1
> [2020-11-03T07:21:12,127Z] make/Main.gmk:148: recipe for target 
> 'jdk.javadoc-gendata' failed
> [2020-11-03T07:21:12,127Z] make[2]: *** [jdk.javadoc-gendata] Error 2
> [2020-11-03T07:21:12,127Z] make[2]: *** Waiting for unfinished jobs
> 
> This was caused by https://bugs.openjdk.java.net/browse/JDK-8216497, which 
> started compiling the same buildtool twic, in different, independent 
> makefiles, to the same output directory.
> 
> The long term correct solution is to fix 
> https://bugs.openjdk.java.net/browse/JDK-8241463.
> 
> For now, I opted to just make sure the output is placed in separate 
> directories to avoid the race. We will still compile the build tool twice; 
> it's annoying but not a major performance problem.

This pull request has now been integrated.

Changeset: 364b0fe8
Author:Magnus Ihse Bursie 
URL:   https://git.openjdk.java.net/jdk/commit/364b0fe8
Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod

8255801: Race when building ct.sym build tools

Reviewed-by: erikj

-

PR: https://git.openjdk.java.net/jdk/pull/1037

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Coleen Phillimore]

> This change turns the HashTable that JVMTI uses for object tagging into a 
> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
> table to follow oops and then to rehash the table, this table points to 
> WeakHandle.  GC walks the backing OopStorages concurrently.
> 
> The hash function for the table is a hash of the lower 32 bits of the 
> address.  A flag is set during GC (gc_notification if in a safepoint, and 
> through a call to JvmtiTagMap::needs_processing()) so that the table is 
> rehashed at the next use.
> 
> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
> to post ObjectFree events.  In concurrent GCs there can be a window of time 
> between weak oop marking where the oop is unmarked, so dead (the phantom load 
> in peek returns NULL) but the gc_notification hasn't been done yet.  In this 
> window, a heap walk or GetObjectsWithTags call would not find an object 
> before the ObjectFree event is posted.  This is dealt with in two ways:
> 
> 1. In the Heap walk, there's an unconditional table walk to post events if 
> events are needed to post.
> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
> required, we use the VM thread to post the event.
> 
> Event posting cannot be done in a JavaThread because the posting needs to be 
> done while holding the table lock, so that the JvmtiEnv state doesn't change 
> before posting is done.  ObjectFree callbacks are limited in what they can do 
> as per the JVMTI Specification.  The allowed callbacks to the VM already have 
> code to allow NonJava threads.
> 
> To avoid rehashing, I also tried to use object->identity_hash() but this 
> breaks because entries can be added to the table during heapwalk, where the 
> objects use marking.  The starting markWord is saved and restored.  Adding a 
> hashcode during this operation makes restoring the former markWord (locked, 
> inflated, etc) too complicated.  Plus we don't want all these objects to have 
> hashcodes because locking operations after tagging would have to always use 
> inflated locks.
> 
> Much of this change is to remove serial weak oop processing for the 
> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
> jvmti code.
> 
> It has also been tested with tier1-6.
> 
> Thank you to Stefan, Erik and Kim for their help with this change.

Coleen Phillimore has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains five commits:

 - Merge branch 'master' into jvmti-table
 - Merge branch 'master' into jvmti-table
 - More review comments from Stefan and ErikO
 - Code review comments from StefanK.
 - 8212879: Make JVMTI TagMap table not hash on oop address

-

Changes: https://git.openjdk.java.net/jdk/pull/967/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=967=03
  Stats: 1749 lines in 41 files changed: 627 ins; 1020 del; 102 mod
  Patch: https://git.openjdk.java.net/jdk/pull/967.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/967/head:pull/967

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8255801: Race when building ct.sym build tools

[Erik Joelsson]

On Tue, 3 Nov 2020 13:39:19 GMT, Magnus Ihse Bursie  wrote:

> There is a race when compiling build.tools.symbolgenerator.CreateSymbols:
> 
> [2020-11-03T07:21:12,118Z] Error: LinkageError occurred while loading main 
> class build.tools.symbolgenerator.CreateSymbols
> [2020-11-03T07:21:12,119Z] java.lang.ClassFormatError: Truncated class file
> [2020-11-03T07:21:12,125Z] Gendata.gmk:69: recipe for target 
> '/.../build/macosx-x64-open/support/javadoc-symbols/symbols' failed
> [2020-11-03T07:21:12,125Z] make[3]: *** 
> [/.../build/macosx-x64-open/support/javadoc-symbols/symbols] Error 1
> [2020-11-03T07:21:12,127Z] make/Main.gmk:148: recipe for target 
> 'jdk.javadoc-gendata' failed
> [2020-11-03T07:21:12,127Z] make[2]: *** [jdk.javadoc-gendata] Error 2
> [2020-11-03T07:21:12,127Z] make[2]: *** Waiting for unfinished jobs
> 
> This was caused by https://bugs.openjdk.java.net/browse/JDK-8216497, which 
> started compiling the same buildtool twic, in different, independent 
> makefiles, to the same output directory.
> 
> The long term correct solution is to fix 
> https://bugs.openjdk.java.net/browse/JDK-8241463.
> 
> For now, I opted to just make sure the output is placed in separate 
> directories to avoid the race. We will still compile the build tool twice; 
> it's annoying but not a major performance problem.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1037

Re: RFR: 8251549: Update docs on building for Git

[Erik Joelsson]

On Tue, 3 Nov 2020 13:21:24 GMT, Michael Keck 
 wrote:

>> @erikj79 
>> 
>> Ok, here is a completely rewritten portion, which highlights the differences 
>> between Cygwin git and Git for Windows.
>> 
>> Suggestion:
>> 
>>   * You need to install a git client. You have two choices, Cygwin git or
>> Git for Windows. Unfortunately there are pros and cons with each 
>> choice.
>> 
>> * The Cygwin `git` client has no line ending issues and understands
>>   Cygwin paths (which are used throughout the JDK build system).
>>   However, it does not currently work well with the Skara CLI 
>> tooling.
>>   Please see the [Skara wiki on Git clients](
>>   https://wiki.openjdk.java.net/display/SKARA/Skara#Skara-Git) for
>>   up-to-date information about the Skara git client support.
>> 
>> * The [Git for Windows](https://gitforwindows.org) client has issues
>>   with line endings, and do not understand Cygwin paths. It does work
>>   well with the Skara CLI tooling, however. To alleviate the line 
>> ending
>>   problems, make sure you set `core.autocrlf` to `false` (this is 
>> asked
>>   during installation).
>> 
>> @jddarcy Do you find this acceptable? This PR has been open for almost a 
>> month, and it's starting getting embarrassing that our build instruction 
>> does not match the new Github reality. :(
>
> You should add a `.gitattributes` file which defines the desired line endings 
> for each file type or for all files. Recommending a specific git client just 
> because of that (?) is unnecessary.
> 
> Here is a arbitrary example:
> https://github.com/alexkaratarakis/gitattributes/blob/f017637f08305dd1402fd025f6b648c45f04f499/Common.gitattributes#L49

I like your proposed text, Magnus.

-

PR: https://git.openjdk.java.net/jdk/pull/21

RFR: 8255801: Race when building ct.sym build tools

[Magnus Ihse Bursie]

There is a race when compiling build.tools.symbolgenerator.CreateSymbols:

[2020-11-03T07:21:12,118Z] Error: LinkageError occurred while loading main 
class build.tools.symbolgenerator.CreateSymbols
[2020-11-03T07:21:12,119Z] java.lang.ClassFormatError: Truncated class file
[2020-11-03T07:21:12,125Z] Gendata.gmk:69: recipe for target 
'/.../build/macosx-x64-open/support/javadoc-symbols/symbols' failed
[2020-11-03T07:21:12,125Z] make[3]: *** 
[/.../build/macosx-x64-open/support/javadoc-symbols/symbols] Error 1
[2020-11-03T07:21:12,127Z] make/Main.gmk:148: recipe for target 
'jdk.javadoc-gendata' failed
[2020-11-03T07:21:12,127Z] make[2]: *** [jdk.javadoc-gendata] Error 2
[2020-11-03T07:21:12,127Z] make[2]: *** Waiting for unfinished jobs

This was caused by https://bugs.openjdk.java.net/browse/JDK-8216497, which 
started compiling the same buildtool twic, in different, independent makefiles, 
to the same output directory.

The long term correct solution is to fix 
https://bugs.openjdk.java.net/browse/JDK-8241463.

For now, I opted to just make sure the output is placed in separate directories 
to avoid the race. We will still compile the build tool twice; it's annoying 
but not a major performance problem.

-

Commit messages:
 - 8255801: Race when building ct.sym build tools

Changes: https://git.openjdk.java.net/jdk/pull/1037/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1037=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255801
  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1037.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1037/head:pull/1037

PR: https://git.openjdk.java.net/jdk/pull/1037

Re: RFR: 8255798: Remove dead headless code in CompileJavaModules.gmk

[Erik Joelsson]

On Tue, 3 Nov 2020 08:21:22 GMT, Magnus Ihse Bursie  wrote:

> The variable BUILD_HEADLESS_ONLY is no longer set. And sun/applet does not 
> exist anymore.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1031

Re: RFR: 8251549: Update docs on building for Git

[Michael Keck]

On Tue, 3 Nov 2020 11:41:20 GMT, Magnus Ihse Bursie  wrote:

>> I would recommend putting in a link to the instructions on the Skara wiki 
>> page. They actually recommend Git for Windows and I have switched too. The 
>> main reason is to be able to use the token manager. To just clone and build, 
>> using Cygwin Git is definitely easier though.
>
> @erikj79 
> 
> Ok, here is a completely rewritten portion, which highlights the differences 
> between Cygwin git and Git for Windows.
> 
> Suggestion:
> 
>   * You need to install a git client. You have two choices, Cygwin git or
> Git for Windows. Unfortunately there are pros and cons with each 
> choice.
> 
> * The Cygwin `git` client has no line ending issues and understands
>   Cygwin paths (which are used throughout the JDK build system).
>   However, it does not currently work well with the Skara CLI tooling.
>   Please see the [Skara wiki on Git clients](
>   https://wiki.openjdk.java.net/display/SKARA/Skara#Skara-Git) for
>   up-to-date information about the Skara git client support.
> 
> * The [Git for Windows](https://gitforwindows.org) client has issues
>   with line endings, and do not understand Cygwin paths. It does work
>   well with the Skara CLI tooling, however. To alleviate the line 
> ending
>   problems, make sure you set `core.autocrlf` to `false` (this is 
> asked
>   during installation).
> 
> @jddarcy Do you find this acceptable? This PR has been open for almost a 
> month, and it's starting getting embarrassing that our build instruction does 
> not match the new Github reality. :(

You should add a `.gitattributes` file which defines the desired line endings 
for each file type or for all files. Recommending a specific git client just 
because of that (?) is unnecessary.

Here is a arbitrary example:
https://github.com/alexkaratarakis/gitattributes/blob/f017637f08305dd1402fd025f6b648c45f04f499/Common.gitattributes#L49

-

PR: https://git.openjdk.java.net/jdk/pull/21

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Erik Österlund]

On Tue, 3 Nov 2020 12:58:22 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More review comments from Stefan and ErikO

Marked as reviewed by eosterlund (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Coleen Phillimore]

> This change turns the HashTable that JVMTI uses for object tagging into a 
> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
> table to follow oops and then to rehash the table, this table points to 
> WeakHandle.  GC walks the backing OopStorages concurrently.
> 
> The hash function for the table is a hash of the lower 32 bits of the 
> address.  A flag is set during GC (gc_notification if in a safepoint, and 
> through a call to JvmtiTagMap::needs_processing()) so that the table is 
> rehashed at the next use.
> 
> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
> to post ObjectFree events.  In concurrent GCs there can be a window of time 
> between weak oop marking where the oop is unmarked, so dead (the phantom load 
> in peek returns NULL) but the gc_notification hasn't been done yet.  In this 
> window, a heap walk or GetObjectsWithTags call would not find an object 
> before the ObjectFree event is posted.  This is dealt with in two ways:
> 
> 1. In the Heap walk, there's an unconditional table walk to post events if 
> events are needed to post.
> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
> required, we use the VM thread to post the event.
> 
> Event posting cannot be done in a JavaThread because the posting needs to be 
> done while holding the table lock, so that the JvmtiEnv state doesn't change 
> before posting is done.  ObjectFree callbacks are limited in what they can do 
> as per the JVMTI Specification.  The allowed callbacks to the VM already have 
> code to allow NonJava threads.
> 
> To avoid rehashing, I also tried to use object->identity_hash() but this 
> breaks because entries can be added to the table during heapwalk, where the 
> objects use marking.  The starting markWord is saved and restored.  Adding a 
> hashcode during this operation makes restoring the former markWord (locked, 
> inflated, etc) too complicated.  Plus we don't want all these objects to have 
> hashcodes because locking operations after tagging would have to always use 
> inflated locks.
> 
> Much of this change is to remove serial weak oop processing for the 
> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
> jvmti code.
> 
> It has also been tested with tier1-6.
> 
> Thank you to Stefan, Erik and Kim for their help with this change.

Coleen Phillimore has updated the pull request incrementally with one 
additional commit since the last revision:

  More review comments from Stefan and ErikO

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/967/files
  - new: https://git.openjdk.java.net/jdk/pull/967/files/cb4c83e0..eeaf9aed

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=967=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=967=01-02

  Stats: 18 lines in 7 files changed: 3 ins; 6 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/967.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/967/head:pull/967

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Coleen Phillimore]

On Tue, 3 Nov 2020 10:33:16 GMT, Stefan Karlsson  wrote:

>> Since I went back and forth about what this function did (it posted events 
>> at one time), I thought the generic _processing name would be better.  GC 
>> callers shouldn't really have to know what processing we're doing here.  
>> Hopefully it won't change from rehashing.  That's why I like processing.
>
>>  GC callers shouldn't really have to know what processing we're doing here.
> 
> I completely disagree with this. It's extremely important that the GC and 
> Runtime code agrees on what this code does and where the GC *must* call it. 
> Knowing the details allows us to skip calling this after mark end, but forces 
> us to call it in relocate start, when objects should start to move. Though, I 
> don't want to block this review because of this point, so if you still thinks 
> that a non-descriptive name is better then we can argue that separately.

Ok, I'll rename it to needs_hashing.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Coleen Phillimore]

On Tue, 3 Nov 2020 10:36:21 GMT, Stefan Karlsson  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code review comments from StefanK.
>
> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 185:
> 
>> 183: // Serially remove unused oops from the table, and notify jvmti.
>> 184: void JvmtiTagMapTable::unlink_and_post(JvmtiEnv* env) {
>> 185: 
> 
> Stray newline

that's a useful newline.

> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 258:
> 
>> 256:   int rehash_len = moved_entries.length();
>> 257:   // Now add back in the entries that were removed.
>> 258:   for (int i = 0; i < moved_entries.length(); i++) {
> 
> rehash_len is read, but not used in for loop condition.

It's in the logging message below.  I'll use it in the loop too.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Coleen Phillimore]

On Tue, 3 Nov 2020 10:22:09 GMT, Erik Österlund  wrote:

>> Ok, will I run afoul of the ZGC people putting the parameter name after the 
>> parameter and the rest of the code, it is before?
>
> ZGC people passionately place the comment after the argument value.

I see that but in the non-zgc code it's the opposite and this is non-zgc code.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8251549: Update docs on building for Git

[Magnus Ihse Bursie]

On Mon, 26 Oct 2020 12:50:59 GMT, Erik Joelsson  wrote:

>> @jddarcy Recommended new wording:
>> Suggestion:
>> 
>>   * Clone the JDK repository using the Cygwin command line `git` client
>> as instructed in this document.
>> 
>> Using the Cygwin `git` client is the recommended way since it
>> guarantees that there will be no line ending issues and it works well
>> with other Cygwin tools. It is however also possible to use [Git for
>> Windows](https://gitforwindows.org). If you chose this path, make 
>> sure
>> to set `core.autocrlf` to `false` (this is asked during 
>> installation).
>
> I would recommend putting in a link to the instructions on the Skara wiki 
> page. They actually recommend Git for Windows and I have switched too. The 
> main reason is to be able to use the token manager. To just clone and build, 
> using Cygwin Git is definitely easier though.

@erikj79 

Ok, here is a completely rewritten portion, which highlights the differences 
between Cygwin git and Git for Windows.

Suggestion:

  * You need to install a git client. You have two choices, Cygwin git or
Git for Windows. Unfortunately there are pros and cons with each choice.

* The Cygwin `git` client has no line ending issues and understands
  Cygwin paths (which are used throughout the JDK build system).
  However, it does not currently work well with the Skara CLI tooling.
  Please see the [Skara wiki on Git clients](
  https://wiki.openjdk.java.net/display/SKARA/Skara#Skara-Git) for
  up-to-date information about the Skara git client support.

* The [Git for Windows](https://gitforwindows.org) client has issues
  with line endings, and do not understand Cygwin paths. It does work
  well with the Skara CLI tooling, however. To alleviate the line ending
  problems, make sure you set `core.autocrlf` to `false` (this is asked
  during installation).

@jddarcy Do you find this acceptable? This PR has been open for almost a month, 
and it's starting getting embarrassing that our build instruction does not 
match the new Github reality. :(

-

PR: https://git.openjdk.java.net/jdk/pull/21

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Stefan Karlsson]

On Mon, 2 Nov 2020 15:58:15 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comments from StefanK.

Some more nit-picking to make the code more consistent.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 52:

> 50:   : Hashtable(_table_size, 
> sizeof(JvmtiTagMapEntry)) {}
> 51: 
> 52: 

Double whitespace

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 185:

> 183: // Serially remove unused oops from the table, and notify jvmti.
> 184: void JvmtiTagMapTable::unlink_and_post(JvmtiEnv* env) {
> 185: 

Stray newline

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 224:

> 222: // Rehash oops in the table
> 223: void JvmtiTagMapTable::rehash() {
> 224: 

Stray newline

src/hotspot/share/prims/jvmtiTagMapTable.hpp line 75:

> 73: 
> 74:   void resize_if_needed();
> 75: public:

Newline between

src/hotspot/share/prims/jvmtiTagMapTable.hpp line 100:

> 98: };
> 99: 
> 100: 

Double newline

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 258:

> 256:   int rehash_len = moved_entries.length();
> 257:   // Now add back in the entries that were removed.
> 258:   for (int i = 0; i < moved_entries.length(); i++) {

rehash_len is read, but not used in for loop condition.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 165:

> 163:   }
> 164: }
> 165: const int _resize_load_trigger = 5;   // load factor that will 
> trigger the resize

Newline between

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Stefan Karlsson]

On Mon, 2 Nov 2020 12:58:49 GMT, Coleen Phillimore  wrote:

>  GC callers shouldn't really have to know what processing we're doing here.

I completely disagree with this. It's extremely important that the GC and 
Runtime code agrees on what this code does and where the GC *must* call it. 
Knowing the details allows us to skip calling this after mark end, but forces 
us to call it in relocate start, when objects should start to move. Though, I 
don't want to block this review because of this point, so if you still thinks 
that a non-descriptive name is better then we can argue that separately.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Erik Österlund]

On Mon, 2 Nov 2020 16:22:57 GMT, Coleen Phillimore  wrote:

>> src/hotspot/share/prims/jvmtiTagMap.cpp line 345:
>> 
>>> 343: 
>>> 344:   // Check if we have to process for concurrent GCs.
>>> 345:   check_hashmap(false);
>> 
>> Maybe add a comment stating the parameter name, as was done in other 
>> callsites for check_hashmap.
>
> Ok, will I run afoul of the ZGC people putting the parameter name after the 
> parameter and the rest of the code, it is before?

ZGC people passionately place the comment after the argument value.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]

[Andrew Haley]

On 02/11/2020 18:37, Bernhard Urban-Forster wrote:
> @theRealAph what gcc version?
> 
> I can reproduce with
> $ gcc --version
> gcc (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008
> which ships in Ubuntu 19.10 as default

My mistake: I think it only triggers with a debug build, because assert
is a macro.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8255798: Remove dead headless code in CompileJavaModules.gmk

[Aleksey Shipilev]

On Tue, 3 Nov 2020 08:21:22 GMT, Magnus Ihse Bursie  wrote:

> The variable BUILD_HEADLESS_ONLY is no longer set. And sun/applet does not 
> exist anymore.

Looks fine and trivial.

-

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1031

RFR: 8255798: Remove dead headless code in CompileJavaModules.gmk

[Magnus Ihse Bursie]

The variable BUILD_HEADLESS_ONLY is no longer set. And sun/applet does not 
exist anymore.

-

Commit messages:
 - 8255798: Remove dead headless code in CompileJavaModules.gmk

Changes: https://git.openjdk.java.net/jdk/pull/1031/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1031=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255798
  Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1031.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1031/head:pull/1031

PR: https://git.openjdk.java.net/jdk/pull/1031