Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v7]
> Finally returning to this review that was started in April 2020. I've > recast it as a github PR. I think the security concern raised by Gil > has been adequately answered. > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html > > Please review a new function: java.lang.ref.Reference.refersTo. > > This function is needed to test the referent of a Reference object without > artificially extending the lifetime of the referent object, as may happen > when calling Reference.get. Some garbage collectors require extending the > lifetime of a weak referent when accessed, in order to maintain collector > invariants. Lifetime extension may occur with any collector when the > Reference is a SoftReference, as calling get indicates recent access. This > new function also allows testing the referent of a PhantomReference, which > can't be accessed by calling get. > > The new function uses native methods whose implementations are in the VM so > they can use the Access API. It is the intent that these methods will be > intrinsified by optimizing compilers like C2 or graal, but that hasn't been > implemented yet. Bear that in mind before rushing off to change existing > uses of Reference.get. > > There are two native methods involved, one in Reference and an override in > PhantomReference, both package private in java.lang.ref. The reason for this > split is to simplify the intrinsification. This is a change from the version > from April 2020; that version had a single native method in Reference, > implemented using the ON_UNKNOWN_OOP_REF Access reference strength category. > However, adding support for that category in the compilers adds significant > implementation effort and complexity. Splitting avoids that complexity. > > Testing: > mach5 tier1 > Locally (linux-x64) verified the new test passes with various garbage > collectors. Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision: - Merge branch 'master' into refersto - improve wording in refersTo javadoc - Merge branch 'master' into refersto - More explicit refersTo0 comment. - simplify test - cleanup nits from Mandy - use Object instead of TestObject - improve refersTo0 descriptions - basic functional test - change referent access - ... and 3 more: https://git.openjdk.java.net/jdk/compare/f06d7348...79277ff3 - Changes: - all: https://git.openjdk.java.net/jdk/pull/498/files - new: https://git.openjdk.java.net/jdk/pull/498/files/3a15b6a9..79277ff3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=498&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=498&range=05-06 Stats: 90837 lines in 1555 files changed: 63919 ins; 19502 del; 7416 mod Patch: https://git.openjdk.java.net/jdk/pull/498.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/498/head:pull/498 PR: https://git.openjdk.java.net/jdk/pull/498
Integrated: 8188055: (ref) Add Reference::refersTo predicate
On Sun, 4 Oct 2020 03:59:59 GMT, Kim Barrett wrote: > Finally returning to this review that was started in April 2020. I've > recast it as a github PR. I think the security concern raised by Gil > has been adequately answered. > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html > https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html > > Please review a new function: java.lang.ref.Reference.refersTo. > > This function is needed to test the referent of a Reference object without > artificially extending the lifetime of the referent object, as may happen > when calling Reference.get. Some garbage collectors require extending the > lifetime of a weak referent when accessed, in order to maintain collector > invariants. Lifetime extension may occur with any collector when the > Reference is a SoftReference, as calling get indicates recent access. This > new function also allows testing the referent of a PhantomReference, which > can't be accessed by calling get. > > The new function uses native methods whose implementations are in the VM so > they can use the Access API. It is the intent that these methods will be > intrinsified by optimizing compilers like C2 or graal, but that hasn't been > implemented yet. Bear that in mind before rushing off to change existing > uses of Reference.get. > > There are two native methods involved, one in Reference and an override in > PhantomReference, both package private in java.lang.ref. The reason for this > split is to simplify the intrinsification. This is a change from the version > from April 2020; that version had a single native method in Reference, > implemented using the ON_UNKNOWN_OOP_REF Access reference strength category. > However, adding support for that category in the compilers adds significant > implementation effort and complexity. Splitting avoids that complexity. > > Testing: > mach5 tier1 > Locally (linux-x64) verified the new test passes with various garbage > collectors. This pull request has now been integrated. Changeset: 6023f6b1 Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/6023f6b1 Stats: 501 lines in 13 files changed: 488 ins; 0 del; 13 mod 8188055: (ref) Add Reference::refersTo predicate Reviewed-by: mchung, pliden, rriggs, dholmes, ihse, smarks, alanb - PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]
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 Yup, using `jlong.h` looks much better. - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 28 Oct 2020 15:56:48 GMT, Alan Bateman wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> improve wording in refersTo javadoc > > The API looks good, thanks for getting this in. Hello! As an IDE developer, I'm thinking about IDE inspection that may suggest the new method. My idea is to suggest replacing every `ref.get() == obj` with `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() == obj` could be preferred? What do you think? - PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]
On Wed, 4 Nov 2020 07:41:39 GMT, Kim Barrett wrote:
>>>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.
>
>> 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.
>
> Enums without any named enumerators are still meaningful types. More so with
> scoped enums, but still with unscoped enums.
> > 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.
I think the deletion stopped at the wrong place; it either went too far, or not
far enough.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 21:31:35 GMT, Coleen Phillimore wrote:
>> 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?
I didn't say it "doesn't work for shenandoah", I said it wouldn't have worked
with the old shenandoah barriers without additional work, like adding calls to
resolve. I understand the design intent of notifying the table management that
its hash codes are out of date. And the num-dead callback isn't the right
place, since there are num-dead callback invocations that aren't associated
with hash code invalidation. (It's not a correctness wrong, it's a "these
things are unrelated and this causes unnecessary work" wrong.)
>> 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.
I know of nothing that leads to "presumably during GC" being a requirement.
Having all pending events of some type occur before that type of event is
disabled seems like a reasonable requirement, but just means that event
disabling also requires the table to be "up to date", in the sense that any
GC-cleared entries need to be dealt with. That can be handled just like other
operations that use the table contents, rather than during the GC. That is,
use post_dead_object_on_vm_thread if there are or might be any pending dead
objects, before disabling the event.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]
On Tue, 3 Nov 2020 23:38:08 GMT, Coleen Phillimore wrote:
>> 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.
> 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.
Enums without any named enumerators are still meaningful types. More so with
scoped enums, but still with unscoped enums.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]
On Tue, 3 Nov 2020 20:12:35 GMT, Magnus Ihse Bursie wrote: >> 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.) Sorry, but I'm neither workin on AIX any more nor do I have access to an AIX machine :) Pulling in @TheRealMDoerr or @tstuefe - PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Wed, 4 Nov 2020 07:52:12 GMT, Kim Barrett wrote: >> 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? > > I didn't say it "doesn't work for shenandoah", I said it wouldn't have worked > with the old shenandoah barriers without additional work, like adding calls > to resolve. I understand the design intent of notifying the table management > that its hash codes are out of date. And the num-dead callback isn't the > right place, since there are num-dead callback invocations that aren't > associated with hash code invalidation. (It's not a correctness wrong, it's > a "these things are unrelated and this causes unnecessary work" wrong.) It used to be that jvmti tagmap processing was all-in-one (in GC weak reference processing, with the weak clearing, dead table entry removal, and rehashing all done in one pass. This change has split that up, with the weak clearing happening in a different place (still as part of the GC's weak reference processing) than the others (which I claim can now be part of the mutator, whether further separated or not). "Concurrent GC" has nothing to do with whether tagmaps need rehashing. Any copying collector needs to do so. A non-copying collector (whether concurrent or not) would not. (We don't have any of those in HotSpot today.) And weak reference clearing (whether concurrent or not) has nothing to do with whether objects have been moved and so the hashing has been invalidated. There's also a "well known" issue with address-based hashing and generational or similar collectors, where a simple boolean "objects have moved" flag can be problematic, and which tagmaps seem likely to be prone to. The old do_weak_oops tries to mitigate it by recognizing when the object didn't move. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v5]
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.
src/hotspot/share/prims/jvmtiTagMapTable.hpp line 36:
> 34: class JvmtiTagMapEntryClosure;
> 35:
> 36: class JvmtiTagMapEntry : public HashtableEntry mtServiceability> {
By using utilities/hashtable this buys into having to use HashtableEntry, which
includes the _hash member, even though that value is trivially computed from
the key (since we're using address-based hashing here). This costs an
additional 8 bytes (_LP64) per entry (a 25% increase) compared to the old
JvmtiTagHashmapEntry. (I think it doesn't currently make a difference on
!_LP64 because of poorly chosen layout in the old code, but fixing that would
make the difference 33%).
It seems like it should not have been hard to replace the oop _object member in
the old code with a WeakHandle while otherwise maintaining the Entry interface,
allowing much of the rest of the code to remain the same or similar and not
incurring this additional space cost.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 4 Nov 2020 09:31:13 GMT, Tagir F. Valeev wrote: >> The API looks good, thanks for getting this in. > > Hello! > > As an IDE developer, I'm thinking about IDE inspection that may suggest the > new method. My idea is to suggest replacing every `ref.get() == obj` with > `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() > == obj` could be preferred? What do you think? Thanks to a whole host of folks for reviews and comments. - PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
On Tue, 3 Nov 2020 21:40:39 GMT, Coleen Phillimore wrote: >> 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 &etc 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. For the GCs that call the num_dead notification in a pause it is much worse than what we had. As I pointed out elsewhere, it used to be that tagmap processing was all-in-one, as a single serial subtask taken by the first thread that reached it in WeakProcessor processing. Other threads would find that subtask taken and move on to processing oopstores in parallel with the tagmap processing. Now everything except the oopstorage-based clearing of dead entries is a single threaded serial task done by the VMThread, after all the parallel WeakProcessor work is done, because that's where the num-dead callbacks are invoked. WeakProcessor's parallel oopstorage processing doesn't have a way to do the num-dead callbacks by the last thread out of each parallel oopstorage processing. Instead it's left to the end, on the assumption that the callbacks are relatively cheap. But that could still be much worse than the old code, since the tagmap oopstorage could be late in the order of processing, an d so still effectively be a serial subtask after all the parallel subtasks are done or mostly done. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]
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 Builds on AIX. - PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 4 Nov 2020 09:31:13 GMT, Tagir F. Valeev wrote: > Hello! > > As an IDE developer, I'm thinking about IDE inspection that may suggest the > new method. My idea is to suggest replacing every `ref.get() == obj` with > `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() > == obj` could be preferred? What do you think? Those have different behaviors when ref's class overrides get. Sometimes that might be intentional (PhantomReference, where get blocks access to the referent, and SoftReference, where get may update heuristics for recent accesses delaying GC clearing). But if some further subclass overrides get for some reason, such a change might not be appropriate. - PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Mon, 2 Nov 2020 18:39:59 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 46 commits: >> >> - Removing trailing whitespace. >> - Merging master into JDK-8250768. >> - Updating tests after records are a final feature. >> - Fixing tests. >> - Finalizing removal of record preview hooks. >> - Merging master into JDK-8250768 >> - Reflecting review comments. >> - Merge branch 'master' into JDK-8250768 >> - Removing unnecessary cast. >> - Using a more correct way to get URLs. >> - ... and 36 more: >> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 > > src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTaskPool.java > line 257: > >> 255: //when patching modules (esp. java.base), it may be >> impossible to >> 256: //clear the symbols read from the patch path: >> 257: polluted |= >> get(JavaFileManager.class).hasLocation(StandardLocation.PATCH_MODULE_PATH); > > OK, but looks unrelated to primary work The tests need to inject preview APIs into compilation, and without this tweak, the incorrect values are re-used for tests that use the javac pool. So it is related in the sense it is needed for the tests to pass. - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Mon, 2 Nov 2020 19:37:39 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 46 commits: >> >> - Removing trailing whitespace. >> - Merging master into JDK-8250768. >> - Updating tests after records are a final feature. >> - Fixing tests. >> - Finalizing removal of record preview hooks. >> - Merging master into JDK-8250768 >> - Reflecting review comments. >> - Merge branch 'master' into JDK-8250768 >> - Removing unnecessary cast. >> - Using a more correct way to get URLs. >> - ... and 36 more: >> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/builders/AnnotationTypeRequiredMemberBuilder.java > line 156: > >> 154: buildSignature(annotationDocTree); >> 155: buildDeprecationInfo(annotationDocTree); >> 156: buildPreviewInfo(annotationDocTree); > > (Just checking) I presume this behavior is inherited into > `AnnotationTypeOptionalMemberBuilder` so no changes required there. Right, I think so. - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Mon, 2 Nov 2020 19:41:08 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 46 commits: >> >> - Removing trailing whitespace. >> - Merging master into JDK-8250768. >> - Updating tests after records are a final feature. >> - Fixing tests. >> - Finalizing removal of record preview hooks. >> - Merging master into JDK-8250768 >> - Reflecting review comments. >> - Merge branch 'master' into JDK-8250768 >> - Removing unnecessary cast. >> - Using a more correct way to get URLs. >> - ... and 36 more: >> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties > line 213: > >> 211: doclet.Preview=Preview. >> 212: doclet.Properties=Properties >> 213: doclet.constructors=constructors > > Is the period after `Preview` intentional? It seems inconsistent. This is consistent with doclet.Deprecated key a few lines up. In the javadoc it then says something along these lines: Preview. A binding pattern tree - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Mon, 2 Nov 2020 19:59:10 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 46 commits: >> >> - Removing trailing whitespace. >> - Merging master into JDK-8250768. >> - Updating tests after records are a final feature. >> - Fixing tests. >> - Finalizing removal of record preview hooks. >> - Merging master into JDK-8250768 >> - Reflecting review comments. >> - Merge branch 'master' into JDK-8250768 >> - Removing unnecessary cast. >> - Using a more correct way to get URLs. >> - ... and 36 more: >> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 > > test/langtools/jdk/javadoc/doclet/testPreview/api/preview/Core.java line 28: > >> 26: import jdk.internal.javac.PreviewFeature.Feature; >> 27: >> 28: @PreviewFeature(feature=Feature.TEST) > > Yeah, I remember `Feature.TEST` from earlier. I guess it's OK for now, as a > workaround for a testing a feature which is inherently, by design, a moving > target across releases. > > These days, javadoc tests are trending towards generating small sample test > programs, instead of having small static side-files dominated by a legal > header. I wonder if there is a possibility of having a "generator class" in > the `javadoc.tester` package that can generate sample code using one or more > of the current set of preview features, as a way of reducing the need for the > TEST feature. I have intentionally added Feature.TEST to improve testability. Before, tests were using one of the constants (typically whatever was the first constant), but that seems somewhat problematic - what if (at some point, transiently) we have no preview features? - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v5]
On Wed, 4 Nov 2020 05:37:00 GMT, Serguei Spitsyn wrote:
>> 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 = Thread::current()->
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v6]
> 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 two additional commits since the last revision: - Add back WeakProcessorPhases::Phase enum. - Serguei 1. - Changes: - all: https://git.openjdk.java.net/jdk/pull/967/files - new: https://git.openjdk.java.net/jdk/pull/967/files/f66ea839..7d3fdf68 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=967&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=967&range=04-05 Stats: 18 lines in 5 files changed: 7 ins; 0 del; 11 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
RFR: 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
Current submit workflow omits `hs_err*.log` and `replay*.log` files, because zip matches the paths, and so only the hs_errs/replays in the current dir are picked up (and there are none). It should be preceded with `*` to match files in subdirs. Incidentally, this is why `*.jtr` match works. Observe: $ mkdir 1/2 -p $ touch 1/2/hs_err_213123.log $ zip -r9 test.zip . -i hs_err* zip warning: zip file empty $ zip -r9 test.zip . -i */hs_err* adding: 1/2/hs_err_213123.log (stored 0%) This was introduced in the initial implementation in JDK-8255352. Additional testing: - [x] Observing current Windows failures package hs_err into archive - Commit messages: - 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch Changes: https://git.openjdk.java.net/jdk/pull/1055/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1055&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255895 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/1055.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1055/head:pull/1055 PR: https://git.openjdk.java.net/jdk/pull/1055
Re: RFR: 8253892: Disable misleading-indentation on clang as well as gcc
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. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1044
Re: RFR: 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
On Wed, 4 Nov 2020 12:59:24 GMT, Aleksey Shipilev wrote: > Current submit workflow omits `hs_err*.log` and `replay*.log` files, because > zip matches the paths, and so only the hs_errs/replays in the current dir are > picked up (and there are none). It should be preceded with `*` to match files > in subdirs. Incidentally, this is why `*.jtr` match works. > > Observe: > > $ mkdir 1/2 -p > $ touch 1/2/hs_err_213123.log > $ zip -r9 test.zip . -i hs_err* > zip warning: zip file empty > $ zip -r9 test.zip . -i */hs_err* > adding: 1/2/hs_err_213123.log (stored 0%) > > This was introduced in the initial implementation in JDK-8255352. > > Additional testing: > - [x] Observing current Windows failures package hs_err into archive Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1055
Re: RFR: 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
On Wed, 4 Nov 2020 13:40:49 GMT, Erik Joelsson wrote: >> Current submit workflow omits `hs_err*.log` and `replay*.log` files, because >> zip matches the paths, and so only the hs_errs/replays in the current dir >> are picked up (and there are none). It should be preceded with `*` to match >> files in subdirs. Incidentally, this is why `*.jtr` match works. >> >> Observe: >> >> $ mkdir 1/2 -p >> $ touch 1/2/hs_err_213123.log >> $ zip -r9 test.zip . -i hs_err* >> zip warning: zip file empty >> $ zip -r9 test.zip . -i */hs_err* >> adding: 1/2/hs_err_213123.log (stored 0%) >> >> This was introduced in the initial implementation in JDK-8255352. >> >> Additional testing: >> - [x] Observing current Windows failures package hs_err into archive > > Marked as reviewed by erikj (Reviewer). Thank you, @erikj79. Trivial? (I would like to get is soon, in order to capture more hs_errs for current failures) - PR: https://git.openjdk.java.net/jdk/pull/1055
Re: RFR: 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
On Wed, 4 Nov 2020 13:43:58 GMT, Aleksey Shipilev wrote: >> Marked as reviewed by erikj (Reviewer). > > Thank you, @erikj79. Trivial? (I would like to get is soon, in order to > capture more hs_errs for current failures) I'm fine with a quick integration of this. - PR: https://git.openjdk.java.net/jdk/pull/1055
Re: RFR: 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
On Wed, 4 Nov 2020 14:38:53 GMT, Erik Joelsson wrote: >> Thank you, @erikj79. Trivial? (I would like to get is soon, in order to >> capture more hs_errs for current failures) > > I'm fine with a quick integration of this. Thank you. - PR: https://git.openjdk.java.net/jdk/pull/1055
Integrated: 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
On Wed, 4 Nov 2020 12:59:24 GMT, Aleksey Shipilev wrote: > Current submit workflow omits `hs_err*.log` and `replay*.log` files, because > zip matches the paths, and so only the hs_errs/replays in the current dir are > picked up (and there are none). It should be preceded with `*` to match files > in subdirs. Incidentally, this is why `*.jtr` match works. > > Observe: > > $ mkdir 1/2 -p > $ touch 1/2/hs_err_213123.log > $ zip -r9 test.zip . -i hs_err* > zip warning: zip file empty > $ zip -r9 test.zip . -i */hs_err* > adding: 1/2/hs_err_213123.log (stored 0%) > > This was introduced in the initial implementation in JDK-8255352. > > Additional testing: > - [x] Observing current Windows failures package hs_err into archive This pull request has now been integrated. Changeset: 61c92470 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/61c92470 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod 8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/1055
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
On Thu, 22 Oct 2020 02:40:44 GMT, Jonathan Gibbons wrote:
> This introduces support for a new `@spec` tag that can be used as either an
> inline tag or as a block tag. It is used to identify references to external
> specifications, in such a way that the references can be collected together
> on a new summary page, called "Other Specifications", available from either
> the static INDEX page or the interactive search box.
>
> As an inline tag, the format is `{@spec url label}`, which is roughly
> translated to `label` in the generated docs.
>
> As a block tag, the format is simply
>
> @spec url label
>
> which is handled in a manner analogous to
>
> @see label
>
> The tag is notable for being the first standard/supported tag that can be
> used as either an inline or block tag. (We have had support for bimodal
> non-standard/custom tags for a while, such as `{@jls}` and `{@jvms}`.) To
> support bimodal standard tags, some changes to `DocCommentParser` are
> incorporated here.
>
> This change is only the _support_ for the new tag; it does _not_ include any
> changes to API docs to _use_ the new tag.
It's very instructive to see a new tag introduced from scratch with all the
accompanying infrastructure and tests. The changes looks good to me, but I need
to stress than I'm not an expert on some of the affected areas such as the
jdk.compiler parts.
-
Marked as reviewed by hannesw (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/790
Integrated: 8253892: Disable misleading-indentation on clang as well as gcc
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. This pull request has now been integrated. Changeset: 7dcaba63 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/7dcaba63 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod 8253892: Disable misleading-indentation on clang as well as gcc Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/1044
RFR: 8255822: Zero: improve build-time JVMTI handling
Current Zero interpreter has the optimization for JVMTI support. It recognizes that JVMTI is disabled most of the time, and that JVMTI checks in the interpreter code slows it down considerably. (I measured it myself for this patch, it gives about 20% hit in build times). Current optimization works as follows. At build time, an XSLT transform is performed on `bytecodeInterpreter.cpp`, yielding `bytecodeInterpreterWithChecks.cpp`. In that new compilation unit, `VM_JVMTI` macro is defined, and a new entry point -- `BytecodeInterpreter::withChecks` -- is defined. Then, both compilation units are compiled. In one of them, `JVMTI` hooks are stripped out. In another, they persist. Then, callers have to choose which entry point to use. I believe this can be rewritten to use C++ templates instead of XLST and defines dance. This also allows to clean up JVMTI checks a bit. Additional testing: - [x] Linux x86_64 Zero fastdebug build with `-jvmti` - [x] Linux x86_64 Zero fastdebug/release build times are not regressing - Commit messages: - Revert one dubious change - 8255822: Zero: improve build-time JVMTI handling Changes: https://git.openjdk.java.net/jdk/pull/1061/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1061&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255822 Stats: 155 lines in 6 files changed: 3 ins; 118 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/1061.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1061/head:pull/1061 PR: https://git.openjdk.java.net/jdk/pull/1061
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 4 Nov 2020 10:13:59 GMT, Kim Barrett wrote: >> Hello! >> >> As an IDE developer, I'm thinking about IDE inspection that may suggest the >> new method. My idea is to suggest replacing every `ref.get() == obj` with >> `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() >> == obj` could be preferred? What do you think? > >> Hello! >> >> As an IDE developer, I'm thinking about IDE inspection that may suggest the >> new method. My idea is to suggest replacing every `ref.get() == obj` with >> `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() >> == obj` could be preferred? What do you think? > > Those have different behaviors when ref's class overrides get. Sometimes > that might be intentional (PhantomReference, where get blocks access to the > referent, and SoftReference, where get may update heuristics for recent > accesses delaying GC clearing). But if some further subclass overrides get > for some reason, such a change might not be appropriate. Checking if a reference has been cleared i.e. `ref.get() == null` or `ref.get() != null` may benefit with IDE giving a hint. - PR: https://git.openjdk.java.net/jdk/pull/498
Integrated: 8255128: linux x86 build failure with libJNIPoint.c
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 This pull request has now been integrated. Changeset: 804bd725 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/804bd725 Stats: 17 lines in 2 files changed: 1 ins; 6 del; 10 mod 8255128: linux x86 build failure with libJNIPoint.c Reviewed-by: coleenp, shade, ihse - PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v6]
> JDK-8252870: Finalize (remove "incubator" from) jpackage Andy Herrick has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge branch 'master' into JDK-8252870 - Merge branch 'master' into JDK-8252870 - Merge branch master into JDK-8252870 - Merge branch 'master' into JDK-8252870 - JDK-8252870: Finalize (remove "incubator" from) jpackage - reverting two auto-generated files, and changing module-info to "@since 16" - JDK-8252870: Finalize (remove "incubator" from) jpackage Merge branch 'finalize' into JDK-8252870 - 8252869 Finalize (remove incubator from) jpackage (implementation) - Changes: https://git.openjdk.java.net/jdk/pull/633/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=633&range=05 Stats: 1733 lines in 259 files changed: 692 ins; 701 del; 340 mod Patch: https://git.openjdk.java.net/jdk/pull/633.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/633/head:pull/633 PR: https://git.openjdk.java.net/jdk/pull/633
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v7]
> This is an update to javac and javadoc, to introduce support for Preview > APIs, and generally improve javac and javadoc behavior to more closely adhere > to JEP 12. > > The notable changes are: > > * adding support for Preview APIs (javac until now supported primarily only > preview language features, and APIs associated with preview language > features). This includes: > * the @PreviewFeature annotation has boolean attribute "reflective", > which should be true for reflective Preview APIs, false otherwise. This > replaces the existing "essentialAPI" attribute with roughly inverted meaning. > * the preview warnings for preview APIs are auto-suppressed as described > in the JEP 12. E.g. the module that declares the preview API is free to use > it without warnings > * improving error/warning messages. Please see [1] for a list of > cases/examples. > * class files are only marked as preview if they are using a preview > feature. [1] also shows if a class file is marked as preview or not. > * the PreviewFeature annotation has been moved to jdk.internal.javac package > (originally was in the jdk.internal package). > * Preview API in JDK's javadoc no longer needs to specify @preview tag in > the source files. javadoc will auto-generate a note for @PreviewFeature > elements, see e.g. [2] and [3] (non-reflective and reflective API, > respectively). A summary of preview elements is also provided [4]. Existing > uses of @preview have been updated/removed. > * non-JDK javadoc is also enhanced to auto-generate preview notes for uses > of Preview elements, and for declaring elements using preview language > features [5]. > > Please also see the CSR [6] for more information. > > [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html > [2] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html > [3] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html > [4] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html > [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/ > [6] https://bugs.openjdk.java.net/browse/JDK-8250769 Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 49 commits: - Cleanup - removing unnecessary code. - Merging master into JDK-8250768-dev4 - Reflecting review comments. - Removing trailing whitespace. - Merging master into JDK-8250768. - Updating tests after records are a final feature. - Fixing tests. - Finalizing removal of record preview hooks. - Merging master into JDK-8250768 - Reflecting review comments. - ... and 39 more: https://git.openjdk.java.net/jdk/compare/7f4d873d...e4b02827 - Changes: https://git.openjdk.java.net/jdk/pull/703/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=703&range=06 Stats: 3719 lines in 148 files changed: 2701 ins; 730 del; 288 mod Patch: https://git.openjdk.java.net/jdk/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703 PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Mon, 2 Nov 2020 20:21:44 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 46 commits: >> >> - Removing trailing whitespace. >> - Merging master into JDK-8250768. >> - Updating tests after records are a final feature. >> - Fixing tests. >> - Finalizing removal of record preview hooks. >> - Merging master into JDK-8250768 >> - Reflecting review comments. >> - Merge branch 'master' into JDK-8250768 >> - Removing unnecessary cast. >> - Using a more correct way to get URLs. >> - ... and 36 more: >> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 > > I have read all the files. > > I have added a n umber of various minor non-blocking comments (no need for > re-review( to fix these. But I have a couple of comments/questions before > finally giving approval. > There's a comment in `PreviewListWriter` about annotation members that needs > too be addressed, and I wonder is RECORD and RECORD_COMPONENT need to be > added into PreviewElementKind. Thanks @jonathan-gibbons for your comments! I've tried to update the code based on them, mostly in https://github.com/lahodaj/jdk/commit/743f516c660b577035cdda4510a0bb97937fd9b2 and https://github.com/lahodaj/jdk/commit/e4b02827998fc2e8f19f983aabfb3d720b03d111 A big chunk of the update is generalization of the deprecated and preview list builders and writers into a "summary" list builder and writer. These should also now handle records. For record components, those are a little tricky, as (AFAIK) can't currently have deprecation/preview-ness for them (and hence there is no good way to test any support for record components in these). But the summary build and writer are looking for record components and will fail in case a record component is sent into them. - PR: https://git.openjdk.java.net/jdk/pull/703
Integrated: JDK-8252870: Finalize (remove "incubator" from) jpackage
On Tue, 13 Oct 2020 12:51:54 GMT, Andy Herrick wrote: > JDK-8252870: Finalize (remove "incubator" from) jpackage This pull request has now been integrated. Changeset: 26e7ef78 Author:Andy Herrick URL: https://git.openjdk.java.net/jdk/commit/26e7ef78 Stats: 1733 lines in 259 files changed: 692 ins; 701 del; 340 mod 8252870: Finalize (remove "incubator" from) jpackage Reviewed-by: kcr, erikj, almatvee, asemenyuk, prr, ihse - PR: https://git.openjdk.java.net/jdk/pull/633
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec) [v2]
> This introduces support for a new `@spec` tag that can be used as either an
> inline tag or as a block tag. It is used to identify references to external
> specifications, in such a way that the references can be collected together
> on a new summary page, called "Other Specifications", available from either
> the static INDEX page or the interactive search box.
>
> As an inline tag, the format is `{@spec url label}`, which is roughly
> translated to `label` in the generated docs.
>
> As a block tag, the format is simply
>
> @spec url label
>
> which is handled in a manner analogous to
>
> @see label
>
> The tag is notable for being the first standard/supported tag that can be
> used as either an inline or block tag. (We have had support for bimodal
> non-standard/custom tags for a while, such as `{@jls}` and `{@jvms}`.) To
> support bimodal standard tags, some changes to `DocCommentParser` are
> incorporated here.
>
> This change is only the _support_ for the new tag; it does _not_ include any
> changes to API docs to _use_ the new tag.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 11 commits:
- Fix merge issues; review feedback
- Merge with master
- allow rich content in createAnchorAndSearchIndex
- update Docs.gmk to stop disabling spec tag
- fix TestSpecTag.testEncodedURI
- fix tests
- remove support to workaround legacy @spec tag
- Merge remote-tracking branch 'upstream/master' into new-spec-tag
- fix trailing whitespace in test
- temporarily allow existing legacy usage `@spec JPMS` `@spec jsr-51`
- ... and 1 more: https://git.openjdk.java.net/jdk/compare/804bd725...ed5512d9
-
Changes: https://git.openjdk.java.net/jdk/pull/790/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=790&range=01
Stats: 1374 lines in 42 files changed: 1337 ins; 14 del; 23 mod
Patch: https://git.openjdk.java.net/jdk/pull/790.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/790/head:pull/790
PR: https://git.openjdk.java.net/jdk/pull/790
New lead for the JDK 7u Project: Andrew Brygin
Hello Andrew Haley resigned as JDK 7u project lead [1]. Andrew Brygin offered to take over leadership of JDK 7u. [2] I have no other nominations. Under the bylaws for Project Roles [3], a new Project Lead may be nominated by the Group Leads of the Project's sponsoring groups. The Build Infrastructure Group is sponsor of the project. As Group Leader [4], I would like to nominate Andrew Brygin [5] to be the new Project Lead. The Build Infrastructure Group is the only sponsoring group for this project. The nomination is automatically approved. Tim Bell [1] https://mail.openjdk.java.net/pipermail/jdk7u-dev/2020-August/011040.html [2] https://mail.openjdk.java.net/pipermail/jdk7u-dev/2020-September/011041.html [3] https://openjdk.java.net/bylaws#project-lead [4] https://openjdk.java.net/census#build [5] http://openjdk.java.net/census#bae
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v6]
On Wed, 4 Nov 2020 12:21: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 incrementally with two > additional commits since the last revision: > > - Add back WeakProcessorPhases::Phase enum. > - Serguei 1. Thank you for the update, Coleen! I leave it for you to decide to refactor the gc_notification or not. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8255822: Zero: improve build-time JVMTI handling [v2]
> Current Zero interpreter has the optimization for JVMTI support. It > recognizes that JVMTI is disabled most of the time, and that JVMTI checks in > the interpreter code slows it down considerably. (I measured it myself for > this patch, it gives about 20% hit in build times). > > Current optimization works as follows. At build time, an XSLT transform is > performed on `bytecodeInterpreter.cpp`, yielding > `bytecodeInterpreterWithChecks.cpp`. In that new compilation unit, `VM_JVMTI` > macro is defined, and a new entry point -- `BytecodeInterpreter::withChecks` > -- is defined. Then, both compilation units are compiled. In one of them, > `JVMTI` hooks are stripped out. In another, they persist. Then, callers have > to choose which entry point to use. > > I believe this can be rewritten to use C++ templates instead of XLST and > defines dance. This also allows to clean up JVMTI checks a bit. > > Additional testing: > - [x] Linux x86_64 Zero fastdebug build with `-jvmti` > - [x] Linux x86_64 Zero fastdebug/release build times are not regressing Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Fix build error - Merge branch 'master' into JDK-8255822-zero-jvmti-rework - Revert one dubious change - 8255822: Zero: improve build-time JVMTI handling Summary: use C++ templates instead of XSLT transforms - Changes: https://git.openjdk.java.net/jdk/pull/1061/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1061&range=01 Stats: 156 lines in 6 files changed: 3 ins; 119 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/1061.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1061/head:pull/1061 PR: https://git.openjdk.java.net/jdk/pull/1061
Re: RFR: 8255822: Zero: improve build-time JVMTI handling [v2]
On Thu, 5 Nov 2020 06:39:02 GMT, Aleksey Shipilev wrote: >> Current Zero interpreter has the optimization for JVMTI support. It >> recognizes that JVMTI is disabled most of the time, and that JVMTI checks in >> the interpreter code slows it down considerably. (I measured it myself for >> this patch, it gives about 20% hit in build times). >> >> Current optimization works as follows. At build time, an XSLT transform is >> performed on `bytecodeInterpreter.cpp`, yielding >> `bytecodeInterpreterWithChecks.cpp`. In that new compilation unit, >> `VM_JVMTI` macro is defined, and a new entry point -- >> `BytecodeInterpreter::withChecks` -- is defined. Then, both compilation >> units are compiled. In one of them, `JVMTI` hooks are stripped out. In >> another, they persist. Then, callers have to choose which entry point to use. >> >> I believe this can be rewritten to use C++ templates instead of XLST and >> defines dance. This also allows to clean up JVMTI checks a bit. >> >> Additional testing: >> - [x] Linux x86_64 Zero fastdebug build with `-jvmti` >> - [x] Linux x86_64 Zero fastdebug/release build times are not regressing > > Aleksey Shipilev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Fix build error > - Merge branch 'master' into JDK-8255822-zero-jvmti-rework > - Revert one dubious change > - 8255822: Zero: improve build-time JVMTI handling >Summary: use C++ templates instead of XSLT transforms This looks like a good cleanup to me - far simpler! Cheers, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1061
Re: RFR: 8255822: Zero: improve build-time JVMTI handling [v2]
On Thu, 5 Nov 2020 06:39:02 GMT, Aleksey Shipilev wrote: >> Current Zero interpreter has the optimization for JVMTI support. It >> recognizes that JVMTI is disabled most of the time, and that JVMTI checks in >> the interpreter code slows it down considerably. (I measured it myself when >> working on this patch: removing this optimization yields about 20% hit in >> build times). >> >> Current optimization works as follows. At build time, an XSLT transform is >> performed on `bytecodeInterpreter.cpp`, yielding >> `bytecodeInterpreterWithChecks.cpp`. In that new compilation unit, >> `VM_JVMTI` macro is defined, and a new entry point -- >> `BytecodeInterpreter::withChecks` -- is defined. Then, both compilation >> units are compiled. In one of them, `JVMTI` hooks are stripped out. In >> another, they persist. Then, callers have to choose which entry point to use. >> >> I believe this can be rewritten to use C++ templates instead of XLST and >> defines dance. This also allows to clean up JVMTI checks a bit. >> >> Additional testing: >> - [x] Linux x86_64 Zero fastdebug build with `-jvmti` >> - [x] Linux x86_64 Zero fastdebug/release build times are not regressing > > Aleksey Shipilev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Fix build error > - Merge branch 'master' into JDK-8255822-zero-jvmti-rework > - Revert one dubious change > - 8255822: Zero: improve build-time JVMTI handling >Summary: use C++ templates instead of XSLT transforms Looks good to me. Thanks for the cleanup! - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1061
