Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Looks fine Lance. I forgot about the copyright year, thank you for
taking care of that one too.

-Jaikiran

On 14/01/20 1:56 am, Lance Andersen wrote:
>
>
>> On Jan 13, 2020, at 1:53 PM, Alan Bateman > > wrote:
>>
>> On 13/01/2020 10:31, Jaikiran Pai wrote:
>>> Hello Christoph,
>>>
>>> Setting inodes to null sounds fine to me and as you say since its usage
>>> is already guarded by ensureOpen, IMO, it should be fine. I've now
>>> updated the patch to set inodes to null in close() and the new updated
>>> webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/
>>>
>> This version looks good except it might be better if the comment just
>> says that it clears the inodes map to allow the keys/values be GC’ed.
>
> I revised the comment to:
>
> 
>
> $ hg diff
> *diff -r 9338d0f52b2e
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java*
> *--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java   
>   Mon Jan 13 11:51:45 2020 -0500*
> *+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java   
>   Mon Jan 13 15:24:37 2020 -0500*
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights
> reserved.
>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>   *
>   * This code is free software; you can redistribute it and/or modify it
> @@ -490,6 +490,14 @@
>                  def.end();
>          }
>
>  
>
> +        beginWrite();                // lock and sync
> +        try {
> +            // Clear the map so that its keys & values can be garbage
> collected
> +            inodes = null;
> +        } finally {
> +            endWrite();
> +        }
> +
>          IOException ioe = null;
>          synchronized (tmppaths) {
>              for (Path p : tmppaths) {
> $
> ———
>
> I will push the change tomorrow barring any hiccups with Mach5 or
> additional comments….
>
> Best
> lance
>>
>> -Alan
>
> 
> 
> Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
>
>
>

Re: RFR: JDK-8233578: Document configurable parameters of msi packages

[Alexander Matveev]

Looks good.

Thanks,
Alexander

On 12/20/2019 8:56 AM, Andy Herrick wrote:

looks good.

/Andy

On 12/19/2019 8:52 PM, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

Comments added to the default WiX source file explaining how the 
default file is expected to be overridden.


- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8233578

[2] http://cr.openjdk.java.net/~asemenyuk/8233578/webrev.00

Re: RFR: JDK-8232077: Investigate if default behavior should allow downgrade scenario

[Alexander Matveev]

Looks good.

Thanks,
Alexander

On 12/20/2019 8:53 AM, Andy Herrick wrote:

fix looks fine.

/Andy

On 12/19/2019 8:02 PM, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

The fix changes the default setting for Windows installers to allow 
downgrades.


- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8232077

[2] http://cr.openjdk.java.net/~asemenyuk/8232077/webrev.00

Re: RFR: 8236850: Operations on constant List/Set.of(element) instances does not consistently constant fold

[John Rose]

On Jan 13, 2020, at 1:23 PM, Claes Redestad  wrote:
> 
> On 2020-01-13 22:18, Paul Sandoz wrote:
>> That’s fine, it was really for informational purposes in case it comes in 
>> handy later on.
> 
> Got it.
> 
>> Speculating: if we had such a public method (with minimal dependencies) I 
>> suspect we would reach for it in this case even if overkill.  It reduces the 
>> cognitive load, but as a downside it would not afford us the opportunity of 
>> a slice Pi:-)
> 
> Not sure it'd pull its own weight as a public API, but refactoring to a
> shared internal utility might be a good start?

Yes.  I want it in the JVM so I can use it there also, to fuzz things like
stack positions and TLB chunk sizes and monitor wait times and
central hash table placements.

As long as the core libs only need it for an occasional static constant,
the JNI call overhead will not be a problem.

— John

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

Thanks Alan. Updates made and changes pushed.

regards,
Sean.

On 13/01/2020 18:50, Alan Bateman wrote:

On 13/01/2020 10:28, Seán Coffey wrote:
some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, 
the EventHelper utility class should never initialize the logging 
framework early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/
Thanks for the update. JAR file verification is tricky and important 
not to attempt to run arbitrary code while doing that, esp. anything 
that might need to load a class or resource from the class path. So I 
think the approach (in v5) looks okay.  A minor nit in JarFile is that 
it should be "static final".  Also you might want to replace or change 
the @summary in both tests to make it clearer that the tests attempt 
to trigger class loading from the class loader during JAR file 
verification.


-Alan.

Re: RFR: 8236850: Operations on constant List/Set.of(element) instances does not consistently constant fold

[Claes Redestad]

On 2020-01-13 22:18, Paul Sandoz wrote:

That’s fine, it was really for informational purposes in case it comes in handy 
later on.


Got it.


Speculating: if we had such a public method (with minimal dependencies) I 
suspect we would reach for it in this case even if overkill.  It reduces the 
cognitive load, but as a downside it would not afford us the opportunity of a 
slice Pi:-)


