Re: RFR: 8279047: Remove expired flags in JDK 20
On Fri, 10 Jun 2022 13:23:51 GMT, David Holmes wrote: > Expired Flags in 20: > > - FilterSpuriousWakeups > - MinInliningThreshold > - PrefetchFieldsAhead > > No remaining usages in code or tests. > > - UseHeavyMonitors (expired in PRODUCT ONLY) > > As this flag now only exists for debug builds it has to be a "develop" flag > rather than product. There are then changes to two tests that use this flag, > so that they only run on a debug VM. > > - test/jdk/java/lang/Thread/virtual/HoldsLock.java > > The second @run that uses UseHeavyMonitors is moved to a second test segment > that only applies to the debug VM. > > - test/jdk/java/util/concurrent/ConcurrentHashMap/MapLoops.java > > Change the test section that uses UseHeavyMonitors to only run on a debug VM > and remove the IgnoreUnrecognizedOptions flag. Note that the existing test > logic would run the same test twice on product builds. > > No documentation in the manpage exists for any of the newly expired flags. > > Obsoleted flags in 20: > > - ExtendedDTraceProbes > > Documented in manpage so moved from Deprecated section to Obsolete section. > There is special handling for messages about use of this flag so that won't > be updated until the flag is actually obsoleted (JDK-8279913). > > - UseContainerCpuShares > - PreferContainerQuotaForCPUCount > - AliasLevel > > Undocumented. > > Java manpage updates: > - Updates Java version to 20 > - Moved ExtendedDTraceProbe info as previously mentioned > - Applied changes from the .1 file that had not been applied to the markdown > source so that they were not lost (and note the .1 file was also missing > changes from the markdown file that had not been propagated). > - Removed an unrepresentable character (the section symbol) that was not > being generated into either the html or nroff file correctly Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.org/jdk/pull/9127
Re: RFR: 8287363: null pointer should use NULL instead of 0
On Sat, 28 May 2022 03:31:30 GMT, Yasumasa Suenaga wrote: > We found using `0` as `NULL` in java_md_common.c . `0` is not a pointer, so > we should use `NULL` where we want to handle it. > > https://github.com/openjdk/jdk/pull/8646#discussion_r882294076 > > Also I found using `0` as NUL char in java_md_common.c , so I replaced it to > `\0` in this PR. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8931
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v10]
On Thu, 26 May 2022 10:55:28 GMT, Yasumasa Suenaga wrote: >> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 >> on Fedora 36. >> As you can see, the warnings spreads several areas. Let me know if I should >> separate them by area. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Fix comments Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v9]
On Wed, 25 May 2022 09:16:43 GMT, Yasumasa Suenaga wrote:
>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should
>> separate them by area.
>>
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> *
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>>
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool*
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Change Array::data() implementation
> - Avoid stringop-overflow warning in jfrTraceIdBits.inline.hpp
Mostly good, but I missed a problem with an earlier part of the change. Sorry
I didn't notice sooner.
src/java.base/unix/native/libjli/java_md_common.c line 133:
> 131:
> 132: snprintf_result = JLI_Snprintf(name, sizeof(name), "%s%c%s", indir,
> FILE_SEPARATOR, cmd);
> 133: if ((snprintf_result < 0) && (snprintf_result >= (int)sizeof(name)))
> {
That should be `||` rather than `&&`.
src/java.base/unix/native/libjli/java_md_common.c line 135:
> 133: if ((snprintf_result < 0) && (snprintf_result >= (int)sizeof(name)))
> {
> 134: return 0;
> 135: }
Pre-existing: It seems odd that this returns `0` above and below, rather than
returning `NULL`. I think there are one or two other places in this file that
are similarly using literal `0` for a null pointer, though others are using
`NULL`. Something to report and clean up separately from this change.
-
Changes requested by kbarrett (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Sun, 22 May 2022 16:45:11 GMT, Kim Barrett wrote:
>> It might be GCC bug...
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>>
>> This issue is for integer literal, but [Comment
>> 29](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c29) mentions address
>> calculation (e.g. `NULL + offset`) - it is similar the problem in
>> jfrTraceIdBits.inline.hp because `dest` comes from `low_addr()` (`addr +
>> low_offset`).
>
> I don't see the similarity. That gcc bug is about literals used as addresses,
> which get treated (suggested inappropriately) as NULL+offset, with NULL+offset
> being a cause of warnings. I don't see that happening in our case. That is,
> in our `addr + low_offset`, `addr` doesn't seem to be either a literal or NULL
> that I can see.
>
> It's hard for me to investigate this any further just by perusing the code, so
> I'm in the process of getting set up to build with gcc12.x. That will let me
> do some experiments. It may take me a few days to get to that point though.
I spent some time looking into this one. I agree there is a false positive
here, and there doesn't seem to be a better solution than suppressing the
warning. I would prefer the change below, rather than what's proposed. Also
eliminate the changes to utilities/compilerWarnings files. This is a very
gcc-specific issue; there's no reason to generalize the solution. The reason
for relocating the suppression is to be able to describe the issue in more
detail in a context where that description makes sense.
template
inline void JfrTraceIdBits::store(jbyte bits, const T* ptr) {
assert(ptr != NULL, "invariant");
// gcc12 warns "writing 1 byte into a region of size 0" when T == Klass.
// The warning seems to be a false positive. And there is no warning for
// other types that use the same mechanisms. The warning also sometimes
// goes away with minor code perturbations, such as replacing function calls
// with equivalent code directly inlined.
PRAGMA_DIAG_PUSH
PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
set(bits, traceid_tag_byte(ptr));
PRAGMA_DIAG_POP
}
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v8]
On Sun, 22 May 2022 08:40:28 GMT, Yasumasa Suenaga wrote:
>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should
>> separate them by area.
>>
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> *
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>>
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool*
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 11 commits:
>
> - Merge remote-tracking branch 'upstream/master' into gcc12-warnings
> - Use getter function to access "_data"
> - Revert changes for bytecodeAssembler.cpp, classFileParser.cpp,
> symbolTable.cpp
> - revert changes for memnode.cpp and type.cpp
> - Add assert to check the range of BasicType
> - Merge remote-tracking branch 'upstream/master' into HEAD
> - Revert change for java.c , parse_manifest.c , LinuxPackage.c
> - Calculate char offset before realloc()
> - Use return value from JLI_Snprintf
> - Avoid pragma error in before GCC 12
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/c156bcc5...042c1c70
Changes requested by kbarrett (Reviewer).
src/hotspot/share/oops/array.hpp line 102:
> 100: // standard operations
> 101: int length() const { return _length; }
> 102: T* data() const { return
> reinterpret_cast(reinterpret_cast(this) +
> base_offset_in_bytes()); }
Adding the const-qualifier to the `data()` function and then implicitly
casting it away (by casting through intptr_t) is wrong. Either don't
const-qualify (and leave it to some future use that needs such to address
appropriately), or have two functions. Also, the line length is excessive.
So this:
T* data() {
return reinterpret_cast(
reinterpret_cast(this) + base_offset_in_bytes());
}
and optionally add this:
const T* data() const {
return reinterpret_cast(
reinterpret_cast(this) + base_offset_in_bytes());
}
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Sun, 22 May 2022 08:45:48 GMT, Yasumasa Suenaga wrote: >> I don't think this warning has anything to do with that NULL check. But I'm >> still not understanding what it is warning about. The "region of size 0" part >> of the warning message seems important, but I'm not (yet?) seeing how it >> could >> be coming up with that. The code involved here is pretty contorted... > > It might be GCC bug... > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578 > > This issue is for integer literal, but [Comment > 29](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c29) mentions address > calculation (e.g. `NULL + offset`) - it is similar the problem in > jfrTraceIdBits.inline.hp because `dest` comes from `low_addr()` (`addr + > low_offset`). I don't see the similarity. That gcc bug is about literals used as addresses, which get treated (suggested inappropriately) as NULL+offset, with NULL+offset being a cause of warnings. I don't see that happening in our case. That is, in our `addr + low_offset`, `addr` doesn't seem to be either a literal or NULL that I can see. It's hard for me to investigate this any further just by perusing the code, so I'm in the process of getting set up to build with gcc12.x. That will let me do some experiments. It may take me a few days to get to that point though. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Sun, 22 May 2022 08:35:54 GMT, Yasumasa Suenaga wrote:
>> `Array::_data` is a pseudo flexible array member. "Pseudo" because C++
>> doesn't have flexible array members. The compiler is completely justified in
>> complaining about the apparently out-of-bounds accesses.
>>
>> There is a "well-known" (though moderately ugly) approach to doing flexible
>> array members in C++. Something like this:
>>
>>
>> T* data() {
>> return reinterpret_cast(
>> reinterpret_cast(this) + data_offset());
>> }
>>
>>
>> where `data_offset()` is new and private:
>>
>>
>> static size_t data_offset() {
>> return offset_of(Array, _data);
>> }
>>
>>
>> Use `data()` everywhere instead of using `_data` directly.
>>
>> There are other places in HotSpot that use this kind of approach.
>
> Thanks @kimbarrett for your advice! Warnings from array.hpp have gone with
> your suggestion.
Good. I see you found and used the existing `base_offset_in_bytes` (which I'd
overlooked), rather than using my suggested `data_offset`.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Tue, 17 May 2022 12:43:02 GMT, Yasumasa Suenaga wrote: >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> line 103: >> >>> 101: PRAGMA_STRINGOP_OVERFLOW_IGNORED >>> 102: *dest = op(bits, *dest); >>> 103: PRAGMA_DIAG_POP >> >> I see no stringop here. I'm still trying to understand what the compiler is >> complaining about. > > I guess GCC cannot understand `assert(dest != NULL` immediately preceding it. > > > In file included from > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdLoadBarrier.inline.hpp:33, > from > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:30, > from > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:30: > In function 'void set_form(jbyte, jbyte*) [with jbyte (* op)(jbyte, jbyte) = > traceid_or]', > inlined from 'void set(jbyte, jbyte*)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:129:23, > inlined from 'static void JfrTraceIdBits::store(jbyte, const T*) [with T > = Klass]' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:135:6, > inlined from 'static void JfrTraceId::tag_as_jdk_jfr_event(const Klass*)' > at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:106:3, > inlined from 'static void JdkJfrEvent::tag_as(const Klass*)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:176:35: I don't think this warning has anything to do with that NULL check. But I'm still not understanding what it is warning about. The "region of size 0" part of the warning message seems important, but I'm not (yet?) seeing how it could be coming up with that. The code involved here is pretty contorted... - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Tue, 17 May 2022 12:38:55 GMT, Yasumasa Suenaga wrote:
>> src/hotspot/share/classfile/classFileParser.cpp line 5970:
>>
>>> 5968: PRAGMA_STRINGOP_OVERFLOW_IGNORED
>>> 5969: _cp->symbol_at_put(hidden_index, _class_name);
>>> 5970: PRAGMA_DIAG_POP
>>
>> I don't understand these warning suppressions for symbol_at_put (here and
>> elsewhere). I don't see any stringops here. What is the compiler
>> complaining about? (There's no mention of classfile stuff in the review
>> cover message.)
>
> Like the others, it is caused by `Array::at_put()`.
>
>
> In file included from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/annotations.hpp:28,
> from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/instanceKlass.hpp:29,
> from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/javaClasses.hpp:30,
> from
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/precompiled/precompiled.hpp:35:
> In member function 'void Array::at_put(int, const T&) [with T = unsigned
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::symbol_at_put(int, Symbol*)' at
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:362:15,
> inlined from 'void
> ClassFileParser::mangle_hidden_class_name(InstanceKlass*)' at
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/classFileParser.cpp:5966:21:
`Array::_data` is a pseudo flexible array member. "Pseudo" because C++
doesn't have flexible array members. The compiler is completely justified in
complaining about the apparently out-of-bounds accesses.
There is a "well-known" (though moderately ugly) approach to doing flexible
array members in C++. Something like this:
T* data() {
return reinterpret_cast(
reinterpret_cast(this) + data_offset());
}
where `data_offset()` is new and private:
static size_t data_offset() {
return offset_of(Array, _data);
}
Use `data()` everywhere instead of using `_data` directly.
There are other places in HotSpot that use this kind of approach.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Tue, 17 May 2022 03:05:09 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert change for java.c , parse_manifest.c , LinuxPackage.c > > src/hotspot/share/opto/memnode.cpp line 1413: > >> 1411:bt == T_BYTE|| bt == T_SHORT || >> 1412:bt == T_INT || bt == T_LONG, "wrong type = %s", >> type2name(bt)); >> 1413: PRAGMA_DIAG_POP > > The problem here is the definition of type2name, which returns NULL for an > unknown BasicType. It would probably be better if it returned a recognizable > string for that situation, eliminating this warning at it's source. While looking at type2name, I noticed the comment for the immediately preceding type2name_tab is wrong. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Fri, 13 May 2022 10:02:30 GMT, Yasumasa Suenaga wrote:
>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should
>> separate them by area.
>>
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> *
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>>
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool*
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Revert change for java.c , parse_manifest.c , LinuxPackage.c
Some suggestions for code changes instead of warning suppression. And some
warnings that I don't yet understand and don't think should be suppressed
without more details or investigation.
src/hotspot/share/classfile/bytecodeAssembler.cpp line 95:
> 93: ShouldNotReachHere();
> 94: }
> 95: PRAGMA_DIAG_POP
One of these is another of the symbol_at_put cases that I don't understand.
Are there other cases in the switch that warn? And if so, which ones and how?
There are no details of this one in the PR cover description.
src/hotspot/share/classfile/classFileParser.cpp line 5970:
> 5968: PRAGMA_STRINGOP_OVERFLOW_IGNORED
> 5969: _cp->symbol_at_put(hidden_index, _class_name);
> 5970: PRAGMA_DIAG_POP
I don't understand these warning suppressions for symbol_at_put (here and
elsewhere). I don't see any stringops here. What is the compiler complaining
about? (There's no mention of classfile stuff in the review cover message.)
src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
line 103:
> 101: PRAGMA_STRINGOP_OVERFLOW_IGNORED
> 102: *dest = op(bits, *dest);
> 103: PRAGMA_DIAG_POP
I see no stringop here. I'm still trying to understand what the compiler is
complaining about.
src/hotspot/share/opto/memnode.cpp line 1413:
> 1411:bt == T_BYTE|| bt == T_SHORT ||
> 1412:bt == T_INT || bt == T_LONG, "wrong type = %s",
> type2name(bt));
> 1413: PRAGMA_DIAG_POP
The problem here is the definition of type2name, which returns NULL for an
unknown BasicType. It would probably be better if it returned a recognizable
string for that situation, eliminating this warning at it's source.
src/hotspot/share/opto/type.cpp line 4300:
> 4298: PRAGMA_FORMAT_OVERFLOW_IGNORED
> 4299: fatal("not an element type: %s", type2name(etype));
> 4300: PRAGMA_DIAG_POP
Another occurrence of type2name returning NULL for unknown BasicType.
-
Changes requested by kbarrett (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]
On Thu, 12 May 2022 18:28:02 GMT, Kim Barrett wrote: >> Thanks for all to review this PR! I think we should separate this issue as >> following: >> >> * Suppress warnings >> * make/modules/java.desktop/lib/Awt2dLibraries.gmk >> * src/hotspot/share/classfile/bytecodeAssembler.cpp >> * src/hotspot/share/classfile/classFileParser.cpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> * src/hotspot/share/opto/memnode.cpp >> * src/hotspot/share/opto/type.cpp >> * src/hotspot/share/utilities/compilerWarnings.hpp >> * src/hotspot/share/utilities/compilerWarnings_gcc.hpp >> * src/java.base/unix/native/libjli/java_md_common.c >> * Bug fixes >> * src/java.base/share/native/libjli/java.c >> * src/java.base/share/native/libjli/parse_manifest.c >> * src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c >> >> I want to include the change of Awt2dLibraries.gmk (HarfBuzz) in this PR >> because it is 3rd party library. >> >> I will separate in above if I do not hear any objections, and this issue >> (PR) handles "suppress warnings" only. > >> @YaSuenag From my PoV this sounds like a good suggestion. > > +1 > Can I get the review from HotSpot folks? @kimbarrett Already working on it. There are some I don't understand yet. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286705: GCC 12 reports use-after-free potential bugs
On Fri, 13 May 2022 09:14:28 GMT, Yasumasa Suenaga wrote: > GCC 12 reports use-after-free potential bugs in below: > > > In function 'find_positions', > inlined from 'find_file' at > /home/ysuenaga/github-forked/jdk/src/java.base/share/native/libjli/parse_manifest.c:364:9: Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8696
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]
On Thu, 12 May 2022 01:29:13 GMT, Yasumasa Suenaga wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Calculate char offset before realloc() > > Thanks for all to review this PR! I think we should separate this issue as > following: > > * Suppress warnings > * make/modules/java.desktop/lib/Awt2dLibraries.gmk > * src/hotspot/share/classfile/bytecodeAssembler.cpp > * src/hotspot/share/classfile/classFileParser.cpp > * > src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp > * src/hotspot/share/opto/memnode.cpp > * src/hotspot/share/opto/type.cpp > * src/hotspot/share/utilities/compilerWarnings.hpp > * src/hotspot/share/utilities/compilerWarnings_gcc.hpp > * src/java.base/unix/native/libjli/java_md_common.c > * Bug fixes > * src/java.base/share/native/libjli/java.c > * src/java.base/share/native/libjli/parse_manifest.c > * src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c > > I want to include the change of Awt2dLibraries.gmk (HarfBuzz) in this PR > because it is 3rd party library. > > I will separate in above if I do not hear any objections, and this issue (PR) > handles "suppress warnings" only. > @YaSuenag From my PoV this sounds like a good suggestion. +1 - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:56:44 GMT, Kim Barrett wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid pragma error in before GCC 12
>
> src/java.base/share/native/libjli/java.c line 1629:
>
>> 1627: const char *arg = jargv[i];
>> 1628: if (arg[0] == '-' && arg[1] == 'J') {
>> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg +
>> 2);
>
> Wow!
I wonder if the client expects NULL strings in the result, or if the NULL value
should be an empty string? If empty strings are okay, this would be simpler
without the conditional, just dup from arg + 2 to the terminating byte (which
might be immediate).
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 08:40:21 GMT, Yasumasa Suenaga wrote:
>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should
>> separate them by area.
>>
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> *
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>>
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool*
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Avoid pragma error in before GCC 12
Changes requested by kbarrett (Reviewer).
make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462:
> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits
> missing-field-initializers strict-aliasing
> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder delete-non-virtual-dtor
> strict-overflow \
> 462: maybe-uninitialized class-memaccess unused-result extra
> use-after-free
Globally disabling use-after-free warnings for this package seems really
questionable. If these are problems in the code, just suppressing the warning
and leaving the problems to bite us seems like a bad idea. And the problems
ought to be reported upstream to the HarfBuzz folks.
src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 51:
> 49:
> 50: #define PRAGMA_STRINGOP_OVERFLOW_IGNORED \
> 51: PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
Are the reported problems with format/stringop-overflow real? Or are they
false positives? If they are real then I'm not going to approve ignoring them,
unless there is some urgent reason and there are followup bug reports for
fixing them.
src/java.base/share/native/libjli/java.c line 1629:
> 1627: const char *arg = jargv[i];
> 1628: if (arg[0] == '-' && arg[1] == 'J') {
> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg + 2);
Wow!
src/java.base/unix/native/libjli/java_md_common.c line 135:
> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) return
> 0;
> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR,
> cmd);
> 135: #pragma GCC diagnostic pop
Wouldn't it be better to just call JLI_Snprintf without the precheck and check
the result to determine whether it was successful or was truncated? I think
that kind of check is supposed to make gcc's truncation checker happy.
src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c line 193:
> 191: #if defined(__GNUC__) && __GNUC__ >= 12
> 192: #pragma GCC diagnostic pop
> 193: #endif
Rather than all this warning suppression cruft and the comment explaining why
it's okay, just compute the `(strBufNextChar - strBufBegin)` offset a few
lines earlier, before the realloc.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
On Wed, 27 Apr 2022 15:32:35 GMT, Roger Riggs wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright, @bug list > > LGTM Thanks for reviews @RogerRiggs and @mlchung - PR: https://git.openjdk.java.net/jdk/pull/8418
Integrated: 8285690: CloneableReference subtest should not throw CloneNotSupportedException
On Wed, 27 Apr 2022 09:24:24 GMT, Kim Barrett wrote: > Please review this fix to test/jdk/java/lang/ref/ReferenceClone.java. It was > passing if CloneableReference::clone were to throw CloneNotSupportedException. > That should be a failure. > > Testing: > Locally (linux-x64) verified test still passes. Temporarily changed > CloneableReference::clone to throw and verified the expected failure gets > reported, unlike before. This pull request has now been integrated. Changeset: 2d8d1402 Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/2d8d1402147f6ddd15732ce7098a8438317a2681 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod 8285690: CloneableReference subtest should not throw CloneNotSupportedException Reviewed-by: rriggs, mchung - PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
On Wed, 27 Apr 2022 18:25:27 GMT, Mandy Chung wrote:
>> That was my initial thought, but it doesn't work - CNSE is a checked
>> exception so must be handled.
>
>> test() and main will need to declare this checked exception.
>
>
> diff --git a/test/jdk/java/lang/ref/ReferenceClone.java
> b/test/jdk/java/lang/ref/ReferenceClone.java
> index bd1ead81bec..2f9386b81e4 100644
> --- a/test/jdk/java/lang/ref/ReferenceClone.java
> +++ b/test/jdk/java/lang/ref/ReferenceClone.java
> @@ -31,12 +31,12 @@ import java.lang.ref.*;
>
> public class ReferenceClone {
> private static final ReferenceQueue QUEUE = new
> ReferenceQueue<>();
> -public static void main(String... args) {
> +public static void main(String... args) throws Exception {
> ReferenceClone refClone = new ReferenceClone();
> refClone.test();
> }
>
> -public void test() {
> +public void test() throws CloneNotSupportedException {
> // test Reference::clone that throws CNSE
> Object o = new Object();
> assertCloneNotSupported(new SoftRef(o));
> @@ -45,9 +45,7 @@ public class ReferenceClone {
>
> // Reference subclass may override the clone method
> CloneableReference ref = new CloneableReference(o);
> -try {
> ref.clone();
> -} catch (CloneNotSupportedException e) {}
> }
>
> private void assertCloneNotSupported(CloneableRef ref) {
> ```
Yes, that would work. But I'd rather keep the code for that subtest close
together, i.e. as currently written.
-
PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
On Wed, 27 Apr 2022 15:57:34 GMT, Mandy Chung wrote:
>> Kim Barrett has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update copyright, @bug list
>
> test/jdk/java/lang/ref/ReferenceClone.java line 52:
>
>> 50: } catch (CloneNotSupportedException e) {
>> 51: throw new RuntimeException("CloneableReference::clone should
>> not throw CloneNotSupportedException");
>> 52: }
>
> Alternatively, it could simply let CNSE propagate.
>
>
> CloneableReference ref = new CloneableReference(o);
> ref.clone();
>
>
> `test()` and `main` will need to declare this checked exception.
That was my initial thought, but it doesn't work - CNSE is a checked exception
so must be handled.
-
PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
> Please review this fix to test/jdk/java/lang/ref/ReferenceClone.java. It was > passing if CloneableReference::clone were to throw CloneNotSupportedException. > That should be a failure. > > Testing: > Locally (linux-x64) verified test still passes. Temporarily changed > CloneableReference::clone to throw and verified the expected failure gets > reported, unlike before. Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: update copyright, @bug list - Changes: - all: https://git.openjdk.java.net/jdk/pull/8418/files - new: https://git.openjdk.java.net/jdk/pull/8418/files/47043626..8f863628 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8418&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8418&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8418.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8418/head:pull/8418 PR: https://git.openjdk.java.net/jdk/pull/8418
RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException
Please review this fix to test/jdk/java/lang/ref/ReferenceClone.java. It was passing if CloneableReference::clone were to throw CloneNotSupportedException. That should be a failure. Testing: Locally (linux-x64) verified test still passes. Temporarily changed CloneableReference::clone to throw and verified the expected failure gets reported, unlike before. - Commit messages: - fail test if CloneableReference throws CloneNotSupportedException Changes: https://git.openjdk.java.net/jdk/pull/8418/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8418&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285690 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8418.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8418/head:pull/8418 PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: JDK-8283415: Update java.lang.ref to use sealed classes
On Sat, 19 Mar 2022 22:30:13 GMT, Joe Darcy wrote: > Another refactoring to use sealed classes where possible, this time in the > java.lang.ref package. > > Copyright dates will be updated, if needed, before a push. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7874
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Fri, 11 Mar 2022 23:38:10 GMT, Mikael Vidstedt wrote:
> Background, from JBS:
>
> src/java.base/share/native/libverify/check_code.c: In function
> 'read_all_code':
> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may
> be used uninitialized [-Werror=maybe-uninitialized]
> 942 | check_and_push(context, lengths, VM_MALLOC_BLK);
> | ^~~
> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument
> 2 of type 'const void *' to 'check_and_push' declared here
> 4145 | static void check_and_push(context_type *context, const void *ptr,
> int kind)
> | ^~
>
>
> Because the second argument of check_and_push is "const void*" GCC assumes
> that the malloc:ed data, which has not yet been initialized, will not be/can
> not be modified later which in turn suggests it may be used without ever
> being initialized.
>
> The same general issue was addressed in
> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably
> for GCC 11.1.
>
>
> Details:
>
> Instead of sprinkling more calloc calls around or using pragmas/gcc
> attributes I chose to change the check_and_push function to take a
> (non-const) void* argument, and provide a new wrapper function
> `check_and_push_const` which handles the const argument case. For the
> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a
> const conversion.
>
> To avoid having multiple ways of solving the same problem I also chose to
> revert the change made in JDK-8266168, reverting the calloc back to a malloc
> call.
>
> Testing:
>
> tier1 + builds-tier{2,3,4,5}
Changes requested by kbarrett (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Thu, 17 Mar 2022 08:08:07 GMT, Kim Barrett wrote:
>> Background, from JBS:
>>
>> src/java.base/share/native/libverify/check_code.c: In function
>> 'read_all_code':
>> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths'
>> may be used uninitialized [-Werror=maybe-uninitialized]
>> 942 | check_and_push(context, lengths, VM_MALLOC_BLK);
>> | ^~~
>> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument
>> 2 of type 'const void *' to 'check_and_push' declared here
>> 4145 | static void check_and_push(context_type *context, const void *ptr,
>> int kind)
>> | ^~
>>
>>
>> Because the second argument of check_and_push is "const void*" GCC assumes
>> that the malloc:ed data, which has not yet been initialized, will not be/can
>> not be modified later which in turn suggests it may be used without ever
>> being initialized.
>>
>> The same general issue was addressed in
>> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably
>> for GCC 11.1.
>>
>>
>> Details:
>>
>> Instead of sprinkling more calloc calls around or using pragmas/gcc
>> attributes I chose to change the check_and_push function to take a
>> (non-const) void* argument, and provide a new wrapper function
>> `check_and_push_const` which handles the const argument case. For the
>> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a
>> const conversion.
>>
>> To avoid having multiple ways of solving the same problem I also chose to
>> revert the change made in JDK-8266168, reverting the calloc back to a malloc
>> call.
>>
>> Testing:
>>
>> tier1 + builds-tier{2,3,4,5}
>
> src/java.base/share/native/libverify/check_code.c line 472:
>
>> 470:
>> 471: static void check_and_push(context_type *context, void *ptr, int kind);
>> 472: static void check_and_push_const(context_type *context, const void
>> *ptr, int kind);
>
> This really needs a comment explaining why two functions are needed.
I think it would be clearer to drop the kind argument and have two functions
check_and_push_vm_string(context_type*, const char*)
check_and_push_malloc_block(context_type*, void*)
with a shared helper that is given the appropriate kind.
-
PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Thu, 17 Mar 2022 06:59:01 GMT, David Holmes wrote:
>> Background, from JBS:
>>
>> src/java.base/share/native/libverify/check_code.c: In function
>> 'read_all_code':
>> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths'
>> may be used uninitialized [-Werror=maybe-uninitialized]
>> 942 | check_and_push(context, lengths, VM_MALLOC_BLK);
>> | ^~~
>> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument
>> 2 of type 'const void *' to 'check_and_push' declared here
>> 4145 | static void check_and_push(context_type *context, const void *ptr,
>> int kind)
>> | ^~
>>
>>
>> Because the second argument of check_and_push is "const void*" GCC assumes
>> that the malloc:ed data, which has not yet been initialized, will not be/can
>> not be modified later which in turn suggests it may be used without ever
>> being initialized.
>>
>> The same general issue was addressed in
>> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably
>> for GCC 11.1.
>>
>>
>> Details:
>>
>> Instead of sprinkling more calloc calls around or using pragmas/gcc
>> attributes I chose to change the check_and_push function to take a
>> (non-const) void* argument, and provide a new wrapper function
>> `check_and_push_const` which handles the const argument case. For the
>> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a
>> const conversion.
>>
>> To avoid having multiple ways of solving the same problem I also chose to
>> revert the change made in JDK-8266168, reverting the calloc back to a malloc
>> call.
>>
>> Testing:
>>
>> tier1 + builds-tier{2,3,4,5}
>
> src/java.base/share/native/libverify/check_code.c line 4168:
>
>> 4166: }
>> 4167:
>> 4168: static void check_and_push_const(context_type *context, const void
>> *ptr, int kind) {
>
> IIUC the only `const` cases all pass a `const char*` so perhaps we can make
> that explicit in the signature? I confess I can't figure out how the passed
> in memory eventually gets used and it seems bizarre that it could be a
> freshly malloc'd block or an existing string.
David - The check_and_push idiom checks if ptr is null, doing the out of memory
handling if so. Otherwise, add ptr to a collection for later release. The
kind determines how the release is done, either JVM_ReleaseUTF or free.
-
PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Fri, 11 Mar 2022 23:38:10 GMT, Mikael Vidstedt wrote:
> Background, from JBS:
>
> src/java.base/share/native/libverify/check_code.c: In function
> 'read_all_code':
> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may
> be used uninitialized [-Werror=maybe-uninitialized]
> 942 | check_and_push(context, lengths, VM_MALLOC_BLK);
> | ^~~
> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument
> 2 of type 'const void *' to 'check_and_push' declared here
> 4145 | static void check_and_push(context_type *context, const void *ptr,
> int kind)
> | ^~
>
>
> Because the second argument of check_and_push is "const void*" GCC assumes
> that the malloc:ed data, which has not yet been initialized, will not be/can
> not be modified later which in turn suggests it may be used without ever
> being initialized.
>
> The same general issue was addressed in
> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably
> for GCC 11.1.
>
>
> Details:
>
> Instead of sprinkling more calloc calls around or using pragmas/gcc
> attributes I chose to change the check_and_push function to take a
> (non-const) void* argument, and provide a new wrapper function
> `check_and_push_const` which handles the const argument case. For the
> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a
> const conversion.
>
> To avoid having multiple ways of solving the same problem I also chose to
> revert the change made in JDK-8266168, reverting the calloc back to a malloc
> call.
>
> Testing:
>
> tier1 + builds-tier{2,3,4,5}
Changes requested by kbarrett (Reviewer).
src/java.base/share/native/libverify/check_code.c line 472:
> 470:
> 471: static void check_and_push(context_type *context, void *ptr, int kind);
> 472: static void check_and_push_const(context_type *context, const void *ptr,
> int kind);
This really needs a comment explaining why two functions are needed.
-
PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8253495: CDS generates non-deterministic output [v6]
On Fri, 11 Mar 2022 06:55:23 GMT, Ioi Lam wrote: >> This patch makes the result of "java -Xshare:dump" deterministic: >> - Disabled new Java threads from launching. This is harmless. See comments >> in jvm.cpp >> - Fixed a problem in hashtable ordering in heapShared.cpp >> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random >> bits. Added code to zero it. >> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in >> make/scripts/compare.sh >> >> Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. >> This will be fixed in >> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828). >> >> Testing under way: >> - tier1~tier5 >> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, >> windows-x86-cmp-baseline, etc). > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > Added helper function CollectedHeap::zap_filler_array_with GC changes look good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v4]
On Fri, 11 Mar 2022 04:56:23 GMT, Ioi Lam wrote:
>> This patch makes the result of "java -Xshare:dump" deterministic:
>> - Disabled new Java threads from launching. This is harmless. See comments
>> in jvm.cpp
>> - Fixed a problem in hashtable ordering in heapShared.cpp
>> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random
>> bits. Added code to zero it.
>> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in
>> make/scripts/compare.sh
>>
>> Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic.
>> This will be fixed in
>> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
>>
>> Testing under way:
>> - tier1~tier5
>> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline,
>> windows-x86-cmp-baseline, etc).
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> zero GC heap filler arrays
Changes requested by kbarrett (Reviewer).
src/hotspot/share/gc/shared/collectedHeap.cpp line 449:
> 447: allocator.initialize(start);
> 448: DEBUG_ONLY(zap_filler_array(start, words, zap);)
> 449: if (DumpSharedSpaces) {
Probably shouldn't both zap and clear for dumping, to avoid wasting time.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: a quick question about String
> On Dec 24, 2021, at 2:46 PM, liangchenb...@gmail.com wrote: > >> Are you saying that new would always create a new object but the GC might >> merge multiple instances of String into a single instance? > > About jvm's string optimizations, jvm may make different string > objects with the same content share the backing array in order to > reduce allocation (like what the original poster alan wonders). This > optimization does not alter the identity of the relative string > objects and thus has zero effect on the user end. For HotSpot this is enabled by -XX:+UseStringDeduplication. For more details see https://openjdk.java.net/jeps/192 Note that as of JDK 18 all of the HotSpot collectors support this, not just G1.
Re: RFR: JDK-8276422 Add command-line option to disable finalization [v4]
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks wrote: >> Pretty much what it says. The new option controls a static member in >> InstanceKlass that's consulted to determine whether the finalization >> machinery is activated for instances when a class is loaded. A new native >> method is added so that this state can be queried from Java. This is used to >> control whether a finalizer thread is created and to disable the `System` >> and `Runtime::runFinalization` methods. Includes tests for the above. >> >> Adding an option to disable finalization is part of [JEP >> 421](https://openjdk.java.net/jeps/421). > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Remove Finalizer.Holder class. Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6442
Re: RFR: JDK-8276422 Add command-line option to disable finalization
On Thu, 18 Nov 2021 01:34:36 GMT, Stuart Marks wrote:
> Pretty much what it says. The new option controls a static member in
> InstanceKlass that's consulted to determine whether the finalization
> machinery is activated for instances when a class is loaded. A new native
> method is added so that this state can be queried from Java. This is used to
> control whether a finalizer thread is created and to disable the `System` and
> `Runtime::runFinalization` methods. Includes tests for the above.
I only really reviewed the hotspot changes.
There is nothing here to make the various GCs take advantage of finalization
being disabled. Is the plan to leave that to followup changes?
src/hotspot/share/oops/instanceKlass.hpp line 338:
> 336:
> 337: // Queries finalization state
> 338: static bool finalization_enabled() { return _finalization_enabled; }
Predicate functions like this are often named "is_xxx"; that idiom is common in
this class.
src/hotspot/share/prims/jvm.cpp line 694:
> 692:
> 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env))
> 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
missing indentation
-
Changes requested by kbarrett (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6442
Re: RFR: JDK-8276422 Add command-line option to disable finalization
On Thu, 18 Nov 2021 06:43:01 GMT, Kim Barrett wrote: >> Pretty much what it says. The new option controls a static member in >> InstanceKlass that's consulted to determine whether the finalization >> machinery is activated for instances when a class is loaded. A new native >> method is added so that this state can be queried from Java. This is used to >> control whether a finalizer thread is created and to disable the `System` >> and `Runtime::runFinalization` methods. Includes tests for the above. > > src/hotspot/share/prims/jvm.cpp line 694: > >> 692: >> 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env)) >> 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE; > > missing indentation I think this could just be `return InstanceKlass::finalization_enabled();`. There is lots of code in this file and elsewhere that assumes C++ `bool` converts to `jboolean` appropriately. - PR: https://git.openjdk.java.net/jdk/pull/6442
Re: RFR: 8274071: Clean up java.lang.ref comments and documentation
On Tue, 21 Sep 2021 12:05:27 GMT, Pavel Rappo wrote: > This PR fixes an inline comment typo and reduces "overlinking" in a doc > comment in `java.lang.ref.Reference`. Overlinking happens because the > `reachabilityFence` method: > * Links `package-summary.html#reachability` twice. > * Refers to JLS 12.6 twice: once from a block tag and once from an inline > tag. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5609
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used
On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden wrote: > While fixing JDK-8270626 it was discovered that the C2 intrinsic for > `Reference.refersTo()` is not used as often as one would think. Instead C2 > often prefers using the native implementation, which is much slower which > defeats the purpose of having an intrinsic in the first place. > > The problem arise from having `refersTo0()` be virtual, native and > @IntrinsicCandidate. This can be worked around by introducing an virtual > method layer and so that `refersTo0()` can be made `final`. That's what this > patch does, by adding a virtual `refersToImpl()` which in turn calls the now > final `refersTo0()`. > > It should be noted that `Object.clone()` and `Object.hashCode()` are also > virtual, native and @IntrinsicCandidate and these methods get special > treatment by C2 to get the intrinsic generated more optimally. We might want > to do a similar optimization for `Reference.refersTo0()`. In that case, it is > suggested that such an improvement could come later and be handled as a > separate enhancement, and @kimbarrett has already filed JDK-8272140 to track > that. > > Testing: > I found this hard to test without instrumenting Hotspot to tell me that the > intrinsic was called instead of the native method. So, for that reason > testing was ad-hoc, using an instrumented Hotspot in combination with the > test > (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) > that initially uncovered the problem. See JDK-8270626 for additional > information. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: JDK-8256844: Make NMT late-initializable
On Thu, 29 Jul 2021 22:53:42 GMT, Coleen Phillimore wrote: >> Short: this patch makes NMT available in custom-launcher scenarios and >> during gtests. It simplifies NMT initialization. It adds a lot of >> NMT-specific testing, cleans them up and makes them sideeffect-free. >> >> - >> >> NMT continues to be an extremely useful tool for SAP to tackle memory >> problems in the JVM. >> >> However, NMT is of limited use due to the following restrictions: >> >> - NMT cannot be used if the hotspot is embedded into a custom launcher >> unless the launcher actively cooperates. Just creating and invoking the JVM >> is not enough, it needs to do some steps prior to loading the hotspot. This >> limitation is not well known (nor, do I believe, documented). Many products >> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this >> problem limits NMT usefulness greatly since our VMs are often embedded into >> custom launchers and modifying every launcher is impossible. >> - Worse, if that custom launcher links the libjvm *statically* there is just >> no way to activate NMT at all. This is the reason NMT cannot be used in the >> `gtestlauncher`. >> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` >> and `-XX:Flags=`. >> - The fact that NMT cannot be used in gtests is really a pity since it would >> allow us to both test NMT itself more rigorously and check for memory leaks >> while testing other stuff. >> >> The reason for all this is that NMT initialization happens very early, on >> the first call to `os::malloc()`. And those calls happen already during >> dynamic C++ initialization - a long time before the VM gets around parsing >> arguments. So, regular VM argument parsing is too late to parse NMT >> arguments. >> >> The current solution is to pass NMT arguments via a specially prepared >> environment variable: `NMT_LEVEL_=`. That environment >> variable has to be set by the embedding launcher, before it loads the >> libjvm. Since its name contains the PID, we cannot even set that variable in >> the shell before starting the launcher. >> >> All that means that every launcher needs to especially parse and process the >> NMT arguments given at the command line (or via whatever method) and prepare >> the environment variable. `java` itself does this. This only works before >> the libjvm.so is loaded, before its dynamic C++ initialization. For that >> reason, it does not work if the launcher links statically against the >> hotspot, since in that case C++ initialization of the launcher and hotspot >> are folded into one phase with no possibility of executing code beforehand. >> >> And since it bypasses argument handling in the VM, it bypasses a number of >> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`. >> >> -- >> >> This patch fixes these shortcomings by making NMT late-initializable: it can >> now be initialized after normal VM argument parsing, like all other parts of >> the VM. This greatly simplifies NMT initialization and makes it work >> automagically for every third party launcher, as well as within our gtests. >> >> The glaring problem with late-initializing NMT is the NMT malloc headers. If >> we rule out just always having them (unacceptable in terms of memory >> overhead), there is no safe way to determine, in os::free(), if an >> allocation came from before or after NMT initialization ran, and therefore >> what to do with its malloc headers. For a more extensive explanation, please >> see the comment block `nmtPreInit.hpp` and the discussion with @kimbarrett >> and @zhengyu123 in the JBS comment section. >> >> The heart of this patch is a new way to track early, pre-NMT-init >> allocations. These are tracked via a lookup table. This was a suggestion by >> Kim and it worked out well. >> >> Changes in detail: >> >> - pre-NMT-init handling: >> - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init >> handling. They contain a small global lookup table managing C-heap blocks >> allocated in the pre-NMT-init phase. >> - `os::malloc()/os::realloc()/os::free()` defer to this code before >> doing anything else. >> - Please see the extensive comment block at the start of >> `nmtPreinit.hpp` explaining the details. >> >> - Changes to NMT: >> - Before, NMT initialization was spread over two phases, `initialize()` >> and `late_initialize()`. Those were merged into one and simplified - there >> is only one initialization now which happens after argument parsing. >> - Minor changes were needed for the `NMT_TrackingLevel` enum - to >> simplify code, I changed NMT_unknown to be numerically 0. A new comment >> block in `nmtCommon.hpp` now clearly specifies what's what, including >> allowed level state transitions. >> - New utility functions to translate tracking level from/to strings >> added to `NMTUtil` >> - NMT has never been able to handle virtual memo
Re: RFR: JDK-8256844: Make NMT late-initializable
On Thu, 29 Jul 2021 22:52:07 GMT, Coleen Phillimore wrote: >> Short: this patch makes NMT available in custom-launcher scenarios and >> during gtests. It simplifies NMT initialization. It adds a lot of >> NMT-specific testing, cleans them up and makes them sideeffect-free. >> >> - >> >> NMT continues to be an extremely useful tool for SAP to tackle memory >> problems in the JVM. >> >> However, NMT is of limited use due to the following restrictions: >> >> - NMT cannot be used if the hotspot is embedded into a custom launcher >> unless the launcher actively cooperates. Just creating and invoking the JVM >> is not enough, it needs to do some steps prior to loading the hotspot. This >> limitation is not well known (nor, do I believe, documented). Many products >> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this >> problem limits NMT usefulness greatly since our VMs are often embedded into >> custom launchers and modifying every launcher is impossible. >> - Worse, if that custom launcher links the libjvm *statically* there is just >> no way to activate NMT at all. This is the reason NMT cannot be used in the >> `gtestlauncher`. >> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` >> and `-XX:Flags=`. >> - The fact that NMT cannot be used in gtests is really a pity since it would >> allow us to both test NMT itself more rigorously and check for memory leaks >> while testing other stuff. >> >> The reason for all this is that NMT initialization happens very early, on >> the first call to `os::malloc()`. And those calls happen already during >> dynamic C++ initialization - a long time before the VM gets around parsing >> arguments. So, regular VM argument parsing is too late to parse NMT >> arguments. >> >> The current solution is to pass NMT arguments via a specially prepared >> environment variable: `NMT_LEVEL_=`. That environment >> variable has to be set by the embedding launcher, before it loads the >> libjvm. Since its name contains the PID, we cannot even set that variable in >> the shell before starting the launcher. >> >> All that means that every launcher needs to especially parse and process the >> NMT arguments given at the command line (or via whatever method) and prepare >> the environment variable. `java` itself does this. This only works before >> the libjvm.so is loaded, before its dynamic C++ initialization. For that >> reason, it does not work if the launcher links statically against the >> hotspot, since in that case C++ initialization of the launcher and hotspot >> are folded into one phase with no possibility of executing code beforehand. >> >> And since it bypasses argument handling in the VM, it bypasses a number of >> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`. >> >> -- >> >> This patch fixes these shortcomings by making NMT late-initializable: it can >> now be initialized after normal VM argument parsing, like all other parts of >> the VM. This greatly simplifies NMT initialization and makes it work >> automagically for every third party launcher, as well as within our gtests. >> >> The glaring problem with late-initializing NMT is the NMT malloc headers. If >> we rule out just always having them (unacceptable in terms of memory >> overhead), there is no safe way to determine, in os::free(), if an >> allocation came from before or after NMT initialization ran, and therefore >> what to do with its malloc headers. For a more extensive explanation, please >> see the comment block `nmtPreInit.hpp` and the discussion with @kimbarrett >> and @zhengyu123 in the JBS comment section. >> >> The heart of this patch is a new way to track early, pre-NMT-init >> allocations. These are tracked via a lookup table. This was a suggestion by >> Kim and it worked out well. >> >> Changes in detail: >> >> - pre-NMT-init handling: >> - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init >> handling. They contain a small global lookup table managing C-heap blocks >> allocated in the pre-NMT-init phase. >> - `os::malloc()/os::realloc()/os::free()` defer to this code before >> doing anything else. >> - Please see the extensive comment block at the start of >> `nmtPreinit.hpp` explaining the details. >> >> - Changes to NMT: >> - Before, NMT initialization was spread over two phases, `initialize()` >> and `late_initialize()`. Those were merged into one and simplified - there >> is only one initialization now which happens after argument parsing. >> - Minor changes were needed for the `NMT_TrackingLevel` enum - to >> simplify code, I changed NMT_unknown to be numerically 0. A new comment >> block in `nmtCommon.hpp` now clearly specifies what's what, including >> allowed level state transitions. >> - New utility functions to translate tracking level from/to strings >> added to `NMTUtil` >> - NMT has never been able to handle virtual memo
Re: [jdk17] RFR: JDK-8225667: Clarify the behavior of System::gc w.r.t. reference processing [v2]
On Wed, 30 Jun 2021 22:52:36 GMT, Mandy Chung wrote: >> This spec clarification is a follow-up to >> [JDK-8224760](https://bugs.openjdk.java.net/browse/JDK-8224760?focusedCommentId=14268320&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14268320) >> w.r.t. reference processing. Since there is no guarantee for any memory >> reclamation by the invocation of `System::gc`, the spec should also clarify >> that there is no guarantee in determining the change of reachability of any >> objects or any particular number of `Reference` objects be enqueued and >> cleared. >> >> CSR: >>https://bugs.openjdk.java.net/browse/JDK-8269690 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Kim's suggestion on the wording Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/183
Re: [jdk17] RFR: JDK-8225667: Clarify the behavior of System::gc w.r.t. reference processing
On Wed, 30 Jun 2021 18:24:24 GMT, Mandy Chung wrote:
> This spec clarification is a follow-up to
> [JDK-8224760](https://bugs.openjdk.java.net/browse/JDK-8224760?focusedCommentId=14268320&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14268320)
> w.r.t. reference processing. Since there is no guarantee for any memory
> reclamation by the invocation of `System::gc`, the spec should also clarify
> that there is no guarantee in determining the change of reachability of any
> objects or any particular number of `Reference` objects be enqueued and
> cleared.
>
> CSR:
>https://bugs.openjdk.java.net/browse/JDK-8269690
Changes requested by kbarrett (Reviewer).
src/java.base/share/classes/java/lang/Runtime.java line 661:
> 659: * the change of reachability in any particular number of objects
> 660: * or any particular number of {@link java.lang.ref.Reference
> Reference}
> 661: * objects be cleared and enqueued.
perhaps
s/number of objects or any/number of objects, or that any/
s/objects be/objects will be/
src/java.base/share/classes/java/lang/System.java line 1867:
> 1865: * the change of reachability in any particular number of objects
> 1866: * or any particular number of {@link java.lang.ref.Reference
> Reference}
> 1867: * objects be cleared and enqueued.
Similar suggestion here as for Runtime.gc.
-
PR: https://git.openjdk.java.net/jdk17/pull/183
Re: [jdk17] RFR: JDK-8262841: Clarify the behavior of PhantomReference::refersTo
On Wed, 30 Jun 2021 16:49:44 GMT, Mandy Chung wrote: > `Reference::refersTo` behaves the same for soft, weak, and phantom references > whereas `PhantomReference::get` always returns `null` which is different than > soft and weak references. > > This patch clarifies the specification of `PhantomReference` to make it clear > that `refersTo` can be used for phantom references. With `refersTo`, phantom > references if not registered in a reference queue are not completely useless. > Update the javadoc of the constructor to reflect that. > > I created a CSR to document this spec clarification: >https://bugs.openjdk.java.net/browse/JDK-8269688 Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/181
Re: RFR: 8254598: StringDedupTable should use OopStorage [v5]
On Fri, 14 May 2021 12:40:46 GMT, Zhengyu Gu wrote:
>> Kim Barrett has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 10 commits:
>>
>> - Merge branch 'master' into new_dedup2
>> - fix shenandoah merge
>> - Merge branch 'master' into new_dedup2
>> - misc cleanups
>> - refactor Requests::add
>> - fix typo
>> - more comment improvements
>> - improve naming and comments around injected String flags
>> - fix some typos in comments
>> - New string deduplication
>
>> The "merge from master" commit
>> ([ccb9951](https://github.com/openjdk/jdk/commit/ccb99515d020785d7fe1cf9a1c247aeed775dc19))
>> doesn't build with Shenandoah. I've asked Zhengyu to take a look.
>
> Just missing a parameter:
>
> ```diff --git a/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
> b/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
> index ddaa66ccc14..93a067fa22d 100644
> --- a/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
> +++ b/src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp
> @@ -57,7 +57,7 @@
> ShenandoahInitMarkRootsClosure::ShenandoahInitMarkRootsClosure(ShenandoahObjToSc
>
> template
> void ShenandoahInitMarkRootsClosure::do_oop_work(T* p) {
> - ShenandoahMark::mark_through_ref(p, _queue, _mark_context,
> false);
> + ShenandoahMark::mark_through_ref(p, _queue, _mark_context,
> NULL, false);
> }```
Thanks @zhengyu123 for the shenandoah merge fix.
Thanks @iklam , @coleenp , @tschatzl , @albertnetymk for reviews.
> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 314:
>
>> 312: size_t _bucket_index;
>> 313: size_t _shrink_index;
>> 314: bool _grow_only;
>
> Indentation
Not sure what this indentation comment is referring to.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Integrated: 8254598: StringDedupTable should use OopStorage
On Fri, 23 Apr 2021 19:48:47 GMT, Kim Barrett wrote:
> Please review this change to the String Deduplication facility. It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
>
> (The Shenandoah update was contributed by Zhengyu Gu.)
>
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in. However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication. Adding support by other GCs will be in followup RFEs.
>
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated. The full webrev might be a better
> place to look.
>
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
>
> This change is additionally labeled for review by core-libs, although there
> are no source changes there. This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class. (There were two unused bytes, now only one.)
>
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah. Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
>
> - gc/stringdedup/ -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
>
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
>
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.
This pull request has now been integrated.
Changeset: be0a6552
Author:Kim Barrett
URL:
https://git.openjdk.java.net/jdk/commit/be0a655208f64e076e9e0141fe5dadc862cba981
Stats: 6192 lines in 105 files changed: 2369 ins; 3204 del; 619 mod
8254598: StringDedupTable should use OopStorage
Co-authored-by: Kim Barrett
Co-authored-by: Zhengyu Gu
Reviewed-by: coleenp, iklam, tschatzl, ayang
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v5]
> Please review this change to the String Deduplication facility. It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
>
> (The Shenandoah update was contributed by Zhengyu Gu.)
>
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in. However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication. Adding support by other GCs will be in followup RFEs.
>
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated. The full webrev might be a better
> place to look.
>
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
>
> This change is additionally labeled for review by core-libs, although there
> are no source changes there. This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class. (There were two unused bytes, now only one.)
>
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah. Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
>
> - gc/stringdedup/ -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
>
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
>
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.
Kim Barrett has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 10 commits:
- Merge branch 'master' into new_dedup2
- fix shenandoah merge
- Merge branch 'master' into new_dedup2
- misc cleanups
- refactor Requests::add
- fix typo
- more comment improvements
- improve naming and comments around injected String flags
- fix some typos in comments
- New string deduplication
-
Changes: https://git.openjdk.java.net/jdk/pull/3662/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=04
Stats: 6192 lines in 105 files changed: 2369 ins; 3204 del; 619 mod
Patch: https://git.openjdk.java.net/jdk/pull/3662.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v4]
On Fri, 14 May 2021 04:31:59 GMT, Kim Barrett wrote:
>> Please review this change to the String Deduplication facility. It is being
>> changed to use OopStorage to hold weak references to relevant objects,
>> rather than bespoke data structures requiring dedicated processing phases by
>> supporting GCs.
>>
>> (The Shenandoah update was contributed by Zhengyu Gu.)
>>
>> This change significantly simplifies the interface between a given GC and
>> the String Deduplication facility, which should make it much easier for
>> other GCs to opt in. However, this change does not alter the set of GCs
>> providing support; currently only G1 and Shenandoah support string
>> deduplication. Adding support by other GCs will be in followup RFEs.
>>
>> Reviewing via the diffs might not be all that useful for some parts, as
>> several files have been essentially completely replaced, and a number of
>> files have been added or eliminated. The full webrev might be a better
>> place to look.
>>
>> The major changes are in gc/shared/stringdedup/* and in the supporting
>> collectors, but there are also some smaller changes in other places, most
>> notably classfile/{stringTable,javaClasses}.
>>
>> This change is additionally labeled for review by core-libs, although there
>> are no source changes there. This change injects a byte field of bits into
>> java.lang.String, using one of the previously unused padding bytes in that
>> class. (There were two unused bytes, now only one.)
>>
>> Testing:
>> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>>
>> Locally (linux-x64) ran all of the existing tests that use string
>> deduplication with both G1 and Shenandoah. Note that
>> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
>> currently fails, for reasons I haven't figured out but suspect are test
>> related.
>>
>> - gc/stringdedup/ -- these used to be in gc/g1
>> - runtime/cds/SharedStringsDedup.java
>> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
>> - runtime/cds/appcds/sharedStrings/*
>>
>> shenandoah-only:
>> - gc/shenandoah/jvmti/TestHeapDump.java
>> - gc/shenandoah/TestStringDedup.java
>> - gc/shenandoah/TestStringDedupStress.java
>>
>> Performance tested baseline, baseline + stringdedup, new with stringdedup,
>> finding no significant differences.
>
> Kim Barrett has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'master' into new_dedup2
> - misc cleanups
> - refactor Requests::add
> - fix typo
> - more comment improvements
> - improve naming and comments around injected String flags
> - fix some typos in comments
> - New string deduplication
The "merge from master" commit (ccb9951) doesn't build with Shenandoah. I've
asked Zhengyu to take a look.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v4]
> Please review this change to the String Deduplication facility. It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
>
> (The Shenandoah update was contributed by Zhengyu Gu.)
>
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in. However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication. Adding support by other GCs will be in followup RFEs.
>
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated. The full webrev might be a better
> place to look.
>
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
>
> This change is additionally labeled for review by core-libs, although there
> are no source changes there. This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class. (There were two unused bytes, now only one.)
>
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah. Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
>
> - gc/stringdedup/ -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
>
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
>
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.
Kim Barrett has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains eight commits:
- Merge branch 'master' into new_dedup2
- misc cleanups
- refactor Requests::add
- fix typo
- more comment improvements
- improve naming and comments around injected String flags
- fix some typos in comments
- New string deduplication
-
Changes: https://git.openjdk.java.net/jdk/pull/3662/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=03
Stats: 6191 lines in 104 files changed: 2369 ins; 3204 del; 618 mod
Patch: https://git.openjdk.java.net/jdk/pull/3662.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v2]
On Wed, 12 May 2021 11:55:08 GMT, Thomas Schatzl wrote: >> Kim Barrett has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - more comment improvements >> - improve naming and comments around injected String flags >> - fix some typos in comments > > src/hotspot/share/gc/shared/stringdedup/stringDedup.hpp line 82: > >> 80: // Doing so would counteract C2 optimizations on string literals. But an >> 81: // interned string might later become a deduplication candidate through >> the >> 82: // normal GC discovery mechanism. To prevent such modificatoins, the > > s/modificatoins/modifications Fixed. - PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v3]
> Please review this change to the String Deduplication facility. It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
>
> (The Shenandoah update was contributed by Zhengyu Gu.)
>
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in. However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication. Adding support by other GCs will be in followup RFEs.
>
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated. The full webrev might be a better
> place to look.
>
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
>
> This change is additionally labeled for review by core-libs, although there
> are no source changes there. This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class. (There were two unused bytes, now only one.)
>
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah. Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
>
> - gc/stringdedup/ -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
>
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
>
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.
Kim Barrett has updated the pull request incrementally with three additional
commits since the last revision:
- misc cleanups
- refactor Requests::add
- fix typo
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3662/files
- new: https://git.openjdk.java.net/jdk/pull/3662/files/656e2426..30e7b8e9
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=01-02
Stats: 69 lines in 6 files changed: 26 ins; 26 del; 17 mod
Patch: https://git.openjdk.java.net/jdk/pull/3662.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v2]
On Wed, 12 May 2021 21:13:54 GMT, Albert Mingkun Yang wrote:
>> Kim Barrett has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - more comment improvements
>> - improve naming and comments around injected String flags
>> - fix some typos in comments
>
> Just some minor comments.
Following up on an off-line discussion with @albertnetymk , I've done a little
refactoring of Requests::add. I also made a few other small cleanups, noticed
while dealing with @albertnetymk comments. I still haven't dealt with the
accumulated merge conflicts. I'll be doing that next.
> src/hotspot/share/gc/shared/stringdedup/stringDedup.cpp line 171:
>
>> 169: // Store the string in the next pre-allocated storage entry.
>> 170: oop* ref = _buffer[--_index];
>> 171: _buffer[_index] = nullptr;
>
> It's not really needed to clear the slot with null, right?
You are right; there used to be some asserts, but no longer. Removed.
> src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.cpp line 51:
>
>> 49: }
>> 50:
>> 51: StringDedup::Processor::~Processor() {}
>
> Why the empty destructor? Could we just not have it?
Changed to use `= default`.
> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 92:
>
>> 90: GrowableArrayCHeap _values;
>> 91:
>> 92: void adjust_capacity(int new_length);
>
> Nit: diff arg name in declaration and implementation, `new_length` vs
> `new_capacity`.
Changed to consistently use `new_capacity`
> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 295:
>
>> 293:
>> 294: protected:
>> 295: CleanupState() {}
>
> Is it possible to use `= default` here? Just like the destructor.
Changed to use `= default`.
> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 314:
>
>> 312: size_t _bucket_index;
>> 313: size_t _shrink_index;
>> 314: bool _grow_only;
>
> Seems unused.
Removed unused `_grow_only`, left over from earlier work.
> src/hotspot/share/memory/universe.cpp line 1153:
>
>> 1151: ResolvedMethodTable::verify();
>> 1152: }
>> 1153: if (should_verify_subset(Verify_StringDedup) &&
>> StringDedup::is_enabled()) {
>
> Maybe drop the `is_enabled()` check in the `if` to keep the consistency with
> other `if`s?
Done. StringDedup::verify already does an is_enabled check. Also fixed the
description comment, which incorrectly said `is_enabled()` was a precondition.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v2]
On Tue, 4 May 2021 11:46:21 GMT, Thomas Schatzl wrote: >> Kim Barrett has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - more comment improvements >> - improve naming and comments around injected String flags >> - fix some typos in comments > > src/hotspot/share/classfile/javaClasses.hpp line 170: > >> 168: static inline bool hash_is_set(oop string); >> 169: static inline bool is_latin1(oop java_string); >> 170: static inline bool no_deduplication(oop java_string); > > That identifier is missing a verb to read better, but I do not have a good > idea. Maybe it would be easier to use the negation of "no_deduplication", > something like "may_deduplicate"? > Feel free to ignore. "may_deduplicate" would require internally flipping the sense, to account for the initial value being false because of zero-initialization. I've changed it to "deduplication_forbidden"; hopefully that helps. - PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v2]
On Tue, 4 May 2021 11:47:25 GMT, Thomas Schatzl wrote: >> Kim Barrett has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - more comment improvements >> - improve naming and comments around injected String flags >> - fix some typos in comments > > src/hotspot/share/classfile/javaClasses.hpp line 171: > >> 169: static inline bool is_latin1(oop java_string); >> 170: static inline bool no_deduplication(oop java_string); >> 171: static inline bool deduplication_requested(oop java_string); > > Having a verb at the beginning like `is_deduplication_requested` sounds > better. "is_deduplication_requested" suggests there is currently a request, but this flag remains true even after the request is processed. - PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage [v2]
> Please review this change to the String Deduplication facility. It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
>
> (The Shenandoah update was contributed by Zhengyu Gu.)
>
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in. However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication. Adding support by other GCs will be in followup RFEs.
>
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated. The full webrev might be a better
> place to look.
>
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
>
> This change is additionally labeled for review by core-libs, although there
> are no source changes there. This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class. (There were two unused bytes, now only one.)
>
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah. Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
>
> - gc/stringdedup/ -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
>
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
>
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.
Kim Barrett has updated the pull request incrementally with three additional
commits since the last revision:
- more comment improvements
- improve naming and comments around injected String flags
- fix some typos in comments
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3662/files
- new: https://git.openjdk.java.net/jdk/pull/3662/files/2df362cb..656e2426
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=00-01
Stats: 70 lines in 8 files changed: 8 ins; 2 del; 60 mod
Patch: https://git.openjdk.java.net/jdk/pull/3662.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Fri, 23 Apr 2021 19:48:47 GMT, Kim Barrett wrote:
> Please review this change to the String Deduplication facility. It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
>
> (The Shenandoah update was contributed by Zhengyu Gu.)
>
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in. However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication. Adding support by other GCs will be in followup RFEs.
>
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated. The full webrev might be a better
> place to look.
>
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
>
> This change is additionally labeled for review by core-libs, although there
> are no source changes there. This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class. (There were two unused bytes, now only one.)
>
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah. Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
>
> - gc/stringdedup/ -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
>
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
>
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.
I've made some improvements to comments, responding to Thomas's suggestions.
Ive also changed some of the naming around the injected java.lang.String flags,
with modification now using test-and-set semantics (and named accordingly),
i.e. returning true if already set rather than if changed.
I've not yet dealt with the merge conflict in Shenandoah, but it looks like it
should be fairly easy. Just waiting for more comments before dealing with
merging updates.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Fri, 23 Apr 2021 19:48:47 GMT, Kim Barrett wrote:
> Please review this change to the String Deduplication facility. It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
>
> (The Shenandoah update was contributed by Zhengyu Gu.)
>
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in. However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication. Adding support by other GCs will be in followup RFEs.
>
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated. The full webrev might be a better
> place to look.
>
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
>
> This change is additionally labeled for review by core-libs, although there
> are no source changes there. This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class. (There were two unused bytes, now only one.)
>
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah. Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
>
> - gc/stringdedup/ -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
>
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
>
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.
src/hotspot/share/classfile/vmSymbols.hpp line 490:
> 488: template(data_cache_line_flush_size_name,
> "DATA_CACHE_LINE_FLUSH_SIZE") \
> 489: template(during_unsafe_access_name,
> "during_unsafe_access") \
> 490: template(no_deduplication_name,
> "no_deduplication") \
This addition was left over from an earlier version of the change, where I'd
added an ordinary private boolean member of this name to String. That was
before I learned about injected members and before the idea of adding
"duplication requested" came up. This name is no longer needed, and I'll be
removing it.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Wed, 28 Apr 2021 06:44:58 GMT, Ioi Lam wrote: >> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 557: >> >>> 555: // non-latin1, and deduplicating if we find a match. For >>> deduplication we >>> 556: // only care if the arrays consist of the same sequence of bytes. >>> 557: const jchar* chars = static_cast(value->base(T_CHAR)); >> >> The encoding of the shared strings always match CompactStrings. Otherwise >> the CDS archive will fail to map. E.g., >> >> >> $ java -XX:-CompactStrings -Xshare:dump >> $ java -XX:+CompactStrings -Xlog:cds -version >> ... >> [0.032s][info][cds] UseSharedSpaces: The shared archive file's >> CompactStrings >> setting (disabled) does not equal the current >> CompactStrings setting (enabled). >> [0.032s][info][cds] UseSharedSpaces: Unable to map shared spaces >> ... >> >> >> So you can avoid this `if` block when `CompactStrings == true`. The >> `!java_lang_String::is_latin1(found)` check below can be changed to an >> assert. >> >> Also, I think we need an explicit test case where you'd return `true` at >> line 564. I can write a new test case and email to you. I think it will >> involve dumping an archive with `-XX:-CompactStrings`. > > Oh, I realized that my suggestion above is wrong. When > `CompactStrings==true`, you can still have a string whose coder is UTF16: > > https://github.com/openjdk/jdk/blob/75a2354dc276e107d64516d20fc72bc7ef3d5f86/src/java.base/share/classes/java/lang/String.java#L343-L351 Correct. - PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Sun, 25 Apr 2021 17:32:51 GMT, Zhengyu Gu wrote:
>> Please review this change to the String Deduplication facility. It is being
>> changed to use OopStorage to hold weak references to relevant objects,
>> rather than bespoke data structures requiring dedicated processing phases by
>> supporting GCs.
>>
>> (The Shenandoah update was contributed by Zhengyu Gu.)
>>
>> This change significantly simplifies the interface between a given GC and
>> the String Deduplication facility, which should make it much easier for
>> other GCs to opt in. However, this change does not alter the set of GCs
>> providing support; currently only G1 and Shenandoah support string
>> deduplication. Adding support by other GCs will be in followup RFEs.
>>
>> Reviewing via the diffs might not be all that useful for some parts, as
>> several files have been essentially completely replaced, and a number of
>> files have been added or eliminated. The full webrev might be a better
>> place to look.
>>
>> The major changes are in gc/shared/stringdedup/* and in the supporting
>> collectors, but there are also some smaller changes in other places, most
>> notably classfile/{stringTable,javaClasses}.
>>
>> This change is additionally labeled for review by core-libs, although there
>> are no source changes there. This change injects a byte field of bits into
>> java.lang.String, using one of the previously unused padding bytes in that
>> class. (There were two unused bytes, now only one.)
>>
>> Testing:
>> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>>
>> Locally (linux-x64) ran all of the existing tests that use string
>> deduplication with both G1 and Shenandoah. Note that
>> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
>> currently fails, for reasons I haven't figured out but suspect are test
>> related.
>>
>> - gc/stringdedup/ -- these used to be in gc/g1
>> - runtime/cds/SharedStringsDedup.java
>> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
>> - runtime/cds/appcds/sharedStrings/*
>>
>> shenandoah-only:
>> - gc/shenandoah/jvmti/TestHeapDump.java
>> - gc/shenandoah/TestStringDedup.java
>> - gc/shenandoah/TestStringDedupStress.java
>>
>> Performance tested baseline, baseline + stringdedup, new with stringdedup,
>> finding no significant differences.
>
> src/hotspot/share/classfile/stringTable.cpp line 355:
>
>> 353: // Notify deduplication support that the string is being interned. A
>> string
>> 354: // must never be deduplicated after it has been interned. Doing so
>> interferes
>> 355: // with compiler optimizations don on e.g. interned string literals.
>
> typo: don -> done
Fixed locally.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Wed, 28 Apr 2021 00:29:23 GMT, Ioi Lam wrote:
>> src/hotspot/share/classfile/stringTable.cpp line 784:
>>
>>> 782: SharedStringIterator iter(f);
>>> 783: _shared_table.iterate(&iter);
>>> 784: }
>>
>> So with this change (somewhere below) do you no longer deduplicate strings
>> from the shared string table? ie
>>
>> // The CDS archive does not include the string deduplication table. Only the
>> string
>> // table is saved in the archive. The shared strings from CDS archive need
>> to be
>> // added to the string deduplication table before deduplication occurs. That
>> is
>> // done in the beginning of the StringDedupThread (see
>> StringDedupThread::do_deduplication()).
>> void StringDedupThread::deduplicate_shared_strings(StringDedupStat* stat) {
>> StringDedupSharedClosure sharedStringDedup(stat);
>> StringTable::shared_oops_do(&sharedStringDedup);
>> }
>> Asking @iklam to have a look then.
>
> If I understand correctly, we no longer need to add the entire set of shared
> strings into the dedup table.
>
>
> +// As a space optimization, when shared StringTable entries exist the shared
> +// part of the StringTable is also used as a source for byte arrays. This
> +// permits deduplication of strings against those shared entries without
> +// recording them in this table too.
> +class StringDedup::Table : AllStatic {
>
> ...
>
> +void StringDedup::Table::deduplicate(oop java_string) {
> + assert(java_lang_String::is_instance(java_string), "precondition");
> + _cur_stat.inc_inspected();
> + if ((StringTable::shared_entry_count() > 0) &&
> + try_deduplicate_shared(java_string)) {
> +return; // Done if deduplicated against shared
> StringTable.
Ioi is correct; we deduplicate "directly" against the shared string table
rather than adding the shared strings to the deduplication table.
-
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8254598: StringDedupTable should use OopStorage
On Tue, 27 Apr 2021 22:30:04 GMT, Coleen Phillimore wrote:
>> Please review this change to the String Deduplication facility. It is being
>> changed to use OopStorage to hold weak references to relevant objects,
>> rather than bespoke data structures requiring dedicated processing phases by
>> supporting GCs.
>>
>> (The Shenandoah update was contributed by Zhengyu Gu.)
>>
>> This change significantly simplifies the interface between a given GC and
>> the String Deduplication facility, which should make it much easier for
>> other GCs to opt in. However, this change does not alter the set of GCs
>> providing support; currently only G1 and Shenandoah support string
>> deduplication. Adding support by other GCs will be in followup RFEs.
>>
>> Reviewing via the diffs might not be all that useful for some parts, as
>> several files have been essentially completely replaced, and a number of
>> files have been added or eliminated. The full webrev might be a better
>> place to look.
>>
>> The major changes are in gc/shared/stringdedup/* and in the supporting
>> collectors, but there are also some smaller changes in other places, most
>> notably classfile/{stringTable,javaClasses}.
>>
>> This change is additionally labeled for review by core-libs, although there
>> are no source changes there. This change injects a byte field of bits into
>> java.lang.String, using one of the previously unused padding bytes in that
>> class. (There were two unused bytes, now only one.)
>>
>> Testing:
>> mach5 tier1-2 with and without -XX:+UseStringDeduplication
>>
>> Locally (linux-x64) ran all of the existing tests that use string
>> deduplication with both G1 and Shenandoah. Note that
>> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
>> currently fails, for reasons I haven't figured out but suspect are test
>> related.
>>
>> - gc/stringdedup/ -- these used to be in gc/g1
>> - runtime/cds/SharedStringsDedup.java
>> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
>> - runtime/cds/appcds/sharedStrings/*
>>
>> shenandoah-only:
>> - gc/shenandoah/jvmti/TestHeapDump.java
>> - gc/shenandoah/TestStringDedup.java
>> - gc/shenandoah/TestStringDedupStress.java
>>
>> Performance tested baseline, baseline + stringdedup, new with stringdedup,
>> finding no significant differences.
>
> src/hotspot/share/classfile/javaClasses.inline.hpp line 77:
>
>> 75:
>> 76: uint8_t* java_lang_String::flags_addr(oop java_string) {
>> 77: assert(_initialized, "Mut be initialized");
>
> typo: must
Fixed locally.
> src/hotspot/share/runtime/globals.hpp line 1994:
>
>> 1992:
>> \
>> 1993: product(uint64_t, StringDeduplicationHashSeed, 0, DIAGNOSTIC,
>> \
>> 1994: "Seed for the table hashing function; 0 requests computed
>> seed") \
>
> Should these two really be develop() options?
StringDeduplicationResizeALot is used by a test that we want to run against
release builds too. If StringDeduplicationHashSeed isn't a diagnostic option
(so available in release builds), I'd be more inclined to just remove it than
to make it a develop option.
> src/hotspot/share/runtime/mutexLocker.cpp line 239:
>
>> 237: def(StringDedupQueue_lock , PaddedMonitor, leaf,true,
>> _safepoint_check_never);
>> 238: def(StringDedupTable_lock , PaddedMutex , leaf + 1,true,
>> _safepoint_check_never);
>> 239: }
>
> Why weren't these duplicate definitions? This is disturbing.
These are assignments, not definitions. Only one of the assignments was being
performed because only one of G1 or Shenandoah will be selected. (Not that it
matters anymore after this change.)
-
PR: https://git.openjdk.java.net/jdk/pull/3662
RFR: 8254598: StringDedupTable should use OopStorage
Please review this change to the String Deduplication facility. It is being
changed to use OopStorage to hold weak references to relevant objects,
rather than bespoke data structures requiring dedicated processing phases by
supporting GCs.
(The Shenandoah update was contributed by Zhengyu Gu.)
This change significantly simplifies the interface between a given GC and
the String Deduplication facility, which should make it much easier for
other GCs to opt in. However, this change does not alter the set of GCs
providing support; currently only G1 and Shenandoah support string
deduplication. Adding support by other GCs will be in followup RFEs.
Reviewing via the diffs might not be all that useful for some parts, as
several files have been essentially completely replaced, and a number of
files have been added or eliminated. The full webrev might be a better
place to look.
The major changes are in gc/shared/stringdedup/* and in the supporting
collectors, but there are also some smaller changes in other places, most
notably classfile/{stringTable,javaClasses}.
This change is additionally labeled for review by core-libs, although there
are no source changes there. This change injects a byte field of bits into
java.lang.String, using one of the previously unused padding bytes in that
class. (There were two unused bytes, now only one.)
Testing:
mach5 tier1-2 with and without -XX:+UseStringDeduplication
Locally (linux-x64) ran all of the existing tests that use string
deduplication with both G1 and Shenandoah. Note that
TestStringDeduplicationInterned.java is disabled for shenandoah, as it
currently fails, for reasons I haven't figured out but suspect are test
related.
- gc/stringdedup/ -- these used to be in gc/g1
- runtime/cds/SharedStringsDedup.java
- runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
- runtime/cds/appcds/sharedStrings/*
shenandoah-only:
- gc/shenandoah/jvmti/TestHeapDump.java
- gc/shenandoah/TestStringDedup.java
- gc/shenandoah/TestStringDedupStress.java
Performance tested baseline, baseline + stringdedup, new with stringdedup,
finding no significant differences.
-
Commit messages:
- New string deduplication
Changes: https://git.openjdk.java.net/jdk/pull/3662/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3662&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8254598
Stats: 6185 lines in 105 files changed: 2361 ins; 3202 del; 622 mod
Patch: https://git.openjdk.java.net/jdk/pull/3662.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3662/head:pull/3662
PR: https://git.openjdk.java.net/jdk/pull/3662
Re: RFR: 8264489: Add more logging to LargeCopyWithMark.java
On Wed, 31 Mar 2021 06:56:23 GMT, Stefan Karlsson wrote: > Add more logging to the LargeCopyWithMark.java test, to gather more > information when this test fails with OOME. > > The intention is to gather more info for JDK-8239089. > > I consider this patch trivial, and expect to push it after I've gotten one > Review. Looks good, and trivial. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3282
Integrated: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
On Sun, 21 Mar 2021 03:53:24 GMT, Kim Barrett wrote: > Please review this change to java.util.Timer, replacing the use of deprecated > finalization-based cleanup with use of java.lang.ref.Cleaner. > > In addition, Timer.cancel now cancels any later execution of the the no > longer relevant cleanup. > > Testing: > mach5 tier1 > New AutoStop test verifies the specified cleanup behavior. > (There are existing tests involving Timer.cancel.) This pull request has now been integrated. Changeset: 2425462a Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/2425462a Stats: 111 lines in 2 files changed: 97 ins; 3 del; 11 mod 8263903: Use Cleaner instead of finalize to auto stop Timer thread Reviewed-by: dholmes, alanb, bchristi, rriggs, mchung - PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread [v4]
> Please review this change to java.util.Timer, replacing the use of deprecated > finalization-based cleanup with use of java.lang.ref.Cleaner. > > In addition, Timer.cancel now cancels any later execution of the the no > longer relevant cleanup. > > Testing: > mach5 tier1 > New AutoStop test verifies the specified cleanup behavior. > (There are existing tests involving Timer.cancel.) 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 four additional commits since the last revision: - Merge branch 'master' into nofinalize - make ThreadReaper constructor non-public - dholmes review - Use cleaner instead of finalize to auto stop Timer thread - Changes: - all: https://git.openjdk.java.net/jdk/pull/3106/files - new: https://git.openjdk.java.net/jdk/pull/3106/files/57500464..f62aa2f0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3106&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3106&range=02-03 Stats: 2955 lines in 191 files changed: 1549 ins; 748 del; 658 mod Patch: https://git.openjdk.java.net/jdk/pull/3106.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3106/head:pull/3106 PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread [v3]
On Tue, 23 Mar 2021 18:36:24 GMT, Brent Christian wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> make ThreadReaper constructor non-public > > Marked as reviewed by bchristi (Reviewer). Thanks all for reviews. - PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread [v3]
> Please review this change to java.util.Timer, replacing the use of deprecated > finalization-based cleanup with use of java.lang.ref.Cleaner. > > In addition, Timer.cancel now cancels any later execution of the the no > longer relevant cleanup. > > Testing: > mach5 tier1 > New AutoStop test verifies the specified cleanup behavior. > (There are existing tests involving Timer.cancel.) Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: make ThreadReaper constructor non-public - Changes: - all: https://git.openjdk.java.net/jdk/pull/3106/files - new: https://git.openjdk.java.net/jdk/pull/3106/files/2153a4c3..57500464 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3106&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3106&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3106.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3106/head:pull/3106 PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread [v2]
On Mon, 22 Mar 2021 07:15:24 GMT, Kim Barrett wrote: >> Please review this change to java.util.Timer, replacing the use of >> deprecated finalization-based cleanup with use of java.lang.ref.Cleaner. >> >> In addition, Timer.cancel now cancels any later execution of the the no >> longer relevant cleanup. >> >> Testing: >> mach5 tier1 >> New AutoStop test verifies the specified cleanup behavior. >> (There are existing tests involving Timer.cancel.) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > dholmes review test/jdk/java/util/Timer/AutoStop.java line 27: > 25: * @test > 26: * @bug 8263903 > 27: * @requires vm.gc != "Epsilon" I added the `@requires` late in dev/test and used the wrong test. - PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread [v2]
> Please review this change to java.util.Timer, replacing the use of deprecated > finalization-based cleanup with use of java.lang.ref.Cleaner. > > In addition, Timer.cancel now cancels any later execution of the the no > longer relevant cleanup. > > Testing: > mach5 tier1 > New AutoStop test verifies the specified cleanup behavior. > (There are existing tests involving Timer.cancel.) Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: dholmes review - Changes: - all: https://git.openjdk.java.net/jdk/pull/3106/files - new: https://git.openjdk.java.net/jdk/pull/3106/files/0d120f56..2153a4c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3106&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3106&range=00-01 Stats: 5 lines in 1 file changed: 2 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3106.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3106/head:pull/3106 PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
On Sun, 21 Mar 2021 22:57:10 GMT, David Holmes wrote:
>> Please review this change to java.util.Timer, replacing the use of
>> deprecated finalization-based cleanup with use of java.lang.ref.Cleaner.
>>
>> In addition, Timer.cancel now cancels any later execution of the the no
>> longer relevant cleanup.
>>
>> Testing:
>> mach5 tier1
>> New AutoStop test verifies the specified cleanup behavior.
>> (There are existing tests involving Timer.cancel.)
>
> test/jdk/java/util/Timer/AutoStop.java line 47:
>
>> 45: public void run() {
>> 46: tdThread = Thread.currentThread();
>> 47: synchronized(wakeup) {
>
> tdThread should be set inside the sync block, and then doesn't need to be
> declared volatile
Without volatile is the while loop still okay? And the read for the join call?
-
PR: https://git.openjdk.java.net/jdk/pull/3106
Re: RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
On Sun, 21 Mar 2021 22:49:37 GMT, David Holmes wrote:
>> Please review this change to java.util.Timer, replacing the use of
>> deprecated finalization-based cleanup with use of java.lang.ref.Cleaner.
>>
>> In addition, Timer.cancel now cancels any later execution of the the no
>> longer relevant cleanup.
>>
>> Testing:
>> mach5 tier1
>> New AutoStop test verifies the specified cleanup behavior.
>> (There are existing tests involving Timer.cancel.)
>
> test/jdk/java/util/Timer/AutoStop.java line 67:
>
>> 65: t.schedule(new TimerTask() {
>> 66: public void run() {
>> 67: ++counter;
>
> This is not thread-safe. Operations on volatile variables are not atomic.
Only one thread (the timer's thread) is writing, via a sequential series of
task executions, so the simple increments are fine. I made `counter` volatile
because it's being written by one thread and read by a different thread. Happy
to drop the qualifier if that's not needed.
-
PR: https://git.openjdk.java.net/jdk/pull/3106
RFR: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
Please review this change to java.util.Timer, replacing the use of deprecated finalization-based cleanup with use of java.lang.ref.Cleaner. In addition, Timer.cancel now cancels any later execution of the the no longer relevant cleanup. Testing: mach5 tier1 New AutoStop test verifies the specified cleanup behavior. (There are existing tests involving Timer.cancel.) - Commit messages: - Use cleaner instead of finalize to auto stop Timer thread Changes: https://git.openjdk.java.net/jdk/pull/3106/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3106&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263903 Stats: 110 lines in 2 files changed: 96 ins; 3 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/3106.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3106/head:pull/3106 PR: https://git.openjdk.java.net/jdk/pull/3106
Integrated: 8132984: incorrect type for Reference.discovered
On Sat, 26 Dec 2020 03:25:51 GMT, Kim Barrett wrote: > Please review this change which fixes the type of the private > Reference.discovered field. It was Reference, but that's wrong because > it can be any Reference object. > > I've changed it to Reference and let that flow through, updating some > other variables that were previously somewhat incorrectly typed (usually > with an Object type parameter). The interesting change is to the > ReferenceQueue.enqueue parameter, which is now also Reference. > > This ultimately end up with a provably safe and correct, but uncheckable, > cast in ReferenceQueue.enqueue. > > An alternative might be to use a raw type for the discovered field, but I > think that ends up with more @SuppressWarnings of various flavors. I think > the unbounded wildcard approach is clearer and cleaner. > > Note that all of the pending list handling, including the discovered field, > could be moved into a non-public, non-generic, sealed(?) base class of > Reference. The pending list handling has nothing to do with the generic > parameter T. > > Testing: > mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) This pull request has now been integrated. Changeset: 33dcc00c Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/33dcc00c Stats: 17 lines in 1 file changed: 10 ins; 1 del; 6 mod 8132984: incorrect type for Reference.discovered Use unbounded wildcard placeholders, plus a new helper to get back to the Reference domain. Reviewed-by: rkennke, plevart, rriggs, mchung - PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8132984: incorrect type for Reference.discovered [v4]
> Please review this change which fixes the type of the private > Reference.discovered field. It was Reference, but that's wrong because > it can be any Reference object. > > I've changed it to Reference and let that flow through, updating some > other variables that were previously somewhat incorrectly typed (usually > with an Object type parameter). The interesting change is to the > ReferenceQueue.enqueue parameter, which is now also Reference. > > This ultimately end up with a provably safe and correct, but uncheckable, > cast in ReferenceQueue.enqueue. > > An alternative might be to use a raw type for the discovered field, but I > think that ends up with more @SuppressWarnings of various flavors. I think > the unbounded wildcard approach is clearer and cleaner. > > Note that all of the pending list handling, including the discovered field, > could be moved into a non-public, non-generic, sealed(?) base class of > Reference. The pending list handling has nothing to do with the generic > parameter T. > > Testing: > mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) 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 five additional commits since the last revision: - Merge branch 'master' into fix_discovered_type - plevart improvement - update copyrights - Merge branch 'master' into fix_discovered_type - Use unbounded wildcard placeholders and final safe but unchecked cast - Changes: - all: https://git.openjdk.java.net/jdk/pull/1897/files - new: https://git.openjdk.java.net/jdk/pull/1897/files/b95f5140..cd66bff1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1897&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1897&range=02-03 Stats: 3433 lines in 92 files changed: 381 ins; 2791 del; 261 mod Patch: https://git.openjdk.java.net/jdk/pull/1897.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1897/head:pull/1897 PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
On Tue, 19 Jan 2021 11:15:17 GMT, Peter Levart wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> plevart improvement > > This looks good. Thanks @plevart , @rkennke , @RogerRiggs , and @mlchung for reviews. - PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
> Please review this change which fixes the type of the private > Reference.discovered field. It was Reference, but that's wrong because > it can be any Reference object. > > I've changed it to Reference and let that flow through, updating some > other variables that were previously somewhat incorrectly typed (usually > with an Object type parameter). The interesting change is to the > ReferenceQueue.enqueue parameter, which is now also Reference. > > This ultimately end up with a provably safe and correct, but uncheckable, > cast in ReferenceQueue.enqueue. > > An alternative might be to use a raw type for the discovered field, but I > think that ends up with more @SuppressWarnings of various flavors. I think > the unbounded wildcard approach is clearer and cleaner. > > Note that all of the pending list handling, including the discovered field, > could be moved into a non-public, non-generic, sealed(?) base class of > Reference. The pending list handling has nothing to do with the generic > parameter T. > > Testing: > mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: plevart improvement - Changes: - all: https://git.openjdk.java.net/jdk/pull/1897/files - new: https://git.openjdk.java.net/jdk/pull/1897/files/80415b71..b95f5140 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1897&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1897&range=01-02 Stats: 23 lines in 2 files changed: 10 ins; 9 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1897.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1897/head:pull/1897 PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8132984: incorrect type for Reference.discovered
> On Jan 18, 2021, at 12:28 PM, Peter Levart wrote:
> If you introduce a private method in Reference:
>
>private void enqueueFromPending() {
>var q = queue;
>if (q != ReferenceQueue.NULL) q.enqueue(this);
>}
>
> ...and use it Reference.processPendingReferences while loop like this:
>
>if (ref instanceof Cleaner) {
>...
>} else {
>ref.enqueueFromPending();
>}
>
> Then you can keep the signature of `ReferenceQueue.enqueue(Reference extends T> r)` and no unchecked casts are needed there.
Nice! And this reverts all changes to ReferenceQueue.java
> But what you have is OK and much better than what was before.
Thanks, but I’m going to take your improvement. I’ll update the PR once I’ve
re-run some tests.
Re: RFR: 8132984: incorrect type for Reference.discovered [v2]
> Please review this change which fixes the type of the private > Reference.discovered field. It was Reference, but that's wrong because > it can be any Reference object. > > I've changed it to Reference and let that flow through, updating some > other variables that were previously somewhat incorrectly typed (usually > with an Object type parameter). The interesting change is to the > ReferenceQueue.enqueue parameter, which is now also Reference. > > This ultimately end up with a provably safe and correct, but uncheckable, > cast in ReferenceQueue.enqueue. > > An alternative might be to use a raw type for the discovered field, but I > think that ends up with more @SuppressWarnings of various flavors. I think > the unbounded wildcard approach is clearer and cleaner. > > Note that all of the pending list handling, including the discovered field, > could be moved into a non-public, non-generic, sealed(?) base class of > Reference. The pending list handling has nothing to do with the generic > parameter T. > > Testing: > mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) 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 three additional commits since the last revision: - update copyrights - Merge branch 'master' into fix_discovered_type - Use unbounded wildcard placeholders and final safe but unchecked cast - Changes: - all: https://git.openjdk.java.net/jdk/pull/1897/files - new: https://git.openjdk.java.net/jdk/pull/1897/files/ff250a19..80415b71 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1897&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1897&range=00-01 Stats: 62902 lines in 1944 files changed: 22288 ins; 24876 del; 15738 mod Patch: https://git.openjdk.java.net/jdk/pull/1897.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1897/head:pull/1897 PR: https://git.openjdk.java.net/jdk/pull/1897
RFR: 8132984: incorrect type for Reference.discovered
Please review this change which fixes the type of the private Reference.discovered field. It was Reference, but that's wrong because it can be any Reference object. I've changed it to Reference and let that flow through, updating some other variables that were previously somewhat incorrectly typed (usually with an Object type parameter). The interesting change is to the ReferenceQueue.enqueue parameter, which is now also Reference. This ultimately end up with a provably safe and correct, but uncheckable, cast in ReferenceQueue.enqueue. An alternative might be to use a raw type for the discovered field, but I think that ends up with more @SuppressWarnings of various flavors. I think the unbounded wildcard approach is clearer and cleaner. Note that all of the pending list handling, including the discovered field, could be moved into a non-public, non-generic, sealed(?) base class of Reference. The pending list handling has nothing to do with the generic parameter T. Testing: mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) - Commit messages: - Use unbounded wildcard placeholders and final safe but unchecked cast Changes: https://git.openjdk.java.net/jdk/pull/1897/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1897&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8132984 Stats: 16 lines in 2 files changed: 8 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/1897.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1897/head:pull/1897 PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v3]
On Tue, 8 Dec 2020 17:30:11 GMT, Mandy Chung wrote: >> 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 five additional >> commits since the last revision: >> >> - Merge branch 'master' into no_isenqueued >> - move REMOVE and RETAIN decls and init >> - update WeakReferenceGC test >> - update ReferenceQueue test >> - update ReferencesGC test > > Marked as reviewed by mchung (Reviewer). Thanks for reviews @mlchung and @tschatzl - PR: https://git.openjdk.java.net/jdk/pull/1691
Integrated: 8257876: Avoid Reference.isEnqueued in tests
On Tue, 8 Dec 2020 09:52:51 GMT, Kim Barrett wrote: > Please review this change that eliminates the use of Reference.isEnqueued by > tests. There were three tests using it: > > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > > In each of them, some combination of using Reference.refersTo and > ReferenceQueue.remove with a timeout were used to eliminate the use of > Reference.isEnqueued. > > I also cleaned up ReferencesGC.java in various respects. It contained > several bits of dead code, and the failure checks were made stronger. > > Testing: > mach5 tier1 > Locally (linux-x64) ran all three tests with each GC (including Shenandoah). This pull request has now been integrated. Changeset: db5da961 Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/db5da961 Stats: 104 lines in 3 files changed: 23 ins; 39 del; 42 mod 8257876: Avoid Reference.isEnqueued in tests Reviewed-by: mchung, tschatzl - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v3]
> Please review this change that eliminates the use of Reference.isEnqueued by > tests. There were three tests using it: > > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > > In each of them, some combination of using Reference.refersTo and > ReferenceQueue.remove with a timeout were used to eliminate the use of > Reference.isEnqueued. > > I also cleaned up ReferencesGC.java in various respects. It contained > several bits of dead code, and the failure checks were made stronger. > > Testing: > mach5 tier1 > Locally (linux-x64) ran all three tests with each GC (including Shenandoah). 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 five additional commits since the last revision: - Merge branch 'master' into no_isenqueued - move REMOVE and RETAIN decls and init - update WeakReferenceGC test - update ReferenceQueue test - update ReferencesGC test - Changes: - all: https://git.openjdk.java.net/jdk/pull/1691/files - new: https://git.openjdk.java.net/jdk/pull/1691/files/01710567..d5355342 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1691&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1691&range=01-02 Stats: 7952 lines in 328 files changed: 5091 ins; 1646 del; 1215 mod Patch: https://git.openjdk.java.net/jdk/pull/1691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691 PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]
> Please review this change that eliminates the use of Reference.isEnqueued by > tests. There were three tests using it: > > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > > In each of them, some combination of using Reference.refersTo and > ReferenceQueue.remove with a timeout were used to eliminate the use of > Reference.isEnqueued. > > I also cleaned up ReferencesGC.java in various respects. It contained > several bits of dead code, and the failure checks were made stronger. > > Testing: > mach5 tier1 > Locally (linux-x64) ran all three tests with each GC (including Shenandoah). Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: move REMOVE and RETAIN decls and init - Changes: - all: https://git.openjdk.java.net/jdk/pull/1691/files - new: https://git.openjdk.java.net/jdk/pull/1691/files/e87206a8..01710567 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1691&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1691&range=00-01 Stats: 6 lines in 1 file changed: 4 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691 PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests [v2]
On Wed, 9 Dec 2020 13:59:09 GMT, Thomas Schatzl wrote:
>> test/jdk/java/lang/ref/ReferenceEnqueue.java line 58:
>>
>>> 56: for (int i = 0; i < iterations; i++) {
>>> 57: System.gc();
>>> 58: enqueued = (queue.remove(100) == ref);
>>
>> The code does not catch `InterruptedException` like it does in the other
>> files.
>
> I understand that the test code previously just forwarded the
> `InterruptedException` if it happened in the `Thread.sleep()` call too. So
> this may only be an exiting issue and please ignore this comment.
> Not catching `InterruptedException` here only seems to be a cause for
> unnecessary failure. Then again, it probably does not happen a lot.
Nothing in the test calls Thread.interrupt(), so there isn't a risk of
failure due to not handling that exception in some "interesting" way. But
InterruptedException must be "handled" somehow, because it's a checked
exception. That's already dealt with by the run() method declaring that it
throws that type, and main declaring that it throws Exception. The other
tests modified in this change don't take that approach (just let it
propagate out through main), instead wrapping the interruptable calls in
try/catch, though again just to satisfy the requirement that a checked
exception must be statically verified to be handled, even though there
aren't going to be any thrown.
-
PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Wed, 9 Dec 2020 13:26:04 GMT, Thomas Schatzl wrote: >> Please review this change that eliminates the use of Reference.isEnqueued by >> tests. There were three tests using it: >> >> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java >> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java >> jdk/java/lang/ref/ReferenceEnqueue.java >> >> In each of them, some combination of using Reference.refersTo and >> ReferenceQueue.remove with a timeout were used to eliminate the use of >> Reference.isEnqueued. >> >> I also cleaned up ReferencesGC.java in various respects. It contained >> several bits of dead code, and the failure checks were made stronger. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) ran all three tests with each GC (including Shenandoah). > > test/hotspot/jtreg/vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java line > 129: > >> 127: } >> 128: >> 129: int REMOVE = (int) (RANGE * RATIO); > > These two constants could be factored out as static finals to match the > casing. I'm making REMOVE and RETAIN statics, near RANGE and RATIO. (Meant to do that before, but forgot.) They can't be final though, because RANGE and RATIO aren't final, and can be set from command line arguments. So they'll get initialized in parseArgs. - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Wed, 9 Dec 2020 13:28:44 GMT, Thomas Schatzl wrote: >> Please review this change that eliminates the use of Reference.isEnqueued by >> tests. There were three tests using it: >> >> vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java >> vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java >> jdk/java/lang/ref/ReferenceEnqueue.java >> >> In each of them, some combination of using Reference.refersTo and >> ReferenceQueue.remove with a timeout were used to eliminate the use of >> Reference.isEnqueued. >> >> I also cleaned up ReferencesGC.java in various respects. It contained >> several bits of dead code, and the failure checks were made stronger. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) ran all three tests with each GC (including Shenandoah). > > Changes requested by tschatzl (Reviewer). > [pre-existing] The topWeakReferenceGC.java description at the top describes > that the test calls System.gc() explicitly to trigger garbage collections at > the end. It does not. Maybe this could be weasel-worded around like in the > other cases in that text. There are a lot of things much more wrong with that comment. Doing more GCs doesn't cause more enqueues to happen. The "non-deterministic" enqueuing is just a race. The GC adds references to the pending list. The reference processing thread transfers references from the pending list to their associated queue (if any). The test code is racing with that. The change to use Reference.remove with a timeout eliminates all that, and one GC should be. Addressing all that would be a substantial rewrite of this test though. Mind if I defer that to a new RFE? - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue [v3]
On Tue, 8 Dec 2020 20:42:52 GMT, Mandy Chung wrote: >> `Reference::isEnqueued` method was never implemented as it was initially >> specified since 1.2. The specification says that it tests if this reference >> object has been enqueued, but the long-standing behavior is to test if this >> reference object is in its associated queue, only reflecting the state at >> the time when this method is executed. The implementation doesn't do what >> the specification promised, which might cause serious bugs if unnoticed. For >> example, an application that relies on this method to release critical >> resources could cause serious performance issues, in particular when this >> method is misused on Reference objects without an associated queue. >> `Reference::refersTo(null)` is the better recommended way to test if a >> Reference object has been cleared. >> >> This proposes to deprecate `Reference::isEnqueued`. Also the spec is >> updated to match the long-standing behavior. >> >> CSR: https://bugs.openjdk.java.net/browse/JDK-8189386 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > improve the deprecation message per feedback Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1684
RFR: 8257876: Avoid Reference.isEnqueued in tests
Please review this change that eliminates the use of Reference.isEnqueued by tests. There were three tests using it: vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java jdk/java/lang/ref/ReferenceEnqueue.java In each of them, some combination of using Reference.refersTo and ReferenceQueue.remove with a timeout were used to eliminate the use of Reference.isEnqueued. I also cleaned up ReferencesGC.java in various respects. It contained several bits of dead code, and the failure checks were made stronger. Testing: mach5 tier1 Locally (linux-x64) ran all three tests with each GC (including Shenandoah). - Commit messages: - update WeakReferenceGC test - update ReferenceQueue test - update ReferencesGC test Changes: https://git.openjdk.java.net/jdk/pull/1691/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1691&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257876 Stats: 102 lines in 3 files changed: 21 ins; 39 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/1691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691 PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
On Tue, 8 Dec 2020 02:36:01 GMT, Mandy Chung wrote: > `Reference::isEnqueued` method was never implemented as it was initially > specified since 1.2. The specification says that it tests if this reference > object has been enqueued, but the long-standing behavior is to test if this > reference object is in its associated queue, only reflecting the state at the > time when this method is executed. The implementation doesn't do what the > specification promised, which might cause serious bugs if unnoticed. For > example, an application that relies on this method to release critical > resources could cause serious performance issues, in particular when this > method is misused on Reference objects without an associated queue. > `Reference::refersTo(null)` is the better recommended way to test if a > Reference object has been cleared. > > This proposes to deprecate `Reference::isEnqueued`. Also the spec is updated > to match the long-standing behavior. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8189386 This change looks good. There are a couple of tests that are using isEnqueued. vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java jdk/java/lang/ref/ReferenceEnqueue.java https://bugs.openjdk.java.net/browse/JDK-8257876 (I'm working on this.) - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1684
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` [v2]
On Fri, 4 Dec 2020 22:38:24 GMT, Mandy Chung wrote: >> This patch replaces some uses of `Reference::get` to `Reference::refersTo` >> to avoid keeping a referent strongly reachable that could cause unnecessary >> delay in collecting such object. I only made change in some but not all >> classes in core libraries when working with Kim on `Reference::refersTo`. >> The remaining uses are left for the component owners to convert at >> appropriate time. > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - revise matchsKey to check equality if the reference is not cleared > - fix typo > - update copyright end year and fixup grammatical nit > - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo > - 8256167: Convert JDK use of to > - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo > - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo > - minor cleanup > - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into > refersTo > - improve refersTo0 descriptions > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/d8ac76fa...72947eb3 Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > This patch replaces some uses of `Reference::get` to `Reference::refersTo` to > avoid keeping a referent strongly reachable that could cause unnecessary > delay in collecting such object. I only made change in some but not all > classes in core libraries when working with Kim on `Reference::refersTo`. > The remaining uses are left for the component owners to convert at > appropriate time. src/java.base/share/classes/java/util/WeakHashMap.java line 293: > 291: // then checks for equality > 292: Object k = e.get(); > 293: return key == k || key.equals(k); I think `key == k` is already covered by refersTo. But k could be null; checking for that here might be useful, to skip the call to equals in that case. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v3]
> Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. 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 six additional commits since the last revision: - Merge branch 'master' into java_clear - new tests need ref in oldgen too - remove obsolete comment about races with clear and enqueue - add private native Reference::clear0 - test clear during marking - test clear during reference processing - Changes: - all: https://git.openjdk.java.net/jdk/pull/1376/files - new: https://git.openjdk.java.net/jdk/pull/1376/files/c19efd70..dfa51fb3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1376&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1376&range=01-02 Stats: 74214 lines in 450 files changed: 70893 ins; 2086 del; 1235 mod Patch: https://git.openjdk.java.net/jdk/pull/1376.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1376/head:pull/1376 PR: https://git.openjdk.java.net/jdk/pull/1376
Integrated: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett wrote: > Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. This pull request has now been integrated. Changeset: 66943fef Author:Kim Barrett URL: https://git.openjdk.java.net/jdk/commit/66943fef Stats: 317 lines in 14 files changed: 286 ins; 22 del; 9 mod 8256517: (ref) Reference.clear during reference processing may lose notification 8240696: (ref) Reference.clear may extend the lifetime of the referent Use private native helper to implement Reference.clear. Reviewed-by: pliden, rkennke, mchung - PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
On Mon, 23 Nov 2020 20:36:57 GMT, Roman Kennke wrote: >> Kim Barrett has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - new tests need ref in oldgen too >> - remove obsolete comment about races with clear and enqueue > > Looks good! Thanks! Thanks @rkennke, @pliden, @mlchung for reviews. - PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
On Tue, 24 Nov 2020 02:59:50 GMT, Kim Barrett wrote:
>> Looks good. Just want to request that you also remove the following comment
>> in zReferenceProcessor.cpp, as it's no longer true.
>>
>> --- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> +++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> @@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop
>> reference, ReferenceType type) con
>> }
>>
>> bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type)
>> const {
>> - // This check is racing with a call to Reference.clear() from the
>> application.
>> - // If the application clears the reference after this check it will still
>> end
>> - // up on the pending list, and there's nothing we can do about that
>> without
>> - // changing the Reference.clear() API. This check is also racing with a
>> call
>> - // to Reference.enqueue() from the application, which is unproblematic,
>> since
>> - // the application wants the reference to be enqueued anyway.
>>const oop referent = reference_referent(reference);
>>if (referent == NULL) {
>> // Reference has been cleared, by a call to Reference.enqueue()
>
>> Looks good. Just want to request that you also remove the following comment
>> in zReferenceProcessor.cpp, as it's no longer true.
>>
>> ```
>> --- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> +++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
>> @@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop
>> reference, ReferenceType type) con
>> }
>>
>> bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type)
>> const {
>> - // This check is racing with a call to Reference.clear() from the
>> application.
>> - // If the application clears the reference after this check it will still
>> end
>> - // up on the pending list, and there's nothing we can do about that
>> without
>> - // changing the Reference.clear() API. This check is also racing with a
>> call
>> - // to Reference.enqueue() from the application, which is unproblematic,
>> since
>> - // the application wants the reference to be enqueued anyway.
>>const oop referent = reference_referent(reference);
>>if (referent == NULL) {
>> // Reference has been cleared, by a call to Reference.enqueue()
>> ```
>
> Done.
I realized there was a theoretical problem with the new tests; they
should also be ensuring the Reference objects are in oldgen. That's
fixed in the latest push.
-
PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
> Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. Kim Barrett has updated the pull request incrementally with two additional commits since the last revision: - new tests need ref in oldgen too - remove obsolete comment about races with clear and enqueue - Changes: - all: https://git.openjdk.java.net/jdk/pull/1376/files - new: https://git.openjdk.java.net/jdk/pull/1376/files/46e5b1f6..c19efd70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1376&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1376&range=00-01 Stats: 17 lines in 3 files changed: 7 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1376.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1376/head:pull/1376 PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]
On Mon, 23 Nov 2020 12:50:31 GMT, Per Liden wrote:
> Looks good. Just want to request that you also remove the following comment
> in zReferenceProcessor.cpp, as it's no longer true.
>
> ```
> --- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
> +++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
> @@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop reference,
> ReferenceType type) con
> }
>
> bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type)
> const {
> - // This check is racing with a call to Reference.clear() from the
> application.
> - // If the application clears the reference after this check it will still
> end
> - // up on the pending list, and there's nothing we can do about that without
> - // changing the Reference.clear() API. This check is also racing with a
> call
> - // to Reference.enqueue() from the application, which is unproblematic,
> since
> - // the application wants the reference to be enqueued anyway.
>const oop referent = reference_referent(reference);
>if (referent == NULL) {
> // Reference has been cleared, by a call to Reference.enqueue()
> ```
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/1376
RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
Please review this change to Reference.clear() to address several issues. (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent field to null may extend the lifetime of the referent value. (JDK-8240696) For GCs with concurrent reference processing, clearing the referent field during reference processing may discard the expected notification. Both of these are addressed by introducing a private native helper function for clearing the referent, rather than using an ordinary in-Java field assignment. Tests have been added for both of these issues. This required adding a new breakpoint in reference processing for ZGC. Of course, finalization adds some complexity to the problem. We deal with that by having FinalReference override clear. The implementation is provided by a new package-private method in Reference. (There are a number of alternatives, all of them clumsy; finalization is annoying that way.) While dealing with FinalReference clearing it was noted that the recent JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not updated to call the new Reference.getInactive(), instead still calling get() on FinalReferences, with the JDK-8256106 problems. Fixing that showed the assertion for inactive FinalReference added by JDK-8256370 used the wrong test. Rather than tracking down and changing all get() and clear() calls on final references and changing them to use getInactive and a new similar clear function, I've changed FinalReference to override get and clear, which call the helper functions in Reference. I've also renamed getInactive to be more explanatory and less convenient to call directly, and similarly named the helper for clear. This means that get/clear should never be called on an active FinalReference. That's already never done, and would have problems if it were. Testing: mach5 tier1-6 Local (linux-x64) tier1 using Shenandoah. New TestReferenceClearDuringMarking fails for G1 without these changes. New TestReferenceClearDuringReferenceProcessing fails for ZGC without these changes. - Commit messages: - add private native Reference::clear0 - test clear during marking - test clear during reference processing Changes: https://git.openjdk.java.net/jdk/pull/1376/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1376&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256517 Stats: 304 lines in 13 files changed: 279 ins; 16 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/1376.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1376/head:pull/1376 PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256370: Add asserts to Reference.getInactive()
On Mon, 16 Nov 2020 18:30:38 GMT, Mandy Chung wrote: >> A follow-up to JDK-8256106, this is adding two asserts to check that the API >> is used as it should be, i.e. only on inactive FinalReferences. Also, in >> Finalizer, where getInactive() is used, there is a null-check. The GC must >> never clean the referent, and Java code doesn't clean it either, it would be >> a bug if we ever see null there. I think it's better to fail there (with >> assert or NPE) when that happens instead of silently accepting it. >> >> Testing: >> - [x] tier1 >> - [x] tier2 > > Thanks for adding this asserts. If there is any reason to use `getInactive` > by other references in the future, we could remove these asserts at that time. I didn't notice this before it was integrated. The test for inactive isn't right; rather than `next == this` it should be `next != null`. This becomes apparent once FinalizerHistogram is fixed to call getInactive() rather than get(). I noticed this while working on JDK-8256517, where I ran into some similar issues. I will address these problems as part of that change. - PR: https://git.openjdk.java.net/jdk/pull/1231
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: 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
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: 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
