Re: Please review CSR : JDK-8200437 String#isBlank

[Andrej Golovnin]

Hi Jim,

please add also the #isNotBlank() method. I know the one can write
!str.isBlank(), but str.isNotBlank() is better from the readability
standpoint of view.

Also I think the new methods:

#isBlank()
#isNotBlank() // if you add it.
#lines()

and the old one

String#isEmpty()

should be defined on the CharSequence interface with default
implementations and not on the String class. The sub-classes of
CharSequence shall then provide optimised implementations. This would
allow us to have this methods on StringBuilder and CharBuffer too.

Best regards,
Andrej Golovnin

On Wed, Apr 25, 2018 at 7:04 PM, Jim Laskey  wrote:
> Please review and mark as reviewed 
> https://bugs.openjdk.java.net/browse/JDK-8200437
>

Re: (M) RFR: 8200167: Validate more special case invocations

[David Holmes]

Revised webrev: http://cr.openjdk.java.net/~dholmes/8200167/webrev.v2/

Karen formulated an additional test scenario invoking Object methods 
through an interface reference, which when turned into a new testcase 
demonstrated that the receiver typecheck was also missing in that case 
for Methodhandle's from findSpecial.


Again Vladimir Ivanov formulated the fix for this. Thanks Vladimir!

Summary of changes from original webrev:

- We introduce a new version of DirectMethodHandle.preparedlambdaForm 
that takes the caller class as a parameter, and that class is checked 
for being an interface (not the method's declaring class) to trigger the 
switch to LF_SPECIAL_IFC. (This obviously addresses one problem with 
invoking Object methods - as Object is not an interface - but by itself 
did not fix the problem.)


- We introduce a new LambdaForm kind, DIRECT_INVOKE_SPECIAL_IFC, which 
we use when dealing with LF_INVSPECIAL_IFC. (This was the key in 
ensuring the receiver typecheck via Special.checkReceiver remained in 
the generated code.)


- In the test we:
  - introduce a new invokeDirect testcase for Object.toString(), but we 
need to do that via a modified jcod file (as javac generates an 
invokevirtual)

  - introduce the new invokeSpecialObjectMH(I2 i) call for the MH case.

Thanks,
David

On 17/04/2018 6:48 PM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8200167
webrev: http://cr.openjdk.java.net/~dholmes/8200167/webrev/

Credit to John Rose and Vladimir Ivanov for the form of the MH fix; and 
to Tobias Hartmann for the C1 fix. Their help was very much appreciated 
(and needed!).


tl;dr version: there are some places where badly formed interface method 
calls (which are not detected by the verifier) were missing runtime 
checks on the type of the receiver. Similar issues have been fixed in 
the past and this was a remaining hole in relation to invokespecial 
semantics and interface calls via MethodHandles. It also turned out 
there was an issue in C1 that affected direct invokespecial calls.


Description of changes:

- src/java.base/share/classes/java/lang/invoke/MethodTypeForm.java

Added a new form LF_INVSPECIAL_IFC for invokespecial of interface methods.

- src/java.base/share/classes/java/lang/invoke/MethodHandles.java

Renamed callerClass parameter to boundCallerClass, where it originates 
from findBoundCallerClass. The name "callerClass" is misleading because 
most of the time it is NULL!


In getDirectMethodCommon, pass the actual caller class (lookupClass) to 
DirectMethodHandle.make. And correct the comment about restrictReceiver.


- src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java

DirectMethodHandle.make now takes a callerClass parameter (which may be 
null).


DirectMethodHandle make "receiver" parameter is renamed "refc" as it is 
not the receiver (it's the resolved type of the method holder ie REFC).


The Special subclass now takes a "checkClass" parameter which is either 
refc, or callerClass. This is what we will check the receiver against.


In preparedLambdaForm we switch from LF_INVSPECIAL to LF_INVSPECIAL_IFC 
if the target method is in an interface.


In makePreparedLambdaForm we augment the needsReceiverCheck test to 
include LF_INVSPECIAL_IFC. (So we will not be doing a new receiver type 
check on all invokespecials, just those involving interface methods.)


Added DMH.checkReceiver which is overridden by both the Special and 
Interface subclasses.


- src/hotspot/share/c1/c1_Canonicalizer.cpp

Don't optimize away the checkcast when it is an invokespecial receiver 
check.


- test/jdk/java/lang/invoke/SpecialInterfaceCall.java

(I've moved this test back and forth between hotspot/runtime and 
j.l.invoke as it spans direct calls and MH calls. But I think on balance 
it lands better here.)


The test sets up direct interface method calls for which invokespecial 
is generated by javac, and similarly it creates MethodHandles with 
invokespecial semantics. The should-not-throw cases are trivial. The 
should-throw cases involve MH wizardry.


The test is broken into three parts to check the behaviour on first use 
(when the method has to be initially resolved); on second use (to test 
we don't use the cpcache in a way that is invalid); and again after 
forcing JIT compilation of the calls.


The test is run 5 times to exercise the interpreter (yes the multiple 
runs internal to the test are redundant in this case, but it's much 
simpler to just run it all than try to work out what needs to be run); 
the three variants of using C1, plus a C2 only run.


---

Testing:
   - the new test of course
   - hotspot/runtime/*
   - hotspot/compiler/jsr292
   - jdk/java/lang/invoke

   - hs tiers 1 and 2
   - jdk tiers 1, 2 and 3

Plus some related closed tests.

Thanks,
David
-

Re: [PATCH] Prefer TMPDIR over hard coded /tmp

[Bernd Eckenfels]

Hello.

For robustness reasons, should it maybe fall back to the hardcoded default if 
the target path does not exist or has the (obvious*) missing write permission?

Gruss
Bernd

* with or without trying a actual write?
--
http://bernd.eckenfels.net
_
From: Brian Burkhalter 
Sent: Donnerstag, April 26, 2018 5:09 AM
Subject: Re: [PATCH] Prefer TMPDIR over hard coded /tmp
To: Robert Stupp 
Cc: 


Hi Robert,

On Apr 23, 2018, at 7:23 AM, Robert Stupp  wrote:

> For MacOS and Windows, Java prefers the user's temporary directory for 
> java.io.tmpdir, but not for Linux, where it is always set to /tmp. The burden 
> with this is that if you want to use a different temp directory, you have to 
> explicitly pass -Djava.io.tmpdir=... on the command line, which can be 
> difficult especially for unit tests or microbenchmarks in 3rd party code ran 
> via Maven for example.
>
> java_props_t.tmp_dir is always initialized as P_tmpdir in GetJavaProperties 
> (src/java.base/unix/native/libjava/java_props_md.c).
>
> The attached patch changed the behavior to use the content of the environment 
> variable TMPDIR, if present. Note that this does not change where the hsperf* 
> folders are created.

The source change looks OK to me aside from the copyright year and that in the 
second line here

+ v = getenv("TMPDIR");
+ if (v) {
+ sprops.tmp_dir = strdup(v);
+ }

it should probably have

+ if (v != NULL) {

for consistency with elsewhere in the file.

> I'm not sure why java.io.tmpdir is always /tmp in Linux as according to the 
> SCM history, this was always as it is (references the initial OpenJDK commit).
>
> I haven't found any tests that validates the content of java.io.tmpdir.

Some sort of test would need to be added to this patch to validate the change.

Thanks,

Brian

Re: [PATCH] regex matcher opt: remove redundant StringBuilder

[Isaac Levy]

yeah perhaps this is enough.  Though users of appendReplacement are
presumably after high performance loops or they'd be using a higher
level API like replaceAll.  I just can't imagine people using this API
hit the format code exception in normal usage, and it's not like java
is dumping junk into the StringBuilder when it throws the exception --
it pushed the correct replacement up to the error.

Maybe I can catch exceptions, roll back the builder using setLength,
and rethrow.

Isaac


On Tue, Apr 24, 2018 at 9:38 PM, Xueming Shen  wrote:
> for String based replaceAll/First() it might be worth doing something like
>
> http://cr.openjdk.java.net/~sherman/regex_removesb/webrev/
>
>
> On 4/24/18, 10:47 AM, Isaac Levy wrote:
>>
>> Hi Sherman,
>>
>> Thanks for clarifying. Looks like exceptions are caused by invalid
>> format string. I wouldn't expect most programs to be catching this and
>> preserving their buffer, but dunno.
>>
>> How much does it affect perf? Well it depends on use case, a jmh of
>> replaceAll with a length 200 string of digits and regex "(\w)" shows
>> about 30% speedup.
>>
>> [info] Benchmark  Mode  Cnt   Score   Error  Units
>> [info] origM  avgt   10  11.669 ± 0.211  us/op
>> [info] newM   avgt   10   8.926 ± 0.095  us/op
>>
>> Isaac
>>
>>
>> On Tue, Apr 24, 2018 at 12:53 PM, Xueming Shen
>> wrote:
>>>
>>> Hi Isaac,
>>>
>>> I actually meant to say "we are not supposed to output the partial text
>>> into
>>> the output buffer in case of an exception". It has nothing to do with the
>>> changeset you cited. This has been the behavior since day one/JDK1.4,
>>> though it is not specified explicitly in the API doc. The newly added
>>> StringBuilder variant simply follows this behavior. If it's really
>>> desired
>>> it
>>> is kinda doable to save that StringBuilder without the "incompatible"
>>> behavior
>>> change but just wonder if it is really worth the effort.
>>>
>>> Thanks,
>>> Sherman
>>>
>>>
>>> On 4/24/18, 9:11 AM, Isaac Levy wrote:

 (moving this to a separate discussion)


 --- a/src/java.base/share/classes/java/util/regex/Matcher.java
 +++ b/src/java.base/share/classes/java/util/regex/Matcher.java
 @@ -993,13 +993,11 @@
   public Matcher appendReplacement(StringBuilder sb, String
 replacement) {
// If no match, return error
if (first<   0)
throw new IllegalStateException("No match available");
 -StringBuilder result = new StringBuilder();
 -appendExpandedReplacement(replacement, result);
// Append the intervening text
sb.append(text, lastAppendPosition, first);
// Append the match substitution
 +appendExpandedReplacement(replacement, sb);
 -sb.append(result);



 On Mon, Apr 23, 2018 at 5:05 PM Xueming Shen
 wrote:
>
>
> I would assume in case of an exception thrown from
> appendExpandedReplacement() we don't
> want "text" to be pushed into the "sb".
>
> -sherman


 Perhaps. Though the behavior under exception is undefined and this
 function is probably primarily used though the replaceAll API, which
 wouldn’t return the intermediate sb under the case of exception.

 My reading of the blame was the temp StringBuilder was an artifact of
 the api previously using StringBuffer externally.  See
 http://hg.openjdk.java.net/jdk9/dev/jdk/rev/763c564451b3
>>>
>>>
>

Re: [PATCH] Prefer TMPDIR over hard coded /tmp

[Brian Burkhalter]

On Apr 25, 2018, at 5:38 PM, Brian Burkhalter  
wrote:

>> The attached patch changed the behavior to use the content of the 
>> environment variable TMPDIR, if present. Note that this does not change 
>> where the hsperf* folders are created.
> 
> The source change looks OK to me aside from the copyright year and that in 
> the second line here

Note that I am only saying that the patch looks OK, not that we want to use it. 
There have been a number of issues filed in this area over time, for example 
https://bugs.openjdk.java.net/browse/JDK-6205979. There are arguments against 
this change.

Brian

Re: [PATCH] Prefer TMPDIR over hard coded /tmp

[Brian Burkhalter]

Hi Robert,

On Apr 23, 2018, at 7:23 AM, Robert Stupp  wrote:

> For MacOS and Windows, Java prefers the user's temporary directory for 
> java.io.tmpdir, but not for Linux, where it is always set to /tmp. The burden 
> with this is that if you want to use a different temp directory, you have to 
> explicitly pass -Djava.io.tmpdir=... on the command line, which can be 
> difficult especially for unit tests or microbenchmarks in 3rd party code ran 
> via Maven for example.
> 
> java_props_t.tmp_dir is always initialized as P_tmpdir in GetJavaProperties 
> (src/java.base/unix/native/libjava/java_props_md.c).
> 
> The attached patch changed the behavior to use the content of the environment 
> variable TMPDIR, if present. Note that this does not change where the hsperf* 
> folders are created.

The source change looks OK to me aside from the copyright year and that in the 
second line here

+v = getenv("TMPDIR");
+if (v) {
+sprops.tmp_dir = strdup(v);
+}

it should probably have

+if (v != NULL) {

for consistency with elsewhere in the file.

> I'm not sure why java.io.tmpdir is always /tmp in Linux as according to the 
> SCM history, this was always as it is (references the initial OpenJDK commit).
> 
> I haven't found any tests that validates the content of java.io.tmpdir.

Some sort of test would need to be added to this patch to validate the change.

Thanks,

Brian

Re: 8202284: FileChannel and FileOutpuStream variants of AtomicAppend should pass silently on macOS >= 10.13

[Brian Burkhalter]

https://bugs.openjdk.java.net/browse/JDK-8202290 to track this problem.

Brian

On Apr 25, 2018, at 3:37 PM, Brian Burkhalter  
wrote:

> None yet. I awaiting direction as to whether I can reopen the inadvertently 
> resolved issue or whether I need to create a new issue with the same content. 
> I think the latter.
> 
> Brian
> 
> On Apr 25, 2018, at 3:30 PM, Mikael Vidstedt  
> wrote:
> 
>> Which JBS issue is now covering the work of looking at the actual failure?

Re: 8202284: FileChannel and FileOutpuStream variants of AtomicAppend should pass silently on macOS >= 10.13

[Brian Burkhalter]

None yet. I awaiting direction as to whether I can reopen the inadvertently 
resolved issue or whether I need to create a new issue with the same content. I 
think the latter.

Brian

On Apr 25, 2018, at 3:30 PM, Mikael Vidstedt  wrote:

> Which JBS issue is now covering the work of looking at the actual failure?

Re: 8202284: FileChannel and FileOutpuStream variants of AtomicAppend should pass silently on macOS >= 10.13

[Mikael Vidstedt]

Which JBS issue is now covering the work of looking at the actual failure?

Cheers,
Mikael

> On Apr 25, 2018, at 3:11 PM, Brian Burkhalter  
> wrote:
> 
> Issue:https://bugs.openjdk.java.net/browse/JDK-8202284
> Patch:http://cr.openjdk.java.net/~bpb/8202284/webrev.00/
> 
> Suite of [1].
> 
> Remove AtomicAppend tests from the problem list instead making then pass 
> silently for macOS version >= 10.13.
> 
> Thanks,
> 
> Brian
> 
> [1] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052751.html

Re: 8202062: Put FileChannel and FileOutpuStream variants of AtomicAppend on problem list

[Brian Burkhalter]

On Apr 24, 2018, at 11:05 PM, Alan Bateman  wrote:

> On 24/04/2018 23:15, Brian Burkhalter wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8202062
>> 
>> Recently more frequent failures have been observed on macOS. Put on the 
>> problem list until the underlying cause can be determined.
>> 
> I think it would be better to change the tests to pass silently when running 
> on macOS 10.13 or higher. At least from first look, that is where the issue 
> is.

Please see

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052782.html

Thanks,

Brian

8202284: FileChannel and FileOutpuStream variants of AtomicAppend should pass silently on macOS >= 10.13

[Brian Burkhalter]

Issue:  https://bugs.openjdk.java.net/browse/JDK-8202284
Patch:  http://cr.openjdk.java.net/~bpb/8202284/webrev.00/

Suite of [1].

Remove AtomicAppend tests from the problem list instead making then pass 
silently for macOS version >= 10.13.

Thanks,

Brian

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052751.html

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

[Claes Redestad]

Hi Peter,

too late! :-)

Good observation about us creating a new Function every time we look up 
an item. If we go with an Object as a reservation marker we could get 
away with a singleton static Function and still use computeIfAbsent 
(which I think is clearer and avoids creating a reservation Object on 
every lookup). I'd consider this a reasonable follow-up RFE:


http://cr.openjdk.java.net/~redestad/scratch/ClassSpec_followup.00/

Its value as an optimization might be somewhat dubious, but it does help 
future-proof the mechanism (say if we wanted to add forms that are even 
more heavyweight or if we needed to side-effect something when calling 
newSpeciesData...).


/Claes


On 2018-04-25 18:45, Peter Levart wrote:

Hi Claes,

Your patch is OK from logical point of view, but something bothers me 
a little. For each species data that gets cached, at least 3 objects 
are created:


- the Function passed to computeIfAbsent
- the unresolved SpeciesData object that serves as a placeholder
- the 2nd resolved SpeciesData object that replaces the 1st

The 1st SpeciesData object serves just as placeholder, so it could be 
lighter-weight. An java.lang.Object perhaps? Creation of Function 
object could be eliminated by using putIfAbsent instead of 
computeIfAbsent then. For the price of some unsafe casts that are 
contained to a single method that is the sole user of CHM cache.


For example, like this:

http://cr.openjdk.java.net/~plevart/jdk-dev/8202184_ClassSpecializer/webrev.01/ 



I won't oppose to your version if you find it easier to understand. I 
just hat to try to do it without redundant creation of placeholder 
SpeciesData object.


What do you think?

Regards, Peter

On 04/24/2018 06:57 PM, Claes Redestad wrote:

Hi,

the current implementation of ClassSpecializer.findSpecies may cause
excessive blocking due to a potentially expensive computeIfAbsent, and
we have reason to believe this might have been cause for a few very
rare bootstrap issues in tests that attach debuggers to VM in the middle
of this.

Breaking this apart so that code generation and linking is done outside
of the computeIfAbsent helps minimize the risk that a thread gets
interrupted by a debugger at a critical point in time, while also 
removing
a few limitations on what we can do from a findSpecies, e.g., look up 
other

SpeciesData.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202184
Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/

This implementation inserts a placeholder, unresolved SpeciesData,
and then replaces it with another instance that is linked together
fully before publishing it, which ensure safe publication. There might
be alternatives involving volatiles and/or careful fencing, but I've not
been able to measure any added startup cost from this anyway.

Thanks!

/Claes

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Xueming Shen]

On 04/25/2018 01:39 PM, Jan Lahoda wrote:

So, if I understand correctly, it would be:
boolean flipEcho;
and the readPassword would do something like:
if (echo0() != false) {


if (echo0()) { ...


flipEcho = true;
echo(false);
}

if (flipEcho) { //this would also be in the shutdown hook
 echo(!echo0());
}


if (flipEcho) {
echo(true);
flipEcho = false;
}
?



I guess I can do that (the variant with two boolean feels to me slightly easier 
to understand and a tiny bit more robust, but this flip boolean variant is 
doable as well).

Thanks,
Jan

On 25.4.2018 21:07, Martin Buchholz wrote:

I keep hoping for something simpler.

Is it possible to have more than one Console object?  Looks like NO.
Assuming no, then you simply need one static flag

boolean restoreEcho;

(it could also be an instance field of Console - that would be slightly
more principled)

In readPassword you check current value of echo and set restoreEcho if it
was changed.  Shutdown hook also checks the same restoreEcho.

For bonus points, only create the shutdown hook the first time readPassword
is called with echo on to appease the Emacs shell users with pitchforks.

On Wed, Apr 25, 2018 at 9:34 AM, Xueming Shen 
wrote:


On 4/25/18, 9:02 AM, Martin Buchholz wrote:

It would be more correct I think for Console to track if there is a
pending readPassword in progress and try to restore echo on exit only if
so.  But a little annoying to implement (need an additional boolean?)


I think that is what Jan proposed "to add restoredEchoOnShutdown = true",
and I'm fine
with that. I'm just a little confused why jshell invokes System.console()
if it handles all "raw"
terminal stuff itself.


On Wed, Apr 25, 2018 at 8:52 AM, Xueming Shen 
wrote:



Hi Jan,

I saw System.console() returns null inside jshell ... but it seems there
are 2 vms.
I would assume jshell  itself sets the terminal to raw and then call
System.console()?
for example an alternative for this issue is to ask jshell's impl to call
System.console()
before going into raw mode? No, I'm not saying the proposed one is not a
good one,
just wanted to make sure I understand the situation correctly.

Thanks,
Sherman



On 4/25/18, 4:50 AM, Jan Lahoda wrote:


Hi,

Under:
https://bugs.openjdk.java.net/browse/JDK-8194750

j.i.Console was changed to capture the state of the terminal echo at
creation time, and to restore it on shutdown.

That is problematic at least in jshell, where the terminal is already in
the raw mode when j.i.Console is created, and so "echo disabled" is
recorded there. So even though jshell itself sets the terminal into the
original mode when it terminates, the shutdown hook in j.i.Console, which
is run later, sets the echo to off even if it was enabled before the VM
started.

My understanding is that the shutdown hook is only needed in case the VM
goes down while readPassword is running. So I tried to change the shutdown
hook to only work while readPassword is running.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/

What do you think?

Thanks,
 Jan

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Martin Buchholz]

On Wed, Apr 25, 2018 at 1:34 PM, Jan Lahoda  wrote:

>
> IIRC the call to System.console() in jshell/jline that is part of this
> problem is basically a way to call Console.istty() - the returned Console
> is not used. I was considering tweaking jshell to avoid this issue, and I
> think it would be possible, but seemed some other programs might run into
> issues as well, so seemed better to fix in Console.


So the shutdown hook is unneeded both by Emacs users and by JShell users,
strengthening the case for optimizing it away.

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Jan Lahoda]

So, if I understand correctly, it would be:
boolean flipEcho;
and the readPassword would do something like:
if (echo0() != false) {
flipEcho = true;
echo(false);
}

if (flipEcho) { //this would also be in the shutdown hook
 echo(!echo0());
}

I guess I can do that (the variant with two boolean feels to me slightly 
easier to understand and a tiny bit more robust, but this flip boolean 
variant is doable as well).


Thanks,
Jan

On 25.4.2018 21:07, Martin Buchholz wrote:

I keep hoping for something simpler.

Is it possible to have more than one Console object?  Looks like NO.
Assuming no, then you simply need one static flag

boolean restoreEcho;

(it could also be an instance field of Console - that would be slightly
more principled)

In readPassword you check current value of echo and set restoreEcho if it
was changed.  Shutdown hook also checks the same restoreEcho.

For bonus points, only create the shutdown hook the first time readPassword
is called with echo on to appease the Emacs shell users with pitchforks.

On Wed, Apr 25, 2018 at 9:34 AM, Xueming Shen 
wrote:


On 4/25/18, 9:02 AM, Martin Buchholz wrote:

It would be more correct I think for Console to track if there is a
pending readPassword in progress and try to restore echo on exit only if
so.  But a little annoying to implement (need an additional boolean?)


I think that is what Jan proposed "to add restoredEchoOnShutdown = true",
and I'm fine
with that. I'm just a little confused why jshell invokes System.console()
if it handles all "raw"
terminal stuff itself.


On Wed, Apr 25, 2018 at 8:52 AM, Xueming Shen 
wrote:



Hi Jan,

I saw System.console() returns null inside jshell ... but it seems there
are 2 vms.
I would assume jshell  itself sets the terminal to raw and then call
System.console()?
for example an alternative for this issue is to ask jshell's impl to call
System.console()
before going into raw mode? No, I'm not saying the proposed one is not a
good one,
just wanted to make sure I understand the situation correctly.

Thanks,
Sherman



On 4/25/18, 4:50 AM, Jan Lahoda wrote:


Hi,

Under:
https://bugs.openjdk.java.net/browse/JDK-8194750

j.i.Console was changed to capture the state of the terminal echo at
creation time, and to restore it on shutdown.

That is problematic at least in jshell, where the terminal is already in
the raw mode when j.i.Console is created, and so "echo disabled" is
recorded there. So even though jshell itself sets the terminal into the
original mode when it terminates, the shutdown hook in j.i.Console, which
is run later, sets the echo to off even if it was enabled before the VM
started.

My understanding is that the shutdown hook is only needed in case the VM
goes down while readPassword is running. So I tried to change the shutdown
hook to only work while readPassword is running.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/

What do you think?

Thanks,
 Jan

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Jan Lahoda]

Hi Sherman,

JShell uses two processes - the main one interacts with the terminal and 
compiles the user's snippets. The other one runs the snippets, and is 
not connected to a terminal, so System.console() does not work there.


IIRC the call to System.console() in jshell/jline that is part of this 
problem is basically a way to call Console.istty() - the returned 
Console is not used. I was considering tweaking jshell to avoid this 
issue, and I think it would be possible, but seemed some other programs 
might run into issues as well, so seemed better to fix in Console.


Jan

On 25.4.2018 17:52, Xueming Shen wrote:


Hi Jan,

I saw System.console() returns null inside jshell ... but it seems there
are 2 vms.
I would assume jshell  itself sets the terminal to raw and then call
System.console()?
for example an alternative for this issue is to ask jshell's impl to
call System.console()
before going into raw mode? No, I'm not saying the proposed one is not a
good one,
just wanted to make sure I understand the situation correctly.

Thanks,
Sherman


On 4/25/18, 4:50 AM, Jan Lahoda wrote:

Hi,

Under:
https://bugs.openjdk.java.net/browse/JDK-8194750

j.i.Console was changed to capture the state of the terminal echo at
creation time, and to restore it on shutdown.

That is problematic at least in jshell, where the terminal is already
in the raw mode when j.i.Console is created, and so "echo disabled" is
recorded there. So even though jshell itself sets the terminal into
the original mode when it terminates, the shutdown hook in
j.i.Console, which is run later, sets the echo to off even if it was
enabled before the VM started.

My understanding is that the shutdown hook is only needed in case the
VM goes down while readPassword is running. So I tried to change the
shutdown hook to only work while readPassword is running.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/

What do you think?

Thanks,
Jan

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[John Rose]

On Apr 25, 2018, at 12:07 PM, Martin Buchholz  wrote:
> 
> For bonus points, only create the shutdown hook the first time readPassword
> is called with echo on to appease the Emacs shell users with pitchforks.

FTR, I use Emacs shell for everything except jshell, and have to
launch a separate shell app. (Terminal) to run jshell.  That's the only
reason I launch that app.  My pitchfork is still in the shed, but I feel
the pain here too.  There's probably a config. switch I'm missing,
but jshell has never worked for me under Emacs.

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Martin Buchholz]

I keep hoping for something simpler.

Is it possible to have more than one Console object?  Looks like NO.
Assuming no, then you simply need one static flag

boolean restoreEcho;

(it could also be an instance field of Console - that would be slightly
more principled)

In readPassword you check current value of echo and set restoreEcho if it
was changed.  Shutdown hook also checks the same restoreEcho.

For bonus points, only create the shutdown hook the first time readPassword
is called with echo on to appease the Emacs shell users with pitchforks.

On Wed, Apr 25, 2018 at 9:34 AM, Xueming Shen 
wrote:

> On 4/25/18, 9:02 AM, Martin Buchholz wrote:
>
> It would be more correct I think for Console to track if there is a
> pending readPassword in progress and try to restore echo on exit only if
> so.  But a little annoying to implement (need an additional boolean?)
>
>
> I think that is what Jan proposed "to add restoredEchoOnShutdown = true",
> and I'm fine
> with that. I'm just a little confused why jshell invokes System.console()
> if it handles all "raw"
> terminal stuff itself.
>
>
> On Wed, Apr 25, 2018 at 8:52 AM, Xueming Shen 
> wrote:
>
>>
>> Hi Jan,
>>
>> I saw System.console() returns null inside jshell ... but it seems there
>> are 2 vms.
>> I would assume jshell  itself sets the terminal to raw and then call
>> System.console()?
>> for example an alternative for this issue is to ask jshell's impl to call
>> System.console()
>> before going into raw mode? No, I'm not saying the proposed one is not a
>> good one,
>> just wanted to make sure I understand the situation correctly.
>>
>> Thanks,
>> Sherman
>>
>>
>>
>> On 4/25/18, 4:50 AM, Jan Lahoda wrote:
>>
>>> Hi,
>>>
>>> Under:
>>> https://bugs.openjdk.java.net/browse/JDK-8194750
>>>
>>> j.i.Console was changed to capture the state of the terminal echo at
>>> creation time, and to restore it on shutdown.
>>>
>>> That is problematic at least in jshell, where the terminal is already in
>>> the raw mode when j.i.Console is created, and so "echo disabled" is
>>> recorded there. So even though jshell itself sets the terminal into the
>>> original mode when it terminates, the shutdown hook in j.i.Console, which
>>> is run later, sets the echo to off even if it was enabled before the VM
>>> started.
>>>
>>> My understanding is that the shutdown hook is only needed in case the VM
>>> goes down while readPassword is running. So I tried to change the shutdown
>>> hook to only work while readPassword is running.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/
>>>
>>> What do you think?
>>>
>>> Thanks,
>>> Jan
>>>
>>
>>
>
>

Re: RFR: 8201274: Launch Single-File Source-Code Programs

[Jonathan Gibbons]

Kumar,

Thank you for your feedback;  I will incorporate it in the next webrev.

-- Jon

On 04/25/2018 09:38 AM, Kumar Srinivasan wrote:

Hi John,

I focused mainly on the native side, looks ok, except  for a couple of
minor issues.

java.c
1320 const char *prop = "-Djdk.internal.javac.source=";
1321 size_t size = JLI_StrLen(prop) + JLI_StrLen(value) + 1;
1322 char *propValue = (char *)JLI_MemAlloc(size + 1);

I think we are allocating extra byte ^^^

1323 JLI_StrCpy(propValue, prop);
1324 JLI_StrCat(propValue, value);

I think we can do this, safer and neater, as follows:

size_t size = JLI_StrLen(prop) + JLI_StrLen(value);
char *propValue = (char *)JLI_MemAlloc(size + 1);
JLI_Snprintf(propValue, size, "%s%s", prop, value);
1483 if (mode == LM_SOURCE) {
1484 AddOption("--add-modules=ALL-DEFAULT", NULL);
1485 *pwhat = SOURCE_LAUNCHER_MAIN_ENTRY;
1486 *pargc = argc + 1;
1487 *pargv = argv - 1;

A short comment perhaps ? why we are incrementing argc, and 
decrementing argv,

saves some head scratching for a casual reader.

I looked at the launcher tests, very nice.


Thanks
Kumar





On 4/12/2018 1:15 PM, Jonathan Gibbons wrote:

Please review an initial implementation for the feature described in
JEP 330: Launch Single-File Source-Code Programs.

The work is described in the JEP and CSR, and falls into various parts:

  * The part to handle the new command-line options is in the native
Java launcher code.
  * The part to invoke the compiler and subsequently execute the code
found in the source file is in a new class in the jdk.compiler
module.
  * There are some minor Makefile changes, to add support for a new
resource file.

There are no changes to javac itself.

JEP: http://openjdk.java.net/jeps/330
JBS: https://bugs.openjdk.java.net/browse/JDK-8201274
CSR: https://bugs.openjdk.java.net/browse/JDK-8201275
Webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.00/

-- Jon

Please review CSR : JDK-8200437 String#isBlank

[Jim Laskey]

Please review and mark as reviewed 
https://bugs.openjdk.java.net/browse/JDK-8200437

Please review CSR : JDK-8200378 String#strip, String#stripLeading, String#stripTrailing

[Jim Laskey]

Please review and mark as reviewed 
https://bugs.openjdk.java.net/browse/JDK-8200378

Please review CSR : JDK-8200373 String#trim JavaDoc should clarify meaning of space

[Jim Laskey]

Please review and mark as reviewed 
https://bugs.openjdk.java.net/browse/JDK-8200373

Please review CSR : JDK-8200425 String#lines

[Jim Laskey]

Please review and mark as reviewed 
https://bugs.openjdk.java.net/browse/JDK-8200425

Re: [11] RFR: 8202026 8193552 : ISO 4217 Amendment #165 # 166 Update

[naoto . sato]

Hi Leo,

Although JDK11 is slated in 09/2018, enabling amendment 166 now is 
technically a bug, as it will be effective from June 4. Please use the 
transition mechanism in make/data/currency/CurrencyData.properties and 
test/jdk/java/util/Currency/tablea1.txt.


OTOH, there are old (past) transition entries. I would clean up those 
entries, such as:


 326 # LATVIA
 327 LV=LVL;2013-12-31-22-00-00;EUR

in CurrencyData.properties. This applies to tabela1.txt as well.

Naoto

On 4/24/18 8:52 PM, Leo Jiang wrote:

Forgot to mention, the tests in Currency fold are passed on Mach5.

-Leo

On 04/25/2018 09:33 AM, Leo Jiang wrote:

Hi,

Please review the changes to address the ISO 4217 Amendment 165 166 
update.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8193552  165
https://bugs.openjdk.java.net/browse/JDK-8202026  166

CR:
http://cr.openjdk.java.net/~ljiang/8202026/webrev.00/


Detail:
#165
From:
MAURITANIA    Ouguiya    MRO    478    2
To:
MAURITANIA    Ouguiya    MRU    929    2

#166
From:
VENEZUELA (BOLIVARIAN REPUBLIC OF)    Bolívar    VEF    937    2
To:
VENEZUELA (BOLIVARIAN REPUBLIC OF)    Bolívar Soberano    VES    928    2


Thanks,
Leo

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

[Peter Levart]

Hi Claes,

Your patch is OK from logical point of view, but something bothers me a 
little. For each species data that gets cached, at least 3 objects are 
created:


- the Function passed to computeIfAbsent
- the unresolved SpeciesData object that serves as a placeholder
- the 2nd resolved SpeciesData object that replaces the 1st

The 1st SpeciesData object serves just as placeholder, so it could be 
lighter-weight. An java.lang.Object perhaps? Creation of Function object 
could be eliminated by using putIfAbsent instead of computeIfAbsent 
then. For the price of some unsafe casts that are contained to a single 
method that is the sole user of CHM cache.


For example, like this:

http://cr.openjdk.java.net/~plevart/jdk-dev/8202184_ClassSpecializer/webrev.01/

I won't oppose to your version if you find it easier to understand. I 
just hat to try to do it without redundant creation of placeholder 
SpeciesData object.


What do you think?

Regards, Peter

On 04/24/2018 06:57 PM, Claes Redestad wrote:

Hi,

the current implementation of ClassSpecializer.findSpecies may cause
excessive blocking due to a potentially expensive computeIfAbsent, and
we have reason to believe this might have been cause for a few very
rare bootstrap issues in tests that attach debuggers to VM in the middle
of this.

Breaking this apart so that code generation and linking is done outside
of the computeIfAbsent helps minimize the risk that a thread gets
interrupted by a debugger at a critical point in time, while also 
removing
a few limitations on what we can do from a findSpecies, e.g., look up 
other

SpeciesData.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202184
Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/

This implementation inserts a placeholder, unresolved SpeciesData,
and then replaces it with another instance that is linked together
fully before publishing it, which ensure safe publication. There might
be alternatives involving volatiles and/or careful fencing, but I've not
been able to measure any added startup cost from this anyway.

Thanks!

/Claes

Re: RFR: 8201274: Launch Single-File Source-Code Programs

[Kumar Srinivasan]

Hi John,

I focused mainly on the native side, looks ok, except  for a couple of
minor issues.

java.c

1320 const char *prop = "-Djdk.internal.javac.source=";
1321 size_t size = JLI_StrLen(prop) + JLI_StrLen(value) + 1;
1322 char *propValue = (char *)JLI_MemAlloc(size + 1);

I think we are allocating extra byte ^^^

1323 JLI_StrCpy(propValue, prop);
1324 JLI_StrCat(propValue, value);

I think we can do this, safer and neater, as follows:

size_t size = JLI_StrLen(prop) + JLI_StrLen(value);
char *propValue = (char *)JLI_MemAlloc(size + 1);
JLI_Snprintf(propValue, size, "%s%s", prop, value);

1483 if (mode == LM_SOURCE) {
1484 AddOption("--add-modules=ALL-DEFAULT", NULL);
1485 *pwhat = SOURCE_LAUNCHER_MAIN_ENTRY;
1486 *pargc = argc + 1;
1487 *pargv = argv - 1;

A short comment perhaps ? why we are incrementing argc, and decrementing 
argv,

saves some head scratching for a casual reader.

I looked at the launcher tests, very nice.


Thanks
Kumar





On 4/12/2018 1:15 PM, Jonathan Gibbons wrote:

Please review an initial implementation for the feature described in
JEP 330: Launch Single-File Source-Code Programs.

The work is described in the JEP and CSR, and falls into various parts:

  * The part to handle the new command-line options is in the native
Java launcher code.
  * The part to invoke the compiler and subsequently execute the code
found in the source file is in a new class in the jdk.compiler module.
  * There are some minor Makefile changes, to add support for a new
resource file.

There are no changes to javac itself.

JEP: http://openjdk.java.net/jeps/330
JBS: https://bugs.openjdk.java.net/browse/JDK-8201274
CSR: https://bugs.openjdk.java.net/browse/JDK-8201275
Webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.00/

-- Jon

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Xueming Shen]

On 4/25/18, 9:02 AM, Martin Buchholz wrote:
It would be more correct I think for Console to track if there is a 
pending readPassword in progress and try to restore echo on exit only 
if so.  But a little annoying to implement (need an additional boolean?)


I think that is what Jan proposed "to add restoredEchoOnShutdown = 
true", and I'm fine
with that. I'm just a little confused why jshell invokes 
System.console() if it handles all "raw"

terminal stuff itself.



On Wed, Apr 25, 2018 at 8:52 AM, Xueming Shen > wrote:



Hi Jan,

I saw System.console() returns null inside jshell ... but it seems
there are 2 vms.
I would assume jshell  itself sets the terminal to raw and then
call System.console()?
for example an alternative for this issue is to ask jshell's impl
to call System.console()
before going into raw mode? No, I'm not saying the proposed one is
not a good one,
just wanted to make sure I understand the situation correctly.

Thanks,
Sherman



On 4/25/18, 4:50 AM, Jan Lahoda wrote:

Hi,

Under:
https://bugs.openjdk.java.net/browse/JDK-8194750


j.i.Console was changed to capture the state of the terminal
echo at creation time, and to restore it on shutdown.

That is problematic at least in jshell, where the terminal is
already in the raw mode when j.i.Console is created, and so
"echo disabled" is recorded there. So even though jshell
itself sets the terminal into the original mode when it
terminates, the shutdown hook in j.i.Console, which is run
later, sets the echo to off even if it was enabled before the
VM started.

My understanding is that the shutdown hook is only needed in
case the VM goes down while readPassword is running. So I
tried to change the shutdown hook to only work while
readPassword is running.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202105

Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/


What do you think?

Thanks,
Jan

Re: JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

[joe darcy]

Hi Jon,


On 4/25/2018 8:57 AM, Jonathan Gibbons wrote:


Joe,

While I note that the primary text has been modified to include long 
types, the italic comment that follows still ends with the following:


Notice that integer literals of type|long|are allowed, but not 
required, to be shared.


-- Jon




It would seem that text missed being deleted with the prior update ;-)

-Joe

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Martin Buchholz]

It would be more correct I think for Console to track if there is a pending
readPassword in progress and try to restore echo on exit only if so.  But a
little annoying to implement (need an additional boolean?)

On Wed, Apr 25, 2018 at 8:52 AM, Xueming Shen 
wrote:

>
> Hi Jan,
>
> I saw System.console() returns null inside jshell ... but it seems there
> are 2 vms.
> I would assume jshell  itself sets the terminal to raw and then call
> System.console()?
> for example an alternative for this issue is to ask jshell's impl to call
> System.console()
> before going into raw mode? No, I'm not saying the proposed one is not a
> good one,
> just wanted to make sure I understand the situation correctly.
>
> Thanks,
> Sherman
>
>
>
> On 4/25/18, 4:50 AM, Jan Lahoda wrote:
>
>> Hi,
>>
>> Under:
>> https://bugs.openjdk.java.net/browse/JDK-8194750
>>
>> j.i.Console was changed to capture the state of the terminal echo at
>> creation time, and to restore it on shutdown.
>>
>> That is problematic at least in jshell, where the terminal is already in
>> the raw mode when j.i.Console is created, and so "echo disabled" is
>> recorded there. So even though jshell itself sets the terminal into the
>> original mode when it terminates, the shutdown hook in j.i.Console, which
>> is run later, sets the echo to off even if it was enabled before the VM
>> started.
>>
>> My understanding is that the shutdown hook is only needed in case the VM
>> goes down while readPassword is running. So I tried to change the shutdown
>> hook to only work while readPassword is running.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
>> Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/
>>
>> What do you think?
>>
>> Thanks,
>> Jan
>>
>
>

Re: JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

[Jonathan Gibbons]

Joe,

While I note that the primary text has been modified to include long 
types, the italic comment that follows still ends with the following:


Notice that integer literals of type|long|are allowed, but not required, 
to be shared.


-- Jon


On 4/25/18 8:18 AM, joe darcy wrote:

Hi David,

On 4/25/2018 5:08 AM, David Holmes wrote:

Hi Joe,

On 25/04/2018 10:30 AM, joe darcy wrote:

Hello,

Please review the patch below to update the specification of 
Long.valueOf(long) to require caching on [-128, 127]. The JDK 
implementation of this functionality has always cached in that 
region, even though it is not required.


Seems very explicit that there is no requirement for Long to do 
caching. So why should that change? Or put another way what has 
changed that invalidates that clearly stated position?




JLS 9 changed the requirements on autobox caching going from Java SE 8 
to 9:


https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.7

vs

https://docs.oracle.com/javase/specs/jls/se9/html/jls-5.html#jls-5.1.7

The Long.valueOf method is used to implement autoboxing and its 
caching spec wasn't updated accordingly at the time.


Cheers,

-Joe

Re: RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Xueming Shen]

Hi Jan,

I saw System.console() returns null inside jshell ... but it seems there 
are 2 vms.
I would assume jshell  itself sets the terminal to raw and then call 
System.console()?
for example an alternative for this issue is to ask jshell's impl to 
call System.console()
before going into raw mode? No, I'm not saying the proposed one is not a 
good one,

just wanted to make sure I understand the situation correctly.

Thanks,
Sherman


On 4/25/18, 4:50 AM, Jan Lahoda wrote:

Hi,

Under:
https://bugs.openjdk.java.net/browse/JDK-8194750

j.i.Console was changed to capture the state of the terminal echo at 
creation time, and to restore it on shutdown.


That is problematic at least in jshell, where the terminal is already 
in the raw mode when j.i.Console is created, and so "echo disabled" is 
recorded there. So even though jshell itself sets the terminal into 
the original mode when it terminates, the shutdown hook in 
j.i.Console, which is run later, sets the echo to off even if it was 
enabled before the VM started.


My understanding is that the shutdown hook is only needed in case the 
VM goes down while readPassword is running. So I tried to change the 
shutdown hook to only work while readPassword is running.


Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/

What do you think?

Thanks,
Jan

Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java

[Adam Farley8]

Hi All, 

Here is the code to resolve JDK-8190187:

http://cr.openjdk.java.net/~mhorie/8190187/hg_diff.txt

Could a committer please take the fix and: 

1) Complete the CSR process for the new JNI Return code. 
2) Commit the changes that contain the Return code.

And, optionally, 3) Perform 1 and 2 for the jdwp agent changes as well, so 
it can use this new functionality.

Let me know if you need any help. :)

Best Regards

Adam Farley

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

[joe darcy]

Hi David,

On 4/25/2018 5:08 AM, David Holmes wrote:

Hi Joe,

On 25/04/2018 10:30 AM, joe darcy wrote:

Hello,

Please review the patch below to update the specification of 
Long.valueOf(long) to require caching on [-128, 127]. The JDK 
implementation of this functionality has always cached in that 
region, even though it is not required.


Seems very explicit that there is no requirement for Long to do 
caching. So why should that change? Or put another way what has 
changed that invalidates that clearly stated position?




JLS 9 changed the requirements on autobox caching going from Java SE 8 to 9:

https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.7

vs

https://docs.oracle.com/javase/specs/jls/se9/html/jls-5.html#jls-5.1.7

The Long.valueOf method is used to implement autoboxing and its caching 
spec wasn't updated accordingly at the time.


Cheers,

-Joe

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Adam Farley8]

> On 13/04/2018 15:14, Adam Farley8 wrote:
>> Hi Alan, Peter, 
>> 
>> I see that native memory is tracked in java.nio.Bits, but that only 
includes what the user thinks they are allocating. 
>> 
>> When the VM adds extra memory to the allocation amount this extra bit 
is not represented in the Bits total. 
>> A cursory glance shows, minimum, that we round the requested memory 
quantity up to the heap word size in 
>> the Unsafe.allocateMemory code, and something to do with 
nmt_header_size in os:malloc() (os.cpp) too.
> Is the align_up(sz, HeapWordSize) really causing an issue?

Coupled with the nmt_header_size business, it makes the Bits values wrong. 
The more DBB allocations, the more 
inaccurate those numbers will be.

> 
> We could change Bits to align with HotSpot. The BufferPoolMXBean API 
allows the capacity and memory usage to differ (when we originally added 
this, direct buffers were page aligned) so doing this would mean it more 
accurately reflects the memory allocated to direct buffers.
> 
> -Alan

I agree with you that the "+x" information should be added to only one of 
the two atomic longs.

To get "X", it seems to me that the best option would be to introduce an 
native method in Bits that fetches
"X" directly from Hotspot, using the same code that Hotspot uses (so we'd 
have to abstract-out the Hotspot logic
that adds X to the memory quantity).

This way, anyone modifying the Hotspot logic won't risk rendering the Bits 
logic wrong again.

Best Regards

Adam Farley

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: JDK 11 RFR of 8200478: For boxing conversion javac uses Long.valueOf which does not guarantee caching according to its javadoc

[David Holmes]

Hi Joe,

On 25/04/2018 10:30 AM, joe darcy wrote:

Hello,

Please review the patch below to update the specification of 
Long.valueOf(long) to require caching on [-128, 127]. The JDK 
implementation of this functionality has always cached in that region, 
even though it is not required.


Seems very explicit that there is no requirement for Long to do caching. 
So why should that change? Or put another way what has changed that 
invalidates that clearly stated position?


Thanks,
David


Additionally, please review the corresponding CSR:

         JDK-8202231: For boxing conversion javac uses Long.valueOf 
which does not guarantee caching according to its javadoc


Thanks,

-Joe

diff -r f909f09569ca src/java.base/share/classes/java/lang/Long.java
--- a/src/java.base/share/classes/java/lang/Long.java    Wed Apr 18 
21:10:09 2018 -0700
+++ b/src/java.base/share/classes/java/lang/Long.java    Tue Apr 24 
17:25:24 2018 -0700

@@ -1164,10 +1164,8 @@
   * significantly better space and time performance by caching
   * frequently requested values.
   *
- * Note that unlike the {@linkplain Integer#valueOf(int)
- * corresponding method} in the {@code Integer} class, this method
- * is not required to cache values within a particular
- * range.
+ * This method will always cache values in the range -128 to 127,
+ * inclusive, and may cache other values outside of this range.
   *
   * @param  l a long value.
   * @return a {@code Long} instance representing {@code l}.

RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Jan Lahoda]

Hi,

Under:
https://bugs.openjdk.java.net/browse/JDK-8194750

j.i.Console was changed to capture the state of the terminal echo at 
creation time, and to restore it on shutdown.


That is problematic at least in jshell, where the terminal is already in 
the raw mode when j.i.Console is created, and so "echo disabled" is 
recorded there. So even though jshell itself sets the terminal into the 
original mode when it terminates, the shutdown hook in j.i.Console, 
which is run later, sets the echo to off even if it was enabled before 
the VM started.


My understanding is that the shutdown hook is only needed in case the VM 
goes down while readPassword is running. So I tried to change the 
shutdown hook to only work while readPassword is running.


Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/

What do you think?

Thanks,
Jan

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

[Peter Levart]

On 04/25/2018 10:06 AM, Claes Redestad wrote:
Besides, CHM.computeIfAbsent has a non-synchronizing fast-path for 
when the key exists,

lines 1731-1734:

    else if (fh == h    // check first node without acquiring 
lock
 && ((fk = f.key) == key || (fk != null && 
key.equals(fk)))

 && (fv = f.val) != null)
    return fv; 


Sorry, you're (almost) right! I confused it with CHM.compute()...

The almost part is that lock is avoided only when the match is found in 
the 1st linked node of the bucket. If there is a hash collision (very 
unlikely) and the entry is in a 2nd or subsequent node in the list, the 
lock is still used. So there's almost no locks used... And if there's no 
hot contention going on, there's no need for prefacing with .get().


Regards, Peter

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

[Claes Redestad]

Hi Paul,

On 2018-04-24 21:01, Paul Sandoz wrote:

Hi,

This looks good.


thanks!


Took me a while to understand the interactions: you need to replace not update 
otherwise there is a race on isResolved (which currently queries multiple 
state, there is no singular guard here). We could push isResolved into the 
synchronized block and simplify but every findSpecies call may take a small hit 
(or there are potentially other ways looking more closely at how 
ClassSpecializer.Factory initializes state, i.e. a fenced static field write, 
but we go further down the rabbit hole)


Right, I've been down that hole unsuccessfully and came up with this 
'play' (a much nicer word than 'hack'; thanks Peter!) as a means to 
escape it. :-)




I think this might fix some rare and intermittent recursive exceptions from 
ConcurrentHashMap cache we have been observing, where a collision occurs on 
keys for recursive updates (rather than for the same key).


I think so, too, but as I've not been able to reproduce any of these 
rare intermittent issues locally I've no real evidence to that effect.


/Claes

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

[Claes Redestad]

Hi Peter,


On 2018-04-25 08:36, Peter Levart wrote:

Hi Claes,

Nice play with CHM and safe publication.


thanks, I was curious how you'd react to this. :-)



If findSpecies() is on a hot concurrent path, [...]


It'd be surprising if it was: findSpecies is typically called once for a 
specific SpeciesData,
and sometimes every now and then during setup of certain method handles 
(in particular
the static speciesData_L* methods in jli.BoundMethodHandle are begging 
to be turned

into lazy constants).

Besides, CHM.computeIfAbsent has a non-synchronizing fast-path for when 
the key exists,

lines 1731-1734:

    else if (fh == h    // check first node without acquiring lock
 && ((fk = f.key) == key || (fk != null && 
key.equals(fk)))

 && (fv = f.val) != null)
    return fv;

.. so I'm not sure we'd gain much from wrapping the preface with a get 
even if it was

hot and contended.

/Claes

Re: RFR of JDK-8201469,test under java/rmi should be restricted to not run concurrently

[Hamlin Li]

Hi Paul, Joe,

Thank you for the comments.

Following directories are added in the exclusiveAccess.dirs list:

*/  java/rmi   (so, java/rmi/Naming is removed)/**/
/**/  com/sun/jndi/rmi/**/
/**/  sun/rmi/*

for the test/bug number and time cost please check following table, here 
time cost is the number on my local machine.


added directories 	test number 	run time 	subdirectories 	test number 
bug number

java/rmi120 

run time in -conc:4, 8:09 (14:15:16 - 14:07:07)

run time in sequence, 18:17 (14:44:00 - 14:25:43)


MarshalledObject

4

0
Naming  6   2
RMISecurityManager/checkPackageAccess   1   0
RemoteException/chaining1   0
activation  31  23
dgc 4   0
invalidName 1   0
module  1   0
registry9   1
reliability 3   2
server  45  7
transport   13  6
com/sun/jndi/rmi2   


0
sun/rmi 17  

run time in -conc:4, 00:44 (14:52:08 - 14:51:24)

run time in sequence, 00:51 (14:48:10 - 14:47:19)



2


@Paul,

Good suggestion, I tried to figure out a subset, but most of 
intermittent rmi test failures happen in java/rmi/[activation, server, 
transport, Naming, reliability, registry] and sun/rmi, they are almost 
all of rmi tests. I'm not sure if it's worth to subset them. I can do it 
if you think it's necessary.


( [1] lists rmi open issue in open area. )


Thank you

-Hamlin


[1] 
https://bugs.openjdk.java.net/issues/?jql=project%20%3D%20jdk%20and%20component%20%3D%20core-libs%20and%20Subcomponent%20%3D%20java.rmi%20and%20status%20in%20(open%2C%20new%2C%20%22In%20Progress%22)%20ORDER%20BY%20updatedDate%20DESC



On 25/04/2018 12:14 PM, joe darcy wrote:
Combining the reformatting and test additions makes the patch more 
difficult to review. The net effect seems to be adding the following 
directories to the exclusiveAccess.dirs list:


  44 sun/management/jmxremote \
  45 sun/rmi \
  46 sun/security/mscapi \
  47 sun/tools/jstatd

Is that correct?

Please characterize the number of tests added to the list and the 
expected performance impact on the tier 3 test group.


Thanks,

-Joe


On 4/23/2018 11:47 AM, Paul Sandoz wrote:

Hi Hamlin,

Do you have a sense of what rmi tests are problematic to run 
concurrently? Maybe you can subset to reduce the increased impact on 
execution time?


Paul.


On Apr 20, 2018, at 12:21 AM, Hamlin Li  wrote:

Is someone available to review the following patch?

Thank you

-Hamlin


On 19/04/2018 4:17 PM, Hamlin Li wrote:

would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8201469

http://cr.openjdk.java.net/~mli/8201469/webrev.00/

( For othervm.dirs property, I just reformat it. )


In my test result, with jtreg option "-concurrency:4", after apply 
the patch, tier3 tests on different platforms will be slower about 
1.5~2.0 times than before.
I think stability of tests are more important than executing time, 
how do you think about it?


Thank you
-Hamlin

Re: RFR: 8202184: Reduce time blocking the ClassSpecializer cache creating SpeciesData

[Peter Levart]

Hi Claes,

Nice play with CHM and safe publication.

If findSpecies() is on a hot concurrent path, you might want to wrap the 
preface:


 176 S speciesData = cache.computeIfAbsent(key, new Function<>() {
 177 @Override
 178 public S apply(K key1) {
 179 return newSpeciesData(key1);
 180 }
 181 });

with a simple:

S speciesData = cache.get(key);
if (speciesData == null) {
    speciesData = cache.computeIfAbsent();
}

or even use putIfAbsent instead if redundant concurrent newSpeciesData 
is OK.


This way you can avoid hot-path synchronization with locks that CHM 
always performs in computeIfAbsent.


Regards, Peter

On 04/24/18 18:57, Claes Redestad wrote:

Hi,

the current implementation of ClassSpecializer.findSpecies may cause
excessive blocking due to a potentially expensive computeIfAbsent, and
we have reason to believe this might have been cause for a few very
rare bootstrap issues in tests that attach debuggers to VM in the middle
of this.

Breaking this apart so that code generation and linking is done outside
of the computeIfAbsent helps minimize the risk that a thread gets
interrupted by a debugger at a critical point in time, while also 
removing
a few limitations on what we can do from a findSpecies, e.g., look up 
other

SpeciesData.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202184
Webrev: http://cr.openjdk.java.net/~redestad/8202184/open.00/

This implementation inserts a placeholder, unresolved SpeciesData,
and then replaces it with another instance that is linked together
fully before publishing it, which ensure safe publication. There might
be alternatives involving volatiles and/or careful fencing, but I've not
been able to measure any added startup cost from this anyway.

Thanks!

/Claes

Re: 8202062: Put FileChannel and FileOutpuStream variants of AtomicAppend on problem list

[Alan Bateman]

On 24/04/2018 23:15, Brian Burkhalter wrote:

https://bugs.openjdk.java.net/browse/JDK-8202062

Recently more frequent failures have been observed on macOS. Put on the problem 
list until the underlying cause can be determined.

I think it would be better to change the tests to pass silently when 
running on macOS 10.13 or higher. At least from first look, that is 
where the issue is.


-Alan