Not sure it'd pull its own weight as a public API, but refactoring to a
shared internal utility might be a good start?

/Claes

Re: RFR: 8236850: Operations on constant List/Set.of(element) instances does not consistently constant fold

[Paul Sandoz]

> On Jan 13, 2020, at 1:12 PM, Claes Redestad  wrote:
> 
> On 2020-01-13 20:49, Paul Sandoz wrote:
>> Looks good.
> 
> Thanks!
> 
>>   78 private static final Object[] EMPTY;
>> Is there a specific reason related to archiving that this is Object[] and 
>> not Object and instead assigned a value of new Object() ?
> 
> No, this was part of an experiment I did to have EMPTY_LIST etc use the
> EMPTY backing array, but that didn't pan out to anything but more code.
> I thought I backed out all changes, but must've slipped.
> 

Ok.


>> --
>> For information purposes: an alternative salt might be to use mix32 from 
>> Splittable random:
>> /**
>>  * Returns the 32 high bits of Stafford variant 4 mix64 function as int.
>>  */
>> private static int mix32(long z) {
>> z = (z ^ (z >>> 33)) * 0x62a9d9ed799705f5L;
>> return (int)(((z ^ (z >>> 28)) * 0xcb24d0a5c88c35b3L) >>> 32);
>> }
>> as in:
>> SALT = mix32(System.nanoTime());
> 
> Interesting - but as John points out we don't need a _really_ good salt
> for this, and I don't want to pull in SplittableRandom since we'd
> distort the bootstrap sequence if we did so here.
> 

That’s fine, it was really for informational purposes in case it comes in handy 
later on.

Speculating: if we had such a public method (with minimal dependencies) I 
suspect we would reach for it in this case even if overkill.  It reduces the 
cognitive load, but as a downside it would not afford us the opportunity of a 
slice Pi :-)

Paul.

Re: RFR: 8236850: Operations on constant List/Set.of(element) instances does not consistently constant fold

[Claes Redestad]

On 2020-01-13 20:49, Paul Sandoz wrote:

Looks good.


Thanks!



   78 private static final Object[] EMPTY;

Is there a specific reason related to archiving that this is Object[] and not 
Object and instead assigned a value of new Object() ?


No, this was part of an experiment I did to have EMPTY_LIST etc use the
EMPTY backing array, but that didn't pan out to anything but more code.
I thought I backed out all changes, but must've slipped.



--

For information purposes: an alternative salt might be to use mix32 from 
Splittable random:

/**
  * Returns the 32 high bits of Stafford variant 4 mix64 function as int.
  */
private static int mix32(long z) {
 z = (z ^ (z >>> 33)) * 0x62a9d9ed799705f5L;
 return (int)(((z ^ (z >>> 28)) * 0xcb24d0a5c88c35b3L) >>> 32);
}

as in:

SALT = mix32(System.nanoTime());


Interesting - but as John points out we don't need a _really_ good salt
for this, and I don't want to pull in SplittableRandom since we'd
distort the bootstrap sequence if we did so here.

/Claes

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Lance Andersen]

> On Jan 13, 2020, at 1:53 PM, Alan Bateman  wrote:
> 
> On 13/01/2020 10:31, Jaikiran Pai wrote:
>> Hello Christoph,
>> 
>> Setting inodes to null sounds fine to me and as you say since its usage
>> is already guarded by ensureOpen, IMO, it should be fine. I've now
>> updated the patch to set inodes to null in close() and the new updated
>> webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/
>> 
> This version looks good except it might be better if the comment just says 
> that it clears the inodes map to allow the keys/values be GC’ed.

I revised the comment to:



$ hg diff
diff -r 9338d0f52b2e 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 11:51:45 2020 -0500
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 15:24:37 2020 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -490,6 +490,14 @@
 def.end();
 }
 
