[7u-dev ONLY] RFR: [8057530] (process) Runtime.exec throws garbled message in jp locale

[Ivan Gerasimov]

Hello everybody!

This is a request for review of a fix which is applicable to 7u only.
The issue has already been addressed in 8 and later releases.

The fix is a combined backport of
https://bugs.openjdk.java.net/browse/JDK-8016579 , which addresses the 
problem with the encoding in windows console

and partial backport of
https://bugs.openjdk.java.net/browse/JDK-8001334 , which reorganizes the 
code at the files of our interest.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8057530
WEBREV: http://cr.openjdk.java.net/~igerasim/8057530/0/webrev/

The fix has been verified on the localized versions of Windows (jp, rus).

Thanks in advance,
Ivan

Re: Precision for JDK-8060012 Class.isAnonymousClass() returns false when class major number is 48.

[Joe Darcy]

Hello,

I don't know if IBM's implementation uses any VM hooks not found in 
OpenJDK. If their JDK only uses common reflection machinery, a patch for 
this functionality for OpenJDK would be welcome for evaluation.


Cheers,

-Joe

On 10/14/2014 4:17 AM, Alexandre Bartel wrote:

Hi,

Regarding issue JDK-8060012
https://bugs.openjdk.java.net/browse/JDK-8060012

I have tried to run the code using the following IBM JVM:

java version "1.7.0"
Java(TM) SE Runtime Environment (build pxa6470_27-20131115_04)
IBM J9 VM (build 2.7, JRE 1.7.0 Linux amd64-64 Compressed References
20131114_175264 (JIT enabled, AOT enabled)
J9VM - R27_Java727_GA_20131114_0833_B175264
JIT  - tr.r13.java_20131113_50523
GC   - R27_Java727_GA_20131114_0833_B175264_CMPRSS
J9CL - 20131114_175264)
JCL - 20131113_01 based on Oracle 7u45-b18

And the result is always the expected result:

isAnonymousClass() always returns true, as expected, even when the
major version of the A$1 class is 48. This suggests that there is
enough information in classes with version 48 to know if the class is
anonymous and that OpenJDK is potentially not analyzing the class
correctly.

Thanks,
/Alexandre

Re: [PATCH] Return -1 after throwing internal error

[Mandy Chung]

Hi Xiaoguang,

On 10/20/2014 6:31 PM, Xiaoguang Sun wrote:

Hi All,

I recently discovered some inconsistency in UnixOperatingSystem_md.c
that do now return -1 after throwing internal error. It usually
shouldn't be a problem, but making it more consistent to other code
within the same file shouldn't be a bad idea.


I'm including serviceability-dev for your patch as UnixOperatingSystem_md.c
belongs to serviceability.

Are you working on a clone of jdk9/dev repo?  
src/solaris/native/com/sun/management/UnixOperatingSystem_md.c looks like jdk7 
source.

It has been renamed to 
src/java.management/unix/native/libmanagement/OperatingSystemImpl.c [1]. Can 
you rebase your patch to the latest jdk9 source and
send it to serviceability-dev?

Mandy
[1] http://hg.openjdk.java.net/jdk9/dev/jdk/

[PATCH] Return -1 after throwing internal error

[Xiaoguang Sun]

Hi All,

I recently discovered some inconsistency in UnixOperatingSystem_md.c
that do now return -1 after throwing internal error. It usually
shouldn't be a problem, but making it more consistent to other code
within the same file shouldn't be a bad idea.

Xiaoguang
exporting patch:
# HG changeset patch
# User Xiaoguang Sun 
# Date 1413212027 -28800
#  Mon Oct 13 22:53:47 2014 +0800
# Node ID 30570cba688d636a9ad6e8ed0a627ac0cf402867
# Parent  992b39afdcb97fa44547d844ce57a32f3e52bd7a
Return -1 after throwing internal error

diff -r 992b39afdcb9 -r 30570cba688d src/solaris/native/com/sun/management/UnixOperatingSystem_md.c
--- a/src/solaris/native/com/sun/management/UnixOperatingSystem_md.c	Thu Jun 13 09:48:58 2013 -0700
+++ b/src/solaris/native/com/sun/management/UnixOperatingSystem_md.c	Mon Oct 13 22:53:47 2014 +0800
@@ -152,6 +152,7 @@
 ret = sysinfo(&si);
 if (ret != 0) {
 throw_internal_error(env, "sysinfo failed to get swap size");
+return -1;
 }
 total = (jlong)si.totalswap * si.mem_unit;
 avail = (jlong)si.freeswap * si.mem_unit;
