Re: RFR (XS) 8228352 : CANON_EQ breaks when pattern contains supplementary codepoint
Thank you Naoto for the review! With kind regards, Ivan On 8/1/19 10:02 AM, naoto.s...@oracle.com wrote: Hi Ivan, The change looks good to me. Naoto On 7/31/19 9:21 PM, Ivan Gerasimov wrote: Hello! Pattern.compile fails with IOOB when normalizing a pattern that contains a surrogate pair. Would you please help review a trivial 1-line fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8228352 WEBREV: http://cr.openjdk.java.net/~igerasim/8228352/00/webrev/ -- With kind regards, Ivan Gerasimov
Re: JDK 14 RFR of JDK-8202385: Annotation to mark serial-related fields and methods
Hi Joe,
It would be good to more closely define the semantics of the @Serial
annotation.
Suggestion for the first sentence:
"@Serial annotates each field or method specified by the Java
Object Serialization Specification of a {@linkplain Serializable
serializable} class."
This annotation type is intended to allow compile-time checking of
serialization-related declarations, analogous to the checking enabled by
the {@link java.lang.Override} annotation type to validate method
overriding.
It would be useful to describe that reflection is used to access and use
the fields and methods and they may otherwise appear to be unused.
A recommendation could be added in an @impleNote to apply @Serial to
each serialization defined method or field.
$02, Roger
On 7/13/19 1:16 PM, Joe Darcy wrote:
PS I've uploaded an updated an iteration of the webrev
http://cr.openjdk.java.net/~darcy/8202385.4/
to address. the syntactic concerns previously raised. I added
...defined by the Java Object Serialization
Specification...
which is the current name of the document and is similar to the style
of reference made in java.io.Serializable. Offhand, I didn't know of
the correct idiom to refer to the document as a working hyperlink, but
would be switch to that idiom.
Cheers,
-Joe
On 7/12/2019 8:19 PM, Joe Darcy wrote:
Hi Roger,
On 7/12/2019 1:31 PM, Roger Riggs wrote:
Hi Joe,
As an annotation on a field or method, this is a use site annotation.
It is an annotation intended for the declarations of fields and
methods of Serializable types.
From the description, the checks that could be added would only be done
if the annotation was present and the annotation is a tag for existing
fields and methods that are part of the serialization spec.
Right; the annotation is semantically only applicable to the fields
and methods associated with the serialization system.
The signatures of the fields and methods can be known to the
compiler independent
of the annotation and produce the same warnings.
So this looks like a case of trying to have belt and suspenders.
If the checks are not done when the annotation is not present, then
it will still be
the case that incorrect or misused fields and methods will still
escape detection.
Though the details of the compiler check are outside of the scope of
this annotation,
it seems unclear whether the annotation is necessary.
I have a prototype annotation processor to implement checks for
JDK-8202056: Expand serial warning to check for bad overloads of
serial-related methods and ineffectual fields
The current version of the processor does not assume the presence of
java.io.Serial. The summarize the existing checking methodology:
If a type is Serialiazable and a field or method has a name
matching the names of one of the special fields or methods to
serialization, check that the field or method has the required
modifiers, type, and, the the case of methods, parameter types and
exception types.
That is all well and good and represents a large fraction of the
checking of interest. However, it does not catch a mis-declaration
like "readobject" instead of "readObject". One could argue that
sufficiently thorough testing should catch that kind of error;
however, my impression is that thoroughness of testing is rare in
practice. I don't think it would be reasonable for javac to have some
kind of Hamming distance
(https://en.wikipedia.org/wiki/Hamming_distance) check between the
name of fields/methods and the name of the serialization related
fields methods to try to catch such mis-declarations. An annotation
like java.io.Serial is intended to allow the programmer to indicate
"yes, this is supposed to be one of the serialization related fields
or methods" and enable the compile to perform checks against that
category of error.
Can the name of the annotation be more descriptive?
Just "Serial" seems a bit too simple and does not have a strong
binding to the Serialization classes and specification.
Alternatives:
SerialMetadata
SerialControl
SerialFunction
From the earlier design iterations "Serial" was chosen to be
evocative of the "@serial" javadoc tag.
Thanks,
-Joe
39: There should be a reference to the serialization specification
for the definition
of the fields and methods to make it clear where the authoritative
identification is spec'd.
73-75: Please align the and tags on separate lines with
matching indentation.
77: Extra leading space.
Regards, Roger
On 7/9/19 7:14 PM, Joe Darcy wrote:
Hello,
Returning to some old work [1], please review the addition of a
java.io.Serial annotation type for JDK 14 to mark serial-related
fields and methods:
webrev: http://cr.openjdk.java.net/~darcy/8202385.3/
CSR: https://bugs.openjdk.java.net/browse/JDK-8217698
Thanks,
-Joe
[1] Previous review threads:
RFC 8228988: Handle annotation values with incompatible runtime type
Hello, I tried to fix a bug in AnnotationParser that throws a
NullPointerException if an annotation enum property was refactored to an
enumeration type or vice versa but retained its old name.
There is currently no check if an annotation property is consistent with
the runtime type, only the existance of the type is validated which causes
this null pointer.
I struggled to write a test for Jtreg as this requires quite a lot of setup
but I hope that it is quite straight-forward to see from the changed code
segments that the validation is missing.
Nevertheless, I am not sure if this is the exception that would be expected
from such a scenario.
Here is the diff file inlined:
Index:
src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===
---
src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
(revision 55752:8ae33203d600a7c9f9b2be9b31a0eb8197270ab1)
+++
src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java
(revision 55752+:8ae33203d600+)
@@ -358,6 +358,9 @@
result = parseConst(tag, buf, constPool);
}
+if (result == null)
+result = new AnnotationTypeMismatchExceptionProxy(
+memberType.getClass().getName());
if (!(result instanceof ExceptionProxy) &&
!memberType.isInstance(result))
result = new AnnotationTypeMismatchExceptionProxy(
@@ -470,7 +473,10 @@
int constNameIndex = buf.getShort() & 0x;
String constName = constPool.getUTF8At(constNameIndex);
-if (!typeName.endsWith(";")) {
+if (!enumType.isEnum())
+return new AnnotationTypeMismatchExceptionProxy(
+typeName + "." + constName);
+else if (!typeName.endsWith(";")) {
// support now-obsolete early jsr175-format class files.
if (!enumType.getName().equals(typeName))
return new AnnotationTypeMismatchExceptionProxy(
Re: RFR: 8158880: test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java fail with zh_CN locale
Looks good to me, Thejasvi. Naoto On 8/1/19 6:42 AM, Thejasvi Voniadka wrote: Hi, Request you to please review this change. JBS:https://bugs.openjdk.java.net/browse/JDK-8158880 (test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java fail with zh_CN locale) Description:The test " test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java# test_appendZoneText_parseGenericTimeZonePatterns" fails if run on certain locales (such as "ja"). At present the test does not enforce a specific locale for the DateTimeFormatter, and hence the machine's locale is used as the default. This could cause parsing mismatches when run on non-US locales. The fix is to set the locale explicitly in the test. Webrev:http://cr.openjdk.java.net/~vagarwal/8158880/webrev.0
RFR: 8158880: test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java fail with zh_CN locale
Hi, Request you to please review this change. JBS:https://bugs.openjdk.java.net/browse/JDK-8158880 (test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java fail with zh_CN locale) Description:The test " test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java# test_appendZoneText_parseGenericTimeZonePatterns" fails if run on certain locales (such as "ja"). At present the test does not enforce a specific locale for the DateTimeFormatter, and hence the machine's locale is used as the default. This could cause parsing mismatches when run on non-US locales. The fix is to set the locale explicitly in the test. Webrev:http://cr.openjdk.java.net/~vagarwal/8158880/webrev.0
Re: RFR: 8224974: Implement JEP 352
Hi Boris,
On 31/07/2019 13:01, Boris Ulasevich wrote:
> I did a quick check of the change across our platforms. Arm32 and x86_64
> built successfully. But I see it fails due to minor issues on aarch64
> and x86_32 with webrev.09.
> Can you please have a look at this?
>
>> src/hotspot/cpu/aarch64/aarch64.ad:2202:1: error: expected ‘;’ before
> ‘}’ token
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp:9925: undefined reference
> to `Assembler::clflush(Address)'
The AArch64 error was simply a missing semi-colon. With that corrected
AArch64 now builds and runs as expected (i.e. it fails the PMem support
test with an UnsupportedOperationException).
The second error is happening because the calling method
MacroAssembler::cache_wb has not been guarded with #ifdef _LP64 (the
same applies for MacroAssembler::cache_wbsync). Note that cache line
writeback via Unsafe.writeBackMemory is only expected to work on Intel
x86_64 so these two methods only get called from x86_64-specific code
(x86_64.ad and stuGenerator_x86_64.cpp).
So, the solution to this specific problem is to add #ifdef _LP64 around
the declaration and implementation of these two methods. At the same
time it would be helpful to remove the redundant #ifdef _LP64/#endif
that I for some strange reason inserted around the definitions, but not
the declarations, of clflushopt and clwb (that didn't help when I was
trying to work out what was going wrong).
However, a related problem also needs fixing. The Java code for method
Unsafe.writebackMemory only proceeds when the data cache line writeback
unit size (value of field UnsafeConstants.DATA_CACHE_LINE_FLUSH_SIZE) is
non-zero. Otherwise it throws an exception. On x86_32 that field /must/
be zero. The native methods which Unsafe calls out to and the intrinsics
which replace the native calls are not implemented on x86_32.
The field from which the value of the Java constant is derived is
currently initialised using CPU-specific information in
vm_version_x86.cpp as follows
if (os::supports_map_sync()) {
// publish data cache line flush size to generic field, otherwise
// let if default to zero thereby disabling writeback
_data_cache_line_flush_size =
_cpuid_info.std_cpuid1_ebx.bits.clflush_size * 8;
}
i.e. writeback is enabled on x86 when the operating is known to be
capable of supporting MAP_SYNC. os_linux.cpp returns true for that call,
irrespective of whether this is 32 or 64 bit linux. The rationale is
that any Linux is capable of supporting map_sync (by contrast Windows,
Solaris, AIX etc currently return false). So, the above assignment also
needs to be guarded by #ifdef _LP64 in order to ensure that writeback is
never attempted on x86_32.
Thank you for spotting these errors. I will add the relevant fixes to
the next patch and add you as a reviewer.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8224974: Implement JEP 352
On 7/31/19 12:55 PM, Andrew Dinn wrote: >> So if pre wbsync is no-op, why do we need to handle it everywhere? We seem >> to be falling through all >> the way to the stub to do nothing there, maybe we should instead cut much >> earlier, e.g. when >> inlining Unsafe.writeBackPresync0? Would it be better to not emit >> CacheWBPreSync at all? > > The pre sync is definitely not needed at present. However, I put it > there because I didn't know for sure if some future port of this > capability (e.g. to ppc) might need to sync prior writes before writing > back cache lines. [Indeed, the old Intel documentation stated that > pre-sync was needed on x86 for clflush to be safe but it is definitely > not needed.] I am more concerned that the writeback call enters the pre sync stub unnecessarily. I had the idea to do this more efficiently, and simplify code at the same time: how about emitting CacheWBPreSync nodes, but emitting nothing for them in .ad matchers? That would leave generic code generic, and architectures would then be able to avoid the stub altogether for pre sync code. This would simplify current stub generators too, I think: you don't need to pass arguments to them. This leaves calling via Unsafe. I believe pulling up the isPre choice to the stub generation time would be beneficial. That is, generate *two* stubs: StubRoutines::data_cache_writeback_pre_sync() and StubRoutines::data_cache_writeback_post_sync(). If arch does not need the pre_sync, generate nop version of pre_sync(). This is not a strong requirement from my side. I do believe it would make code a bit more straight-forward. >> === src/hotspot/cpu/x86/assembler_x86.cpp >> >> It feels like these comments are redundant, especially L8630 and L8646 which >> mention magic values >> "6" and "7", not present in the code: ... > 8624 // 0x66 is instruction prefix > > 8627 // 0x0f 0xAE is opcode family > > 8630 // rdi == 7 is extended opcode byte > . . . > > Given that the code is simply stuffing numbers (whether supplied as > literals or as symbolic constants) into a byte stream I think these > comments are a help when it comes to cross-checking each specific > assembly against the corresponding numbers declared in the Intel > manuals. So, I don't really want to remove them. Would you prefer me to > reverse the wording as above? I was merely commenting on the style: the rest of the file does not have comments like that. The positions of prefixes, opcode families, etc is kinda implied by the code shape. >> === src/hotspot/cpu/x86/macroAssembler_x86.cpp > // prefer clwb (potentially parallel writeback without evict) > // otherwise prefer clflushopt (potentially parallel writeback > // with evict) > // otherwise fallback on clflush (serial writeback with evict) > > In the second case the comment is redundant because the need for an > sfence is covered by the existing comment inside the if: > > // need an sfence for post flush when using clflushopt or clwb > // otherwise no no need for any synchroniaztion Yes, this would be good to add. >> === src/hotspot/cpu/x86/stubGenerator_x86_64.cpp >> >> Is it really "cmpl" here, not "cmpb"? I think aarch64 code tests for byte. >> >> 2942 __ cmpl(is_pre, 0); > > This is a Java boolean input. I believe that means the value will be > loaded into c_arg0 as an int so this test ought to be adequate. Okay. >> === src/hotspot/share/opto/c2compiler.cpp >> >> Why inject new cases here, instead of at the end of switch? Saves sudden >> "break": >> >> 578 break; >> 579 case vmIntrinsics::_writeback0: >> 580 if (!Matcher::match_rule_supported(Op_CacheWB)) return false; >> 581 break; >> 582 case vmIntrinsics::_writebackPreSync0: >> 583 if (!Matcher::match_rule_supported(Op_CacheWBPreSync)) return false; >> 584 break; >> 585 case vmIntrinsics::_writebackPostSync0: >> 586 if (!Matcher::match_rule_supported(Op_CacheWBPostSync)) return >> false; >> 587 break; > > I placed them here so they were close to the other Unsafe intrinsics. In > particular they precede _allocateInstance, an ordering which is also the > case in the declarations in vmSymbols.hpp. > > In what sense do you mean that an extra 'break' is saved? That would be > true as regards the textual layout. It wouldn't affect the logic of > folding different ranges of values into branching range tests (which is > only determined by the numeric values of the intrinsics). If you are > concerned about the former then I would argue that placing the values in > declaration order seems to me to be the more important concern. I don't think we have to follow whatever ordering mess in vmSymbols.hpp. New code cuts into the last case block in that switch, which is mostly about "we know about these symbols, they are falling-through to the break". Adding cases with Matcher::match_rule_supported seems odd there. If anything, those new cases should be moved upwards to other
Re: RFR: 8224974: Implement JEP 352
Hello Dmitry,
On 01/08/2019 05:09, Dmitry Chuyko wrote:
> Just a small comment about the tests. As you can see, some of tests in
> jdk/java/nio/channels/FileChannel check various modes, arguments, and
> expected exceptions. E.g. see MapTest, Mode and Args.
>
> I noticed that in your changes for the new map mode there are new
> exceptions thrown in different situations. For example, when the
> required mode is not supported, or it does not fit. In particular, this
> should happen correctly on systems that do not support nvram. Have you
> considered the possibility of extending or adding tests for this behavior?
I agree that these failure cases need better test coverage and
automation. However, that is not to say they have not been tested. All
current success and failure cases can be exercised by the current mmap
test (PmemTest) if run in the correct circumstance. Unfortunately,
automatic testing is not straightforward.
1) On x86_64 where MAP_SYNC is supported test PMemTest includes
instructions to exercise a successful map from a real or emulated DAX
file system. It can also be used (and has been used) to exercise the
IOException failure path in the case where the file it tries to open
belongs to a non-DAX file system (see line 99).
Note that testing for success or failure cannot be done automatically
using the test as it currently stands. Testing for a successful map
requires mounting a real or emulated DAX file system at a known mount
point and creating a writable dir to hold the mapped file. Testing for
the IOException failure requires either setting up an equivalent non-DAX
file system mount or editing the test to open the file in a non-DAX file
system dir like, say, /tmp.
A new, separate test for the IOException failure could be automated on
x86_64 by trying to map a file located in /tmp (on the assumption that
/tmp is not mounted as a DAX file system). Of course, at present this
will only give the IOException result when MAP_SYNC is supported. Given
a suitably old Linux/x86_64, UnsupportedOperationException could also be
thrown.
2) On AArch64 where MAP_SYNC is not currently supported PmemTest clearly
cannot be used to exercise the success path. However, it can be used
(and has been used) to exercise the UnsupportedOperationException
failure path.
A check for the UnsupportedOperationException failure could be automated
on AArch64 by a new test as described above. Of course, once MAP_SYNC
support arrives in a new Linux/AArch64 it would also become for
IOException to be thrown.
So, to sum up, it would be possible to add a new, automatic test that
checks for one or other failure occurring. I am happy to add such a test
to the next webrev.
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re[2]: The final optimized version of Dual-Pivot Quicksort (ver.19.1)
Hi Brent, see my comments inline: >Четверг, 1 августа 2019, 9:23 +03:00 от Laurent Bourgès >: > >Hi Brent, > >Le jeu. 1 août 2019 à 02:32, Brent Christian < brent.christ...@oracle.com > a >écrit : >>Thanks, Laurent. I can sponsor this fix, get a RFR thread going for >>JDK-8226297, keep webrevs updated as the review progresses, etc. Thank you for review! > >Excellent, I will let you and Vladimir work on this review. > >FYI I have implemented DPQS (int[] ) sorting 2 arrays (values + indices) in >the Marlin renderer. It could be generalized to any array type and having the >index array is very important in many algorithms... > >> >>However I've uncovered an issue with the fix that should be resolved >>before proceeding. >> >>Specifically, the new Arrays.COMMON_PARALLELISM static constant causes >>exceptions at startup under some circumstances: >> * running with LANG=C on Linux[1] >> * setting a property value with non-Ascii characters on Mac[2] >> >>java.util.Arrays is used a fair bit for String handling >>(java.lang.StringCoding, java.langStringLatin1, etc). The problem is >>that early in startup, depending on the locale/language setup and/or >>property settings, java.util.Arrays can be called during initialization >>of system properties. >> >>During Arrays., COMMON_PARALLELISM is setup with a call to >>ForkJoinPool.getCommonPoolParallelism(). ForkJoinPool sets up >>some VarHandles, VarHandles leads to >>MethodHandleStatics., which reads some system properties. But >>we're still initializing the system properties - 'props' is null, so NPE. > >Chicken-egg problem. Maybe this field could be wrapped in a getter doing lazy >resolution... >> >>I think Arrays.java needs to remove the COMMON_PARALLELISM constant, and >>go back to making calls to ForkJoinPool.getCommonPoolParallelism(). > >I do not know why Vladimir changed that recently. Any good reason ? >> >>I can do the update and post an updated webrev (unless further >>discussion is needed). Yes, please, remove COMMON_PARALLELISM constant and replace by call ForkJoinPool.getCommonPoolParallelism(). in parallelSort() methods. Please, go ahead and update webrev with corrected Arrays.java >> >PS: I can help on benchmarking as I developed a fair sort benchmark based on >JMH: >https://github.com/bourgesl/nearly-optimal-mergesort-code > >JMH test code: >https://github.com/bourgesl/nearly-optimal-mergesort-code/tree/master/sort-bench/src/main/java/edu/sorting/bench >I would be interested by improving the perf test code and possibly integrate >it in OpenJDK JMH test suite... (later) > >Goodbye & good luck, >Laurent -- Vladimir Yaroslavskiy
Re: The final optimized version of Dual-Pivot Quicksort (ver.19.1)
Hi Brent, Le jeu. 1 août 2019 à 02:32, Brent Christian a écrit : > Thanks, Laurent. I can sponsor this fix, get a RFR thread going for > JDK-8226297, keep webrevs updated as the review progresses, etc. > Excellent, I will let you and Vladimir work on this review. FYI I have implemented DPQS (int[] ) sorting 2 arrays (values + indices) in the Marlin renderer. It could be generalized to any array type and having the index array is very important in many algorithms... > However I've uncovered an issue with the fix that should be resolved > before proceeding. > > Specifically, the new Arrays.COMMON_PARALLELISM static constant causes > exceptions at startup under some circumstances: > * running with LANG=C on Linux[1] > * setting a property value with non-Ascii characters on Mac[2] > > java.util.Arrays is used a fair bit for String handling > (java.lang.StringCoding, java.langStringLatin1, etc). The problem is > that early in startup, depending on the locale/language setup and/or > property settings, java.util.Arrays can be called during initialization > of system properties. > > During Arrays., COMMON_PARALLELISM is setup with a call to > ForkJoinPool.getCommonPoolParallelism(). ForkJoinPool sets up > some VarHandles, VarHandles leads to > MethodHandleStatics., which reads some system properties. But > we're still initializing the system properties - 'props' is null, so NPE. > Chicken-egg problem. Maybe this field could be wrapped in a getter doing lazy resolution... > I think Arrays.java needs to remove the COMMON_PARALLELISM constant, and > go back to making calls to ForkJoinPool.getCommonPoolParallelism(). > I do not know why Vladimir changed that recently. Any good reason ? > I can do the update and post an updated webrev (unless further > discussion is needed). > PS: I can help on benchmarking as I developped a fair sort benchmark based on JMH: https://github.com/bourgesl/nearly-optimal-mergesort-code JMH test code: https://github.com/bourgesl/nearly-optimal-mergesort-code/tree/master/sort-bench/src/main/java/edu/sorting/bench I would be interested by improving the perf test code and possibly integrate it in OpenJDK JMH test suite... (later) > Goodbye & good luck, Laurent