+beginWrite();// lock and sync
+try {
+// Clear the map so that its keys & values can be garbage collected
+inodes = null;
+} finally {
+endWrite();
+}
+
 IOException ioe = null;
 synchronized (tmppaths) {
 for (Path p : tmppaths) {
$
———

I will push the change tomorrow barring any hiccups with Mach5 or additional 
comments….

Best
lance
> 
> -Alan

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: 8236850: Operations on constant List/Set.of(element) instances does not consistently constant fold

[John Rose]

On Jan 13, 2020, at 11:49 AM, Paul Sandoz  wrote:
> 
> For information purposes: an alternative salt might be to use mix32 from 
> Splittable random:
> 
> /**
> * Returns the 32 high bits of Stafford variant 4 mix64 function as int.
> */
> private static int mix32(long z) {
>z = (z ^ (z >>> 33)) * 0x62a9d9ed799705f5L;
>return (int)(((z ^ (z >>> 28)) * 0xcb24d0a5c88c35b3L) >>> 32);
> }
> 
> as in:
> 
> SALT = mix32(System.nanoTime());

Thank you for mentioning that!  I get this google hit on it:

https://blogs.oracle.com/dave/a-simple-prng-construction-idiom

There are also similar functions in xxHash.

https://github.com/easyaspi314/xxhash-clean/blob/master/xxhash64-ref.c#L104

Here’s a scary list:

https://en.wikipedia.org/wiki/List_of_hash_functions

I think “doing it right” is overkill for this one place, but I hope we can put
something like that into the JVM or some other low-level place for use
whenever we need a cheap pseudo-random nonce of some sort.

Once there’s an API for it I think we will reach for it a little more often.
Having a common engine means we can afford to tune its predictability
qualities.  In both directions:  resistance to attack, and reproducibility
for debugging.  In the case of Set.of, the former concern doesn’t apply
much, but the latter might.

— John

Re: RFR: 8236850: Operations on constant List/Set.of(element) instances does not consistently constant fold

[Paul Sandoz]

Looks good.

  78 private static final Object[] EMPTY;

Is there a specific reason related to archiving that this is Object[] and not 
Object and instead assigned a value of new Object() ?

--

For information purposes: an alternative salt might be to use mix32 from 
Splittable random:

/**
 * Returns the 32 high bits of Stafford variant 4 mix64 function as int.
 */
private static int mix32(long z) {
z = (z ^ (z >>> 33)) * 0x62a9d9ed799705f5L;
return (int)(((z ^ (z >>> 28)) * 0xcb24d0a5c88c35b3L) >>> 32);
}

as in:

SALT = mix32(System.nanoTime());

Paul.

> On Jan 13, 2020, at 6:48 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> I included John's suggested salt improvement, reverted to use an
> EMPTY object marker rather than 'this', and removed the
> extraneous blank line Roger pointed out:
> 
> http://cr.openjdk.java.net/~redestad/8236850/open.01/
> 
> /Claes
> 
> On 2020-01-11 04:19, John Rose wrote:
>> On Jan 10, 2020, at 6:48 PM, Claes Redestad > > wrote:
>>> 
>>> Yes. The XOR seems pointless with this approach, but some shifting
>>> might be good.
>> A single multiply mixes the bits of its inputs.  If one input is
>> “random” or “white” (50% 1’s irregularly spaced) then the output
>> will probably look similar.  The output bits are not equally mixed,
>> though:  The LSB depends only on two bits of the input, and the
>> MSB (barring rare carry-outs) depends mostly on two other bits
>> of the input.  But the middle bits depend on most of the input bits.
>> So, given the goal of making a very simple, good-enough mixing
>> expression to get a 32-bit salt, this one is relatively good:
>>long COLOR = 0x243F_6A88_85A3_08D3L;  // pi slice
>>long SEED = System.nanoTime();  // or command line value
>>int SALT = (int)( (COLOR * SEED) >> 16 );  // avoid LSB and MSB
>> In the longer term, I think the JVM should provide salt values
>> both for itself and (a few) core libs., and should allow those values
>> to be (partially) configured from the command line.  (Crash dumps
>> should report the SEED values used for reproducing the error.)
>> Given such a facility, it would be reasonable to upgrade it to use
>> better quality entropy sources and hashing (maybe optionally
>> crypto-quality, though I have reservations about that).  That
>> would make the variable behaviors unpredictable in practice.
>> Except when they are intentionally configured to be predictable.
>> An algorithm like xxHash would be a good starting point; it’s
>> cheap and salty.
>> — John

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Alan Bateman]

On 13/01/2020 10:31, Jaikiran Pai wrote:

Hello Christoph,

Setting inodes to null sounds fine to me and as you say since its usage
is already guarded by ensureOpen, IMO, it should be fine. I've now
updated the patch to set inodes to null in close() and the new updated
webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/

This version looks good except it might be better if the comment just 
says that it clears the inodes map to allow the keys/values be GC'ed.


-Alan

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Alan Bateman]