@@ -162,6 +163,7 @@
 size_t size = sizeof(vmusage);
 if (sysctlbyname("vm.swapusage", &vmusage, &size, NULL, 0) != 0) {
 throw_internal_error(env, "sysctlbyname failed");
+return -1;
 }
 return available ? (jlong)vmusage.xsu_avail : (jlong)vmusage.xsu_total;
 #else /* _ALLBSD_SOURCE */
@@ -237,6 +239,7 @@
 kern_return_t res = task_info(mach_task_self(), TASK_BASIC_INFO, (task_info_t)&t_info, &t_info_count);
 if (res != KERN_SUCCESS) {
 throw_internal_error(env, "task_info failed");
+return -1;
 }
 return t_info.virtual_size;
 #else /* _ALLBSD_SOURCE */

Re: Review request: JDK-8055723 Replace concat String to append in StringBuilder parameters

[Ulf Zibis]

Am 21.10.2014 um 01:02 schrieb Otávio Gonçalves de Santana:

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8055723


WEBREV: http://cr.openjdk.java.net/~weijun/8055723/client/webrev.02/
WEBREV: http://cr.openjdk.java.net/~weijun/8055723/core/webrev.03/


I did not look through all sources.
In Scanner.java I discovered:
1307 sb.append("[delimiters=").append(delimPattern).append(']');
1308 sb.append("[position=").append(position).append(']');
...
Maybe better:
1307 sb.append("[delimiters=").append(delimPattern);
1308 sb.append("][position=").append(position);
...

-Ulf

Review request: JDK-8055723 Replace concat String to append in StringBuilder parameters

[Otávio Gonçalves de Santana]

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8055723


WEBREV: http://cr.openjdk.java.net/~weijun/8055723/client/webrev.02/
WEBREV: http://cr.openjdk.java.net/~weijun/8055723/core/webrev.03/

-- 

Otávio Gonçalves de Santana

blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
55 (11) 98255-3513

Re: RFR JDK-6321472: Add CRC-32C API

[Staffan Friberg]

Hi David,


The intrinsic that can be implemented for CRC32C will probably be even faster 
as it can make use of the specific crc32c instruction instead of using carry 
less multiplication.

It might not hurt to mock up a quick benchmark using the CRC32C
instruction against jdk8’s java.util.zip.CRC32 intrinsic.  CRC32
seems to consume 64 bits per instruction, and I think each instruction
depends on its predecessor (naively coded, at least; there’s probably
a hack to run them in parallel, just as there is a hack that allows fork/join.
So maybe we should think about that hack, too; simply computing the
two half-CRCs in parallel, then combining them as if fork/join, might
break the data dependence and allow higher speed).

The carryless multiply inner loop also consumes 64 bits per carryless
multiply, but the instructions are not data dependent among themselves,
so it might go faster.  Haswell chips execute clmul especially quickly.

The assembly language I wrote to make that go fast is agnostic about P;
it is parameterized by a structure that contains a small number of constants
that make it all go (6 64-bit numbers) based on powers of x (regarded as a
polynomial) modulo the CRC polynomial P.  If the endianness conventions
match, and if zip CRC runs faster than the CRC32 instruction, I could
revive it, and modern tools might let us avoid coding in hex.
Yes, when an intrinsic is added it will definitely need to be compared 
the the current crc32. The calculation speed of the two should at least 
be the same, and hopefully we will be able to get crc32c even faster.  
Here is a paper describing a algorithm that uses the Intel crc32 
instruction, http://dl.acm.org/citation.cfm?id=2108885, and utilizes the 
fact that it is a 3 cycle instruction to execute 3 independent crc32 
instructions. (Presume it is using a similar algorithm to merge the 
results as you do in your fork/join implementation.)



I have also been thinking that using fork-join to help for large arrays. I 
decided to not go after it for this first implementation of the new API. The 
other question is if that should be something that is more explicit, similar to 
parallel streams, as not everyone might want/expect that the API to suddenly 
start using multiple threads to calculate the result.