On 13/01/2020 10:28, Seán Coffey wrote:
some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, 
the EventHelper utility class should never initialize the logging 
framework early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/
Thanks for the update. JAR file verification is tricky and important not 
to attempt to run arbitrary code while doing that, esp. anything that 
might need to load a class or resource from the class path. So I think 
the approach (in v5) looks okay.  A minor nit in JarFile is that it 
should be "static final".  Also you might want to replace or change the 
@summary in both tests to make it clearer that the tests attempt to 
trigger class loading from the class loader during JAR file verification.


-Alan.

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Daniel Fuchs]

On 13/01/2020 17:21, Chris Hegarty wrote:

On 13 Jan 2020, at 17:19, Seán Coffey  wrote:

Thanks for the reviews. All callers of EventHelper log methods are ensuring 
that isLoggingSecurity() is true before proceeding. I've added an assert null 
check in the 4 logger methods to ensure expectations are in place.

http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/

Thanks. LGTM.



Likewise.

Thanks Seán!

best regards,

-- daniel

Re: [15] RFR: 8174270: Consolidate ICU sources in one location

[Steven R. Loomis]

approve

--
Steven R. Loomis | @srl295 | git.io/srl295



> El ene. 10, 2020, a las 4:02 p. m., naoto.s...@oracle.com escribió:
> 
> Hi Steven,
> 
> On 1/10/20 2:26 PM, Steven R. Loomis wrote:
>> in Norm2AllModes.java and UBiDiProps.java  and UCharacterProperty.java the 
>> stub name (icudt64b) should be calculated from the version number.   At 
>> least put “icudt64b” in one place and refer to it.
> 
> Good point. Modified as suggested:
> 
> https://cr.openjdk.java.net/~naoto/8174270/webrev.01/
> 
> Naoto
> 
>> OR keep the data file names unversioned, such as 
>> src/java.base/share/classes/jdk/internal/icu/impl/data/nfc.nrm
>> otherwise looks good
>> --
>> Steven R. Loomis | @srl295 | git.io/srl295 
>>> El ene. 10, 2020, a las 2:02 p. m., naoto.s...@oracle.com 
>>>  escribió:
>>> 
>>> Hi,
>>> 
>>> Please review the fix to the following issue:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8174270
>>> 
>>> The proposed changeset is located at:
>>> 
>>> https://cr.openjdk.java.net/~naoto/8174270/webrev.00/
>>> 
>>> Although the number of modified files are huge, the change is simply moving 
>>> the scattered ICU sources in various locations under jdk.internal.icu 
>>> package, keeping the original ICU's source tree structure.
>>> 
>>> Naoto

Re: RFR: JDK-8236125: Windows (MSVC 2013) build fails in jpackage: Need to include strsafe.h after tchar.h

[Aleksey Shipilev]

Thank you, pushed.

-Aleksey

On 1/13/20 5:41 PM, Andy Herrick wrote:
> sure
> 
> /Andy
> 
> On 1/13/2020 11:03 AM, Aleksey Shipilev wrote:
>> You good for this to go to jdk14?
>>
>> On 1/13/20 4:15 PM, Andy Herrick wrote:
>>> I'm fine with this change.
>>>
>>> /Andy
>>>
>>> On 12/23/2019 6:52 PM, Alex Kashchenko wrote:
 Hi,

 Please review this minor change to jpackage, that is required for
 successful compilation with VS2013 toolchain:

 Bug: https://bugs.openjdk.java.net/browse/JDK-8236125
 Webrev: http://cr.openjdk.java.net/~akasko/jdk/8236125/webrev.00/

 Some details on this problem: https://stackoverflow.com/a/24302290/314015

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Chris Hegarty]

> On 13 Jan 2020, at 17:19, Seán Coffey  wrote:
> 
> Thanks for the reviews. All callers of EventHelper log methods are ensuring 
> that isLoggingSecurity() is true before proceeding. I've added an assert null 
> check in the 4 logger methods to ensure expectations are in place.
> 
> http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/

Thanks. LGTM.

-Chris.

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

Thanks for the reviews. All callers of EventHelper log methods are 
ensuring that isLoggingSecurity() is true before proceeding. I've added 
an assert null check in the 4 logger methods to ensure expectations are 
in place.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/

Hope this helps,
Sean.

On 13/01/2020 14:38, Daniel Fuchs wrote:

On 13/01/2020 14:06, Chris Hegarty wrote:
I’m going to ask, since I cannot find the answer myself. Why are some 
securityLogger::log invocations guarded with isLoggingSecurity, and 
others not? With this change shouldn’t all invocations be guarded, 
since it is isLoggingSecurity that assigns securityLogger a value?


Argh! Well spotted chris!

-- daniel

Re: RFR: JDK-8236125: Windows (MSVC 2013) build fails in jpackage: Need to include strsafe.h after tchar.h

[Andy Herrick]

sure

/Andy

On 1/13/2020 11:03 AM, Aleksey Shipilev wrote:

You good for this to go to jdk14?

On 1/13/20 4:15 PM, Andy Herrick wrote:

I'm fine with this change.

/Andy

On 12/23/2019 6:52 PM, Alex Kashchenko wrote:

Hi,

Please review this minor change to jpackage, that is required for
successful compilation with VS2013 toolchain:

Bug: https://bugs.openjdk.java.net/browse/JDK-8236125
Webrev: http://cr.openjdk.java.net/~akasko/jdk/8236125/webrev.00/

Some details on this problem: https://stackoverflow.com/a/24302290/314015

Re: [15] RFR: 8174270: Consolidate ICU sources in one location

[Roger Riggs]

Looks fine to me also.

Roger

On 1/10/20 11:21 PM, Joe Wang wrote:

I see. It's all good to me then.

Best,
Joe

On 1/10/20 4:04 PM, naoto.s...@oracle.com wrote:

Hi Joe,

That data file seems no longer included in the ICU4J package (as of 
64.2), thus I left it as it is.


Naoto

On 1/10/20 2:57 PM, Joe Wang wrote:

Is there a reason why uidna.spp was left out of the move?

-Joe

On 1/10/20 2:02 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following issue:

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

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8174270/webrev.00/

Although the number of modified files are huge, the change is 
simply moving the scattered ICU sources in various locations under 
jdk.internal.icu package, keeping the original ICU's source tree 
structure.


Naoto

Re: RFR: JDK-8236125: Windows (MSVC 2013) build fails in jpackage: Need to include strsafe.h after tchar.h

[Aleksey Shipilev]

You good for this to go to jdk14?

On 1/13/20 4:15 PM, Andy Herrick wrote:
> I'm fine with this change.
> 
> /Andy
> 
> On 12/23/2019 6:52 PM, Alex Kashchenko wrote:
>> Hi,
>>
>> Please review this minor change to jpackage, that is required for 
>> successful compilation with VS2013 toolchain:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8236125
>> Webrev: http://cr.openjdk.java.net/~akasko/jdk/8236125/webrev.00/
>>
>> Some details on this problem: https://stackoverflow.com/a/24302290/314015
>>
> 


-- 
Thanks,
-Aleksey

RFR: 8236641: Improve Set.of(...).iterator() warmup characteristics

[Claes Redestad]

Hi,

we're doing plenty of iterations over Set.of() instances during
bootstrap, which makes these operations show up prominently in
startup profiles. This patch refactors a few hot methods to get a
measureable startup improvement without regressing on targeted
microbenchmarks.

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

(The patch is baselined against 8236850)

Thanks!

/Claes

Re: RFR: JDK-8236125: Windows (MSVC 2013) build fails in jpackage: Need to include strsafe.h after tchar.h

[Andy Herrick]

I'm fine with this change.

/Andy

On 12/23/2019 6:52 PM, Alex Kashchenko wrote:

Hi,

Please review this minor change to jpackage, that is required for 
successful compilation with VS2013 toolchain:


Bug: https://bugs.openjdk.java.net/browse/JDK-8236125
Webrev: http://cr.openjdk.java.net/~akasko/jdk/8236125/webrev.00/

Some details on this problem: https://stackoverflow.com/a/24302290/314015

Re: RFR: 8236850: Operations on constant List/Set.of(element) instances does not consistently constant fold

[Claes Redestad]

Hi,

I included John's suggested salt improvement, reverted to use an
EMPTY object marker rather than 'this', and removed the
extraneous blank line Roger pointed out:

http://cr.openjdk.java.net/~redestad/8236850/open.01/

/Claes

On 2020-01-11 04:19, John Rose wrote:
On Jan 10, 2020, at 6:48 PM, Claes Redestad > wrote:


Yes. The XOR seems pointless with this approach, but some shifting
might be good.


A single multiply mixes the bits of its inputs.  If one input is
“random” or “white” (50% 1’s irregularly spaced) then the output
will probably look similar.  The output bits are not equally mixed,
though:  The LSB depends only on two bits of the input, and the
MSB (barring rare carry-outs) depends mostly on two other bits
of the input.  But the middle bits depend on most of the input bits.

So, given the goal of making a very simple, good-enough mixing
expression to get a 32-bit salt, this one is relatively good:

    long COLOR = 0x243F_6A88_85A3_08D3L;  // pi slice
    long SEED = System.nanoTime();  // or command line value
    int SALT = (int)( (COLOR * SEED) >> 16 );  // avoid LSB and MSB