That’s why we have a system fork/join pool; if you don’t want lots of 
concurrency,
limit the number of threads in that pool.  Soon enough people are going to
yell at us for not using concurrency when it is available, and forcing them to
use a different API just to get concurrency will hinder the ability to write 
portable
code.


We definiately should try to do a parallel version that can further 
speed up Checksum's on very large arrays/buffers, but we probably need 
to discuss further and get input on how it should be enabled. Do we need 
to add an optional parallel method or do we simply make the current 
method parallel? I believe that is what needs to be discussed and 
decided on a more general level, as we might want to parallelize other 
APIs as well when possible and preferably should follow the same 
principle across the whole Java standard library.


//Staffan

Re: RFR 8023173: FileOutputStream(FileDescriptor) does not respect append flag on Windows

[roger riggs]

Looks fine.  Thanks, Roger

On 10/20/2014 4:25 PM, Ivan Gerasimov wrote:

Thank you Roger!

Yes, you're right.
Please find the updated webrev here:
http://cr.openjdk.java.net/~igerasim/8023173/2/webrev/

Sincerely yours,
Ivan


On 20.10.2014 17:20, roger riggs wrote:

Hi Ivan,

This webrev appears removes the ability to interpose on various method
using byte-code injection.  For example, FileOutputStream.write(byte).
Do *not *replace delete the Java method calls that call native.
It looks like an optimization but disables some functions that allow 
monitoring

of I/O activities such as JFR.

Thanks, Roger


On 10/20/2014 8:34 AM, Ivan Gerasimov wrote:

Thank you Alan for the review!

On 17.10.2014 13:00, Alan Bateman wrote:

On 06/10/2014 11:41, Ivan Gerasimov wrote:

Hello everybody!

The append mode is emulated on Windows, therefore we have to keep 
a flag indicating that.


With the current implementation, the FileDescriptor does not know 
if the file were opened with O_APPEND, and the flag is maintained 
at the upper level.
This can cause inconsistency, when the FileDescriptor is reused to 
construct a new FileOutputStream, as there is no information 
available about the append/non-append mode.


Even though the solution is quite straight-forward: moving the 
flag from FileOutputStream,  FileDispatcherImpl and 
FileChannelImpl to the lower level of FileDescriptor, it touches 
20 files across the source-code base.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8023173
WEBREV: http://cr.openjdk.java.net/~igerasim/8023173/0/webrev/

With the fix, all the io/nio tests, including the new one, pass on 
all available platforms.


Would you please help review this fix?
Martin prodded me a few times but I was busy with other things and 
only getting to this now.


At a high level then moving the append down to FileDescriptor make 
sense but I have a few concerns.


On Unix systems then it looks like getAppend is calling into the 
ioctl to get the file descriptor flags.
If I read it correctly then it creates several problems for the 
usage in FileChannelImpl.position.


I thought it might look like a data duplication, if we store the 
append flag both in FileChannelImpl and FileDescriptor.

Though, we surely can retrieve the append flag only once and cache it.

I updated the webrev with this change.

I just wondering if you consider just leaving the append flag there 
and just retrieve the value once in the open method.




I'm also wondering about the handleWrite implementation on Windows 
which changes to use additional JNI to retrieve the value of the 
append flag each time. We should try to avoid this (we want the 
native methods to be as simple as possible so that we can replace 
them in the future) so there may be an argument for passing that 
down as per the current implementation.




We would still need to retrieve the flag from native code.
For example, the platform specific handleWrite() is called from the 
native FileOutputStream.writeBytes(), which is called from 
FileOutputStream.writt(), which is common for all the platforms.
Thus, we should either retrieve the append flag from the Java code 
for any platform, and ignore it later on Unix, or read the flag in 
the native Windows-only code.


Alternatively, we could separate implementations of 
FileOutputStream.java for different platforms, but that would 
complicate everything.


Hopefully, in the future we could find a way to use 
FILE_APPEND_DATA, so FileDescriptor.append can be removed altogether 
with that corresponding JNI code.


Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8023173/1/webrev/

I've also included a few typo fixes left after JDK-8059840.

Sincerely yours,
Ivan

Re: RFR 8023173: FileOutputStream(FileDescriptor) does not respect append flag on Windows

[Ivan Gerasimov]

Thank you Roger!

Yes, you're right.
Please find the updated webrev here:
http://cr.openjdk.java.net/~igerasim/8023173/2/webrev/

Sincerely yours,
Ivan


On 20.10.2014 17:20, roger riggs wrote:

Hi Ivan,

This webrev appears removes the ability to interpose on various method
using byte-code injection.  For example, FileOutputStream.write(byte).
Do *not *replace delete the Java method calls that call native.
It looks like an optimization but disables some functions that allow 
monitoring

of I/O activities such as JFR.

Thanks, Roger


On 10/20/2014 8:34 AM, Ivan Gerasimov wrote:

Thank you Alan for the review!

On 17.10.2014 13:00, Alan Bateman wrote:

On 06/10/2014 11:41, Ivan Gerasimov wrote:

Hello everybody!

The append mode is emulated on Windows, therefore we have to keep a 
flag indicating that.


With the current implementation, the FileDescriptor does not know 
if the file were opened with O_APPEND, and the flag is maintained 
at the upper level.
This can cause inconsistency, when the FileDescriptor is reused to 
construct a new FileOutputStream, as there is no information 
available about the append/non-append mode.


Even though the solution is quite straight-forward: moving the flag 
from FileOutputStream,  FileDispatcherImpl and FileChannelImpl to 
the lower level of FileDescriptor, it touches 20 files across the 
source-code base.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8023173
WEBREV: http://cr.openjdk.java.net/~igerasim/8023173/0/webrev/

With the fix, all the io/nio tests, including the new one, pass on 
all available platforms.


Would you please help review this fix?
Martin prodded me a few times but I was busy with other things and 
only getting to this now.


At a high level then moving the append down to FileDescriptor make 
sense but I have a few concerns.


On Unix systems then it looks like getAppend is calling into the 
ioctl to get the file descriptor flags.
If I read it correctly then it creates several problems for the 
usage in FileChannelImpl.position.


I thought it might look like a data duplication, if we store the 
append flag both in FileChannelImpl and FileDescriptor.

Though, we surely can retrieve the append flag only once and cache it.

I updated the webrev with this change.

I just wondering if you consider just leaving the append flag there 
and just retrieve the value once in the open method.




I'm also wondering about the handleWrite implementation on Windows 
which changes to use additional JNI to retrieve the value of the 
append flag each time. We should try to avoid this (we want the 
native methods to be as simple as possible so that we can replace 
them in the future) so there may be an argument for passing that 
down as per the current implementation.




We would still need to retrieve the flag from native code.
For example, the platform specific handleWrite() is called from the 
native FileOutputStream.writeBytes(), which is called from 
FileOutputStream.writt(), which is common for all the platforms.
Thus, we should either retrieve the append flag from the Java code 
for any platform, and ignore it later on Unix, or read the flag in 
the native Windows-only code.


Alternatively, we could separate implementations of 
FileOutputStream.java for different platforms, but that would 
complicate everything.


Hopefully, in the future we could find a way to use FILE_APPEND_DATA, 
so FileDescriptor.append can be removed altogether with that 
corresponding JNI code.


Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8023173/1/webrev/

I've also included a few typo fixes left after JDK-8059840.

Sincerely yours,
Ivan

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[Alan Bateman]

On 20/10/2014 20:11, roger riggs wrote:

Hi,

Updated with recommendations to make the code more readable and
to use the more appropriate Integer.parseInt instead of valueOf.


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
https://bugs.openjdk.java.net/browse/JDK-8048124
This looks okay now although I guess the other clean-ups are really 
something for a different issue.


You want to want to check Integer.java in the webrev, I assumed that 
isn't meant to be there.


-Alan.

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[roger riggs]

Hi,

Updated with recommendations to make the code more readable and
to use the more appropriate Integer.parseInt instead of valueOf.


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
https://bugs.openjdk.java.net/browse/JDK-8048124

Roger


On 10/20/2014 2:53 PM, Alan Bateman wrote:

On 20/10/2014 19:17, roger riggs wrote:

Hi,

Updated with recommendations.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
https://bugs.openjdk.java.net/browse/JDK-8048124

"<>" seems excessive when I think you only need 
${java.home}/- but still better than all.
yes, reading the java.home property required a doPriv and then a nested 
doPriv with

the properly constructed  FilePermission.  The code was getting unwieldy.


A minor comment is that expression for the try-with-resources uses 
AccessController with parameters over 4 lines isn't easy to read. It 
might be a bit clearer with multiple statements, as in:


PrivilegedAction action = () -> HijrahChronology 
.class.getResourceAsStream(resourceName);

Permission permission = new FilePermission("<>", "read");
try (InputStream in = AccessController.doPrivileged(action, null, 
permission)) {

:
}

Yes, easier to read and parse.

Thanks, Roger

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[Alan Bateman]

On 20/10/2014 19:17, roger riggs wrote:

Hi,

Updated with recommendations.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
https://bugs.openjdk.java.net/browse/JDK-8048124

"<>" seems excessive when I think you only need 
${java.home}/- but still better than all.


A minor comment is that expression for the try-with-resources uses 
AccessController with parameters over 4 lines isn't easy to read. It 
might be a bit clearer with multiple statements, as in:


PrivilegedAction action = () -> HijrahChronology 
.class.getResourceAsStream(resourceName);

Permission permission = new FilePermission("<>", "read");
try (InputStream in = AccessController.doPrivileged(action, null, 
permission)) {

:
}

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[Stanimir Simeonoff]

Hi,
A minor issue: there are quite a few instances of parsing integers via
Integer.valueOf(String) instead just Integer.parseInt(String):

HijrahChronology.java:
864 int year = Integer.valueOf(key);

972 months[i] = Integer.valueOf(numbers[i]);


Those should be optimized not to create substrings either
 994 ymd[0] = Integer.valueOf(string.substring(0, 4));
 995 ymd[1] = Integer.valueOf(string.substring(5, 7));
 996 ymd[2] = Integer.valueOf(string.substring(8, 10));



Stanimir

On Mon, Oct 20, 2014 at 9:17 PM, roger riggs  wrote:

> Hi,
>
> Updated with recommendations.
>
> Webrev:
> http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8048124
>
> Thanks, Roger
>
>
> On 10/20/2014 11:25 AM, Alan Bateman wrote:
>
>> On 20/10/2014 16:11, roger riggs wrote:
>>
>>> What permission would be needed to read the resource?
>>> The limited doPrivileged should include the minimum permission.
>>>
>> The resources will be be resources.jar so I think read access to that
>> should be sufficient. If you run a small test with -Djava.security.manager
>> that triggers the config file to load then it would help verify that. When
>> we move to the modular image then the resources will be elsewhere in the
>> runtime image so if you really want to use limited doPrivileged here then
>> access to ${java.home}/** should do it. Of course not using limited
>> doPrivileged is a possibility too.
>>
>> -Alan
>>
>>
>

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[roger riggs]

Hi,

Updated with recommendations.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
https://bugs.openjdk.java.net/browse/JDK-8048124

Thanks, Roger

On 10/20/2014 11:25 AM, Alan Bateman wrote:

On 20/10/2014 16:11, roger riggs wrote:

What permission would be needed to read the resource?
The limited doPrivileged should include the minimum permission.
The resources will be be resources.jar so I think read access to that 
should be sufficient. If you run a small test with 
-Djava.security.manager that triggers the config file to load then it 
would help verify that. When we move to the modular image then the 
resources will be elsewhere in the runtime image so if you really want 
to use limited doPrivileged here then access to ${java.home}/** should 
do it. Of course not using limited doPrivileged is a possibility too.


-Alan

Precision for JDK-8060012 Class.isAnonymousClass() returns false when class major number is 48.

[Alexandre Bartel]

Hi,

Regarding issue JDK-8060012
https://bugs.openjdk.java.net/browse/JDK-8060012

I have tried to run the code using the following IBM JVM:

java version "1.7.0"
Java(TM) SE Runtime Environment (build pxa6470_27-20131115_04)
IBM J9 VM (build 2.7, JRE 1.7.0 Linux amd64-64 Compressed References
20131114_175264 (JIT enabled, AOT enabled)
J9VM - R27_Java727_GA_20131114_0833_B175264
JIT  - tr.r13.java_20131113_50523
GC   - R27_Java727_GA_20131114_0833_B175264_CMPRSS
J9CL - 20131114_175264)
JCL - 20131113_01 based on Oracle 7u45-b18

And the result is always the expected result:

isAnonymousClass() always returns true, as expected, even when the
major version of the A$1 class is 48. This suggests that there is
enough information in classes with version 48 to know if the class is
anonymous and that OpenJDK is potentially not analyzing the class
correctly.

Thanks,
/Alexandre

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[Alan Bateman]

On 20/10/2014 16:11, roger riggs wrote:

What permission would be needed to read the resource?
The limited doPrivileged should include the minimum permission.
The resources will be be resources.jar so I think read access to that 
should be sufficient. If you run a small test with 
-Djava.security.manager that triggers the config file to load then it 
would help verify that. When we move to the modular image then the 
resources will be elsewhere in the runtime image so if you really want 
to use limited doPrivileged here then access to ${java.home}/** should 
do it. Of course not using limited doPrivileged is a possibility too.


-Alan

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[roger riggs]

Hi Alan,

Thanks for the review...

On 10/20/2014 11:07 AM, Alan Bateman wrote:

On 20/10/2014 15:53, roger riggs wrote:

Please review for JDK 9 only.

To aid the modularization effort, the configuration of the Hijrah 
calendar

should move the Hijrah calendar data to a resource.

At this point, it does not look like there will be other Hijrah calendar
variants; the function of calendar.properties to configure variants
is unnecessary and is proposed to be removed.

Since the other uses of calendars.properties have been eliminated
the calendars.properties is removed.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
  https://bugs.openjdk.java.net/browse/JDK-8048124
This mostly looks good to me. I just wonder about the removal of the 
doPrivileged in readConfigProperties where you will get null if there 
is something on the stack with restricted permissions.

What permission would be needed to read the resource?
The limited doPrivileged should include the minimum permission.


A minor comment on the @implNote in HijrahChronology is that it has 
more than I would expect, it might be simpler to just say that 
hijrah-config-.properties is loaded as a resource file. 
Also the import of java.lang.String looks unnecessary.

ok, will correct.

Roger



-Alan.

Re: RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[Alan Bateman]

On 20/10/2014 15:53, roger riggs wrote:

Please review for JDK 9 only.

To aid the modularization effort, the configuration of the Hijrah 
calendar

should move the Hijrah calendar data to a resource.

At this point, it does not look like there will be other Hijrah calendar
variants; the function of calendar.properties to configure variants
is unnecessary and is proposed to be removed.

Since the other uses of calendars.properties have been eliminated
the calendars.properties is removed.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
  https://bugs.openjdk.java.net/browse/JDK-8048124
This mostly looks good to me. I just wonder about the removal of the 
doPrivileged in readConfigProperties where you will get null if there is 
something on the stack with restricted permissions.


A minor comment on the @implNote in HijrahChronology is that it has more 
than I would expect, it might be simpler to just say that 
hijrah-config-.properties is loaded as a resource file. 
Also the import of java.lang.String looks unnecessary.


-Alan.

RFR 9: 8048124 Read hijrah-config-umalqura.properties as a resource

[roger riggs]

Please review for JDK 9 only.

To aid the modularization effort, the configuration of the Hijrah calendar
should move the Hijrah calendar data to a resource.

At this point, it does not look like there will be other Hijrah calendar
variants; the function of calendar.properties to configure variants
is unnecessary and is proposed to be removed.

Since the other uses of calendars.properties have been eliminated
the calendars.properties is removed.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-hijrah-config-8049376/
Issue:
  https://bugs.openjdk.java.net/browse/JDK-8048124

Thanks, Roger

Re: RFR 8023173: FileOutputStream(FileDescriptor) does not respect append flag on Windows

[roger riggs]

Hi Ivan,

This webrev appears removes the ability to interpose on various method
using byte-code injection.  For example, FileOutputStream.write(byte).
Do *not *replace delete the Java method calls that call native.
It looks like an optimization but disables some functions that allow 
monitoring

of I/O activities such as JFR.

Thanks, Roger


On 10/20/2014 8:34 AM, Ivan Gerasimov wrote:

Thank you Alan for the review!

On 17.10.2014 13:00, Alan Bateman wrote:

On 06/10/2014 11:41, Ivan Gerasimov wrote:

Hello everybody!

The append mode is emulated on Windows, therefore we have to keep a 
flag indicating that.


With the current implementation, the FileDescriptor does not know if 
the file were opened with O_APPEND, and the flag is maintained at 
the upper level.
This can cause inconsistency, when the FileDescriptor is reused to 
construct a new FileOutputStream, as there is no information 
available about the append/non-append mode.


Even though the solution is quite straight-forward: moving the flag 
from FileOutputStream,  FileDispatcherImpl and FileChannelImpl to 
the lower level of FileDescriptor, it touches 20 files across the 
source-code base.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8023173
WEBREV: http://cr.openjdk.java.net/~igerasim/8023173/0/webrev/

With the fix, all the io/nio tests, including the new one, pass on 
all available platforms.


Would you please help review this fix?
Martin prodded me a few times but I was busy with other things and 
only getting to this now.


At a high level then moving the append down to FileDescriptor make 
sense but I have a few concerns.


On Unix systems then it looks like getAppend is calling into the 
ioctl to get the file descriptor flags.
If I read it correctly then it creates several problems for the usage 
in FileChannelImpl.position.


I thought it might look like a data duplication, if we store the 
append flag both in FileChannelImpl and FileDescriptor.

Though, we surely can retrieve the append flag only once and cache it.

I updated the webrev with this change.

I just wondering if you consider just leaving the append flag there 
and just retrieve the value once in the open method.




I'm also wondering about the handleWrite implementation on Windows 
which changes to use additional JNI to retrieve the value of the 
append flag each time. We should try to avoid this (we want the 
native methods to be as simple as possible so that we can replace 
them in the future) so there may be an argument for passing that down 
as per the current implementation.




We would still need to retrieve the flag from native code.
For example, the platform specific handleWrite() is called from the 
native FileOutputStream.writeBytes(), which is called from 
FileOutputStream.writt(), which is common for all the platforms.
Thus, we should either retrieve the append flag from the Java code for 
any platform, and ignore it later on Unix, or read the flag in the 
native Windows-only code.


Alternatively, we could separate implementations of 
FileOutputStream.java for different platforms, but that would 
complicate everything.


Hopefully, in the future we could find a way to use FILE_APPEND_DATA, 
so FileDescriptor.append can be removed altogether with that 
corresponding JNI code.


Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8023173/1/webrev/

I've also included a few typo fixes left after JDK-8059840.

Sincerely yours,
Ivan

Re: RFR 8023173: FileOutputStream(FileDescriptor) does not respect append flag on Windows

[Ivan Gerasimov]

Thank you Alan for the review!

On 17.10.2014 13:00, Alan Bateman wrote:

On 06/10/2014 11:41, Ivan Gerasimov wrote:

Hello everybody!

The append mode is emulated on Windows, therefore we have to keep a 
flag indicating that.


With the current implementation, the FileDescriptor does not know if 
the file were opened with O_APPEND, and the flag is maintained at the 
upper level.
This can cause inconsistency, when the FileDescriptor is reused to 
construct a new FileOutputStream, as there is no information 
available about the append/non-append mode.


Even though the solution is quite straight-forward: moving the flag 
from FileOutputStream,  FileDispatcherImpl and FileChannelImpl to the 
lower level of FileDescriptor, it touches 20 files across the 
source-code base.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8023173
WEBREV: http://cr.openjdk.java.net/~igerasim/8023173/0/webrev/

With the fix, all the io/nio tests, including the new one, pass on 
all available platforms.


Would you please help review this fix?
Martin prodded me a few times but I was busy with other things and 
only getting to this now.


At a high level then moving the append down to FileDescriptor make 
sense but I have a few concerns.


On Unix systems then it looks like getAppend is calling into the ioctl 
to get the file descriptor flags.
If I read it correctly then it creates several problems for the usage 
in FileChannelImpl.position.


I thought it might look like a data duplication, if we store the append 
flag both in FileChannelImpl and FileDescriptor.

Though, we surely can retrieve the append flag only once and cache it.

I updated the webrev with this change.

I just wondering if you consider just leaving the append flag there 
and just retrieve the value once in the open method.




I'm also wondering about the handleWrite implementation on Windows 
which changes to use additional JNI to retrieve the value of the 
append flag each time. We should try to avoid this (we want the native 
methods to be as simple as possible so that we can replace them in the 
future) so there may be an argument for passing that down as per the 
current implementation.




We would still need to retrieve the flag from native code.
For example, the platform specific handleWrite() is called from the 
native FileOutputStream.writeBytes(), which is called from 
FileOutputStream.writt(), which is common for all the platforms.
Thus, we should either retrieve the append flag from the Java code for 
any platform, and ignore it later on Unix, or read the flag in the 
native Windows-only code.


Alternatively, we could separate implementations of 
FileOutputStream.java for different platforms, but that would complicate 
everything.


Hopefully, in the future we could find a way to use FILE_APPEND_DATA, so 
FileDescriptor.append can be removed altogether with that corresponding 
JNI code.


Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8023173/1/webrev/

I've also included a few typo fixes left after JDK-8059840.

Sincerely yours,
Ivan

Re: [9] Review request : JDK-8059070: [TESTBUG] java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java failed - timeout

[Konstantin Shefov]

Gently reminder

On 17.10.2014 13:38, Konstantin Shefov wrote:

Hi,

I have updated the webrev: 
http://cr.openjdk.java.net/~kshefov/8059070/webrev.01/


-Konstantin

16.10.2014 17:24, Igor Ignatyev пишет:

Konstantin,

I haven't looked at code religiously, so I wouldn't say that I have 
reviewed it. however I have some comments, please see them inline.


On 10/16/2014 02:03 PM, Konstantin Shefov wrote:

Paul,

Thanks for reviewing

In the jtreg scripts of the three existing LFCaching tests timeout is
set explicitly to 300 seconds. The file currently being changed is 
not a

test itself, it is parent class of tests.
In fact we can unset this explicit timeout and use the current fix
together with default JTREG timeout and jtreg timeout factor option.
These test cases are randomly generated, so the more iterations, the
better, but in fact we need at least one iteration.

As for "if (avgIterTime > 2 * remainTime)", I think 2 is enough.

-Konstantin

On 16.10.2014 13:30, Paul Sandoz wrote:

On Oct 16, 2014, at 10:43 AM, Konstantin Shefov
 wrote:


Gently reminder

On 14.10.2014 16:58, Konstantin Shefov wrote:

Hello,

Please review the test bug fix
https://bugs.openjdk.java.net/browse/JDK-8059070
Webrev is http://cr.openjdk.java.net/~kshefov/8059070/webrev.00/


   45 private static final long TIMEOUT = 30;

Does jtreg define a system property for this value? and is 300s the
same as what jtreg defines as the default?
unfortunately, jtreg doesn't define a property for timeout, but I 
think it should do. we need to file an RFE against jtreg to add such 
a property and an RFE against these tests/testlibrary to use this 
property. could you please file them?


The online documentation (jtreg -onlineHelp) says the default is 2
minutes, but that might be out of date.

Might be more readable to use:

   TimeUnit.SECONDS.toMillis(300);


  143 long passedTime = new Date().getTime() - startTime;

Why don't you use System.currentTimeMillis() instead of "new
Date().getTime()" ?


  145 double timeoutFactor = new
Double(System.getProperty("test.timeout.factor", "1.0"));

You can that pull out into a static and perhaps merge with TIMEOUT.
+ you should use jdk.testlibrary.Utils::adjustTimeout instead of 
writing this code again.



  147 if (avgIterTime > 2 * remainTime) {

That seems sufficient but it will be interesting to see if
intermittent failures still occur due to high variance.

Paul.





Igor

Re: Lower overhead String encoding/decoding

[Alan Bateman]

On 19/10/2014 23:13, Richard Warburton wrote:

Hi,

Hi Richard, couple comments after a quick scan.
Thanks for your comments - only just had a post Javaone chance to look at
this stuff. I've pushed an update to:

*http://cr.openjdk.java.net/~rwarburton/string-patch-webrev-7
*


I think this looks better but I have a few comments on the API.

For String(ByteBuffer, Charset) then it's still inconsistent to read 
from the buffer position but not advance it. I think the constructor 
needs to work like a regular decode in that respect. Related to this is 
the underflow case where there are insufficient remaining bytes to 
complete. If you don't advance the position then there is no way to 
detect this.


The statement about the length of the String ".. may not be equal to the 
length of the subarray" might  be there from a previous iteration. There 
isn't any array in the method signature so I think this statement needs 
to be make a bit clearer.


For getBytes(byte[], int, int, Charset) then I think the javadoc could 
say a bit more. It will encode to a maximum of destBuffer.length - 
destOffset for example. The @return should probably say that it's the 
number of bytes written to the array rather than "copied".  Minor nits 
is that you probably don't want the starting . Also the finals in the 
signature seem noisy/not needed.


The getBytes(ByteBuffer, Charset) method needs a bit more javadoc. You 
should be able to borrow from text from the CharsetEncoder#encode 
methods to help with that.


-Alan.