In the longer term, I think the JVM should provide salt values
both for itself and (a few) core libs., and should allow those values
to be (partially) configured from the command line.  (Crash dumps
should report the SEED values used for reproducing the error.)
Given such a facility, it would be reasonable to upgrade it to use
better quality entropy sources and hashing (maybe optionally
crypto-quality, though I have reservations about that).  That
would make the variable behaviors unpredictable in practice.
Except when they are intentionally configured to be predictable.
An algorithm like xxHash would be a good starting point; it’s
cheap and salty.

— John

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Daniel Fuchs]

On 13/01/2020 14:06, Chris Hegarty wrote:

I’m going to ask, since I cannot find the answer myself. Why are some 
securityLogger::log invocations guarded with isLoggingSecurity, and others not? 
With this change shouldn’t all invocations be guarded, since it is 
isLoggingSecurity that assigns securityLogger a value?


Argh! Well spotted chris!

-- daniel

Re: RFR 8222100: tools/jimage/JImageTest.java time out

[James Laskey]

+1

On the road.

> On Jan 13, 2020, at 10:25 AM, sundararajan.athijegannat...@oracle.com wrote:
> 
> Bumping the default timeout (other tests in the same dir have similar 
> timeout settings).
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8222100
> 
> Webrev: http://cr.openjdk.java.net/~sundar/8222100/webrev.00/
> 
> Thanks,
> 
> -Sundar
> 

RFR 8222100: tools/jimage/JImageTest.java time out

[sundararajan . athijegannathan]

Bumping the default timeout (other tests in the same dir have similar 
timeout settings).


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

Webrev: http://cr.openjdk.java.net/~sundar/8222100/webrev.00/

Thanks,

-Sundar

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Chris Hegarty]

> On 13 Jan 2020, at 13:14, Daniel Fuchs  wrote:
> 
> On 13/01/2020 10:28, Seán Coffey wrote:
>> some off line comments suggested that I could move the jar initialization 
>> checks to the EventHelper class. With that in place, the EventHelper utility 
>> class should never initialize the logging framework early during jar 
>> initialization.
>> http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/
> 
> Looks good to me Seán. Probably safer than the other alternatives.

+1

I’m going to ask, since I cannot find the answer myself. Why are some 
securityLogger::log invocations guarded with isLoggingSecurity, and others not? 
With this change shouldn’t all invocations be guarded, since it is 
isLoggingSecurity that assigns securityLogger a value?

-Chris

Re: RFR (14) 8235837: Memory access API refinements

[Chris Hegarty]

> On 9 Jan 2020, at 18:36, Maurizio Cimadamore  
> wrote:
> 
> New revision:
> 
> http://cr.openjdk.java.net/~mcimadamore/8235837_v2
> 
> delta from previous iteration:
> 
> http://cr.openjdk.java.net/~mcimadamore/8235837_v2_delta
> 
> javadoc
> 
> http://cr.openjdk.java.net/~mcimadamore/8235837_v2_javadoc
> 
> specdiff
> 
> http://cr.openjdk.java.net/~mcimadamore/8235837_v2_specdiff/overview-summary.html

Thank you for the updates Maurizio. The changes look good to me. ( I’ve added 
myself as reviewer on the CSR )

-Chris.

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Daniel Fuchs]

On 13/01/2020 10:28, Seán Coffey wrote:
some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, the 
EventHelper utility class should never initialize the logging framework 
early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/


Looks good to me Seán. Probably safer than the other alternatives.
  46 private static boolean loggingSecurity;
that should be volatile too.


best regard,

-- daniel



regards,
Sean.

On 16/12/2019 14:15, Seán Coffey wrote:
The recent crypto event logging mechanism (JDK-8148188) has introduced 
a regression whereby the System Logger may be invoked too early in the 
bootstrap phase. This causes issue when JarFile objects are locked by 
JarFile verifier initialization code. The logging work records an X509 
Certificate which is used during the jar file 
verification/initialization phase and hence leads to an early 
System.Logger call.


One thread invokes the initialization of the Logger framework via 
ServiceLoader and waits to lock a JarFile in use via another thread. 
Unfortunately that other thread is also waiting for the System Logger 
to initialize. For now, I think we can avoid the early Logger 
initialization via use of a ThreadLocal. I've tried reproducing the 
reported issue through manual and automated tests but to no avail. 
I've added a new ServiceLoader test which has concurrent threads. One 
is loading providers and another is initializing JarFile verifiers. 
Hope it helps improve code coverage for the future.


JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/

Re: [14] RFR (S) 8236920: 32-bit build failures in libNativeAccess.c

[Maurizio Cimadamore]

Hi Alex,
has this been addressed by Nick changes (JDK-8236634) ?

Thanks
Maurizio


On 12/01/2020 11:15, Aleksey Shipilev wrote:

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

Fix:
   https://cr.openjdk.java.net/~shade/8236920/webrev.01/

This breaks x86_32 test builds. I would like to have this fix in 14.

Testing: Linux {x86_64, x86_32} builds; java/foreign (still passes on x86_64; 
still fails on x86_32,
seems unrelated to this build breakage); jdk14-submit (still stuck somewhere!)

Re: [14] RFR (S) 8236920: 32-bit build failures in libNativeAccess.c

[Aleksey Shipilev]

On 1/13/20 1:22 PM, Maurizio Cimadamore wrote:
> Hi Alex,
> has this been addressed by Nick changes (JDK-8236634) ?

Seems to be, I already closed my issue as duplicate.

-- 
Thanks,
-Aleksey

Re: [14] RFR (S) 8236920: 32-bit build failures in libNativeAccess.c

[Maurizio Cimadamore]

Thanks

Maurizio

On 13/01/2020 12:25, Aleksey Shipilev wrote:

On 1/13/20 1:22 PM, Maurizio Cimadamore wrote:

Hi Alex,
has this been addressed by Nick changes (JDK-8236634) ?

Seems to be, I already closed my issue as duplicate.

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Hello Christoph,

Setting inodes to null sounds fine to me and as you say since its usage
is already guarded by ensureOpen, IMO, it should be fine. I've now
updated the patch to set inodes to null in close() and the new updated
webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/

-Jaikiran

On 13/01/20 12:26 pm, Langer, Christoph wrote:
> Hi,
>
> I'm wondering whether we shouldn't just do "inodes = null;"? I guess this is 
> cheaper and accesses to the inodes map on a closed filesystem object should 
> not happen anyway (e.g. all is guarded by ensureOpen()). Other than that, the 
> change looks fine.
>
> Cheers
> Christoph
>
>> -Original Message-
>> From: nio-dev  On Behalf Of Jaikiran
>> Pai
>> Sent: Samstag, 11. Januar 2020 11:24
>> To: Alan Bateman ; nio-...@openjdk.java.net
>> Cc: core-libs-dev@openjdk.java.net
>> Subject: Re: RFR 7143743: (zipfs) Potential memory leak with zip provider
>>
>> Hello Alan,
>>
>> On 11/01/20 3:37 pm, Alan Bateman wrote:
>>> On 11/01/2020 09:51, Jaikiran Pai wrote:
 :

 The commit here fixes that issue by simply clearing the "inodes" map in
 the jdk.nio.zipfs.ZipFileSystem.close() method. I have checked the usage
 of the "inodes" map and from what I see, it's usage in various places is
 guarded by "ensureOpen" checks, which means that once the
>> ZipFileSystem
 instance is closed, the contents of these "inodes" map is no longer
 relevant and hence clearing it shouldn't cause any issues.

>>> Clearing the inodes map should be okay for cases where something is
>>> holding a reference to a closed zip file system. However, you should
>>> look at beginWrite/endWrite so that all access to the map is
>>> consistently synchronized.
>>>
>> Thank you very much for that input - I hadn't considered the concurrency
>> aspect of it. Based on your input and after looking at the usage of the
>> "inodes", I have now updated the patch to use proper locks during the
>> clearing of the inodes. The updated webrev is available at
>> https://cr.openjdk.java.net/~jpai/webrev/7143743/2/webrev/
>>
>> -Jaikiran

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, the 
EventHelper utility class should never initialize the logging framework 
early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/

regards,
Sean.

On 16/12/2019 14:15, Seán Coffey wrote:
The recent crypto event logging mechanism (JDK-8148188) has introduced 
a regression whereby the System Logger may be invoked too early in the 
bootstrap phase. This causes issue when JarFile objects are locked by 
JarFile verifier initialization code. The logging work records an X509 
Certificate which is used during the jar file 
verification/initialization phase and hence leads to an early 
System.Logger call.


One thread invokes the initialization of the Logger framework via 
ServiceLoader and waits to lock a JarFile in use via another thread. 
Unfortunately that other thread is also waiting for the System Logger 
to initialize. For now, I think we can avoid the early Logger 
initialization via use of a ThreadLocal. I've tried reproducing the 
reported issue through manual and automated tests but to no avail. 
I've added a new ServiceLoader test which has concurrent threads. One 
is loading providers and another is initializing JarFile verifiers. 
Hope it helps improve code coverage for the future.


JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/

Re: [PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy

[Severin Gehwolf]

Hi Bob,

On Fri, 2020-01-10 at 11:50 -0500, Bob Vandette wrote:
> Severin,
> 
> The changes look ok to me.  I assume you’ve run the container tests
> on a cgroupv2 and cgroupv1 enabled system, right?

Yes, I have.

> You’re going to have to find a “R” reviewer.

Indeed.

> What’s the status of the hotspot cgroupv2 changes.  Have these been
> reviewed?

Latest webvrev was v5 here:
http://mail.openjdk.java.net/pipermail/hotspot-dev/2019-November/040120.html

It included the memory_max_usage_in_bytes synthesized hack for cgroups
v2, though. Should I remove it there as well? I'd be in favor of that.
Otherwise nothing new. It's ready from my perspective other than
finding another Reviewer for it.

Thanks,
Severin

> Bob.
> 
> 
> > On Jan 9, 2020, at 2:51 PM, Severin Gehwolf 
> > wrote:
> > 
> > Hi,
> > 
> > On Fri, 2020-01-03 at 15:37 -0500, Bob Vandette wrote:
> > > Here are a few comments from scanning the webrev.
> > > 
> > > 
> > > It looks like you can remove line 151
> > > 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06-incremental/webrev/src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java.sdiff.html
> > > 
> > > 151 int[] ints = new int[0];
> > > 
> > > —
> > > 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06-incremental/webrev/src/java.base/share/classes/jdk/internal/platform/Metrics.java.sdiff.html
> > > 
> > > There are several comments stating that -2 == unlimited.  This is
> > > not
> > > the case.
> > > 
> > > @return count of elapsed periods or -2 if the quota is unlimited.
> > > 
> > > —
> > > 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/test/jdk/jdk/internal/platform/docker/TestDockerMemoryMetrics.java.sdiff.html
> > > 
> > > Shouldn’t you just check for cgroupv2 before trying to run the
> > > testKernelMemoryLimit and testOomKillFlag tests?
> > > 
> > > —
> > > 
> > > There are a few places where getPerCpuUsage is returning “new
> > > long[0]” but you changed the interface to expect null
> > > for not supported.
> > > 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java.html
> > > 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java.html
> > > 
> > > You probably need to check all users of the APIs which used to
> > > return
> > > array[0] which now return null to make sure you
> > > don’t get null pointer exceptions.
> > > 
> > > One example is testCpuConsumption.
> > > 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java.html
> > > 
> > > Also, 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV2.java.html:167
> > > 
> > > 
> > > You’ll also have to update copyrights to 2020.
> > 
> > Thanks for the review! Should all be fixed now. Updated webrev:
> > 
> > incremental: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/07/incremental/webrev/
> > full: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/07/webrev/
> > 
> > Note: I'll go through touched files and update copyright dates to
> > 2020.
> > Not all have been updated in the full webrev. It'll be done though.
> > 
> > Thanks,
> > Severin
> > 
> > > Bob.
> > > 
> > > 
> > > > On Dec 20, 2019, at 9:50 AM, Severin Gehwolf <
> > > > sgehw...@redhat.com>
> > > > wrote:
> > > > 
> > > > Hi Bob,
> > > > 
> > > > Sorry for the delay to get this updated.
> > > > 
> > > > Changes done in this latest webrev:
> > > > 
> > > >  1. Rebased on top of 8226575: OperatingSystemMXBean should be
> > > > made
> > > > container aware. File read ops now use privileged code.
> > > >  2. Warning for mixed cgroup controllers and returning null for
> > > > metrics.
> > > >  3. Added implementations for getBlkIOServiceCount() and
> > > > getBlkIOServiced() using sum of rios/wios and rbytes/wbytes
> > > > across
> > > > devices. Added impl for getTcpMemoryUsage()
> > > >  4. Returning -2 for not supported (if the metric would return
> > > > long).
> > > > boolean metrics now return Boolean and null if not
> > > > supported.
> > > > Same
> > > > for array return types. null if not supported for symmetry.
> > > >  5. -XshowSettings:system output has been updated to return
> > > > "N/A"
> > > > for
> > > > when a metric is not available. E.g. "Kernel OOM killer
> > > > enabled"
> > > > Boolean.
> > > >  6. Tests have been updated to reflect this.
> > > > 
> > > > webrev:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06/webrev/
> > > > incremental webrev:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-823/06-incremental/webrev/
> > > > 
> > > > Testing: Docker tests and podman testing on cgroupsv2. I'll