Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

On 6 February 2020 21:13:52 GMT, Peter Levart  wrote:
>
>
>On 2/6/20 6:54 PM, Seán Coffey wrote:
>> the current proposal is still:
>> http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/
>
>But wasn't this one already better:
>
>https://cr.openjdk.java.net/~coffeys/webrev.8223260.v3/webrev/

D'oh! You're right Peter. v3 is my preference (and most up to date) at the 
moment. I'll look at Alan's comment in the morning. I could create a custom 
exception if the current UndeclaredThrowableException is not a good use.

Regards,
Sean.

>Or do you prefer the v2 so that spec change and behavior change would
>be 
>synchronized?
>
>
>Regards, Peter
>
>>
>> I'd like to make the specification change in a follow on bug ID (if 
>> that works for people)
>>
>> Regards,
>> Sean.
>>
>> On 06/02/20 17:49, Alan Bateman wrote:
>>> On 06/02/2020 15:32, Seán Coffey wrote:
 InitialContextFactory itself is an extremely simple interface with 
 one method. I've browsed some code bases and could only find a
>small 
 number of such Factories implementations with simple logic to
>return 
 a Context. Applications will most likely delegate to the same 
 Factory over and over also (albeit, it's all controlled by
>HashTable 
 parameters). The new caching logic should help memory pressure here
>
 and not hinder it. I'm not seeing a major concern with current 
 solution as a result.
>>> Okay, although I could imagine a non-JDK InitialContextFactory 
>>> implementation keeping a graph of objects alive. I saw your mail 
>>> about separating the clarification in the APIs docs so does it mean 
>>> the proposal on the table is the last webrev or is there a new
>version?
>>>
>>> -Alan
>>

Re: RFR: 8238599: Refactor and simplify implAddOpensToAllUnnamed

[Claes Redestad]

On 2020-02-06 18:52, Alan Bateman wrote:

Webrev: http://cr.openjdk.java.net/~redestad/8238599/open.00/
This looks good, just a minor nit that declaring emptySet without any 
context looks a bit strange. If you move it to below the comment "open 
specific packages in the system modules" then it will be clearer that 
it's used in this part of the method.


Thanks, will fix before push.

/Claes

Re: RFR: 8237484: Improve module system bootstrap

[Claes Redestad]

On 2020-02-06 18:56, Alan Bateman wrote:

On 06/02/2020 12:35, Claes Redestad wrote:

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

This looks good to me.


Thanks!

/Claes

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Peter Levart]

On 2/6/20 6:54 PM, Seán Coffey wrote:

the current proposal is still:
http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/


But wasn't this one already better:

https://cr.openjdk.java.net/~coffeys/webrev.8223260.v3/webrev/

Or do you prefer the v2 so that spec change and behavior change would be 
synchronized?



Regards, Peter



I'd like to make the specification change in a follow on bug ID (if 
that works for people)


Regards,
Sean.

On 06/02/20 17:49, Alan Bateman wrote:

On 06/02/2020 15:32, Seán Coffey wrote:
InitialContextFactory itself is an extremely simple interface with 
one method. I've browsed some code bases and could only find a small 
number of such Factories implementations with simple logic to return 
a Context. Applications will most likely delegate to the same 
Factory over and over also (albeit, it's all controlled by HashTable 
parameters). The new caching logic should help memory pressure here 
and not hinder it. I'm not seeing a major concern with current 
solution as a result.
Okay, although I could imagine a non-JDK InitialContextFactory 
implementation keeping a graph of objects alive. I saw your mail 
about separating the clarification in the APIs docs so does it mean 
the proposal on the table is the last webrev or is there a new version?


-Alan

Re: 6202130: Need to handle UTF-8 values and break up lines longer than 72 bytes

[Lance Andersen]

Hi Philipp,

Thank you for the reminder.  Your email came in during the holidays so it kind 
of was lost in my backlog.

First, thank you for the proposed patch.  This will take some time to review 
but it is on the list


Best
Lance

> On Feb 6, 2020, at 1:37 PM, Philipp Kunz  wrote:
> 
> Hi,
> 
> I recently submitted two patches related to jar manifests and UTF-8 and
> haven't got any reaction so far. I understand and appreciate that
> everyone has not time for every wish and my enquiry is certainly not
> urgent, but still, may I gently ask if I may continue to hope for any
> progress, have missed anything, or if this of no interest at all?
> 
> Unfortunately the line breaks in the previous mail went bad which is
> why I paste the text again below and hope it looks nicer this time.
> 
> Regards,
> Philipp
> 
> 
> 
> [1] 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064190.html
>  
> 
> [2] 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-December/064149.html
>  
> 
> 
> 
> On Thu, 2019-12-26 at 17:50 +0100, Philipp Kunz wrote:
> Hi,
> The specification says, a line break in a manifest can occur before or
> after a Unicode character encoded in UTF-8.
> ...  value: SPACE *otherchar newline *continuation 
> continuation:  SPACE *otherchar newline...  otherchar: any UTF-8
> character except NUL, CR and LF
> The current implementation breaks manifest lines at 72 bytes regardless
> of how the bytes around the break are part of a sequence of bytes
> encoding a character. Code points may use up to four bytes when encoded
> in UTF-8. Manifests with line breaks inside of sequences of bytes
> encoding Unicode characters in UTF-8 with more than one bytes not only
> are invalid UTF-8 but also look ugly in text editors. For example, a
> manifest could look like this:
> Manifest-Version: 1.0Some-Key: Some languages have decorated
> characters, for example: espa? ?ol
> Below code produces a result as seen above with some unexpected
> question marks where the encoding is invalid:
> import java.util.jar.Manifest;import java.util.jar.Attributes;import
> static java.util.jar.Attributes.Name;
> public class CharacterBrokenDemo1 {public static void main(String[]
> args) throws Exception{Manifest mf = new
> Manifest();Attributes attrs =
> mf.getMainAttributes();attrs.put(Name.MANIFEST_VERSION,
> "1.0");attrs.put(new Name("Some-Key"),  "Some
> languages have decorated characters, " +   "for
> example: español"); // or
> "espa\u00D1ol"mf.write(System.out);}}
> This is of course an example written with actual question marks to get
> a valid text for this message. The trick here is that "Some-Key" to
> "example :espa" amounts to exactly one byte less encoded in UTF-8 than
> would fit on one line with the 72 byte limit so that the subsequent
> character encoded with two bytes gets broken inside of the sequence of
> two bytes for this character across a continuation line break.
> When decoding the resulting bytes from UTF-8 as one whole string, the
> two question marks will not fit together again even if the line break
> with the continuation space is removed. However, Manifest::read removes
> the continuation line breaks ("\r\n ") before decoding the manifest
> header value from UTF-8 and hence can reproduce the original value.
> Characters encoded in UTF-8 can not only span up to four bytes for one
> code point each, there are also combining characters or classes thereof
> or combining diacritical marks or whatever the appropriate term could
> be, that combine more than one code point into what is usually
> experienced and referred to as a character.
> The term character really gets ambiguous at this point. I wouldn't know
> what the specification actually refers to with that term "character".
> So rather than diving in too much specification or any sorts of theory,
> let's look at another example:
> import java.util.jar.Manifest;import java.util.jar.Attributes;import
> static java.util.jar.Attributes.Name;
> public class DemoCharacterBroken2 {public static void main(String[]
> args) throws Exception{Manifest mf = new
> Manifest();Attributes attrs =
> mf.getMainAttributes();attrs.put(Name.MANIFEST_VERSION,
> "1.0");attrs.put(new Name("Some-Key"), " ".repeat(53) +
> "Angstro\u0308m");mf.write(System.out);}}
> which produces console output as follows:
> Manifest-Version: 1.0Some-Key: Angstro ̈m
> (In case this does not display well, the diaeresis is on the m on the
> last line)
> When the whole Manifest is decoded from UTF-8 as one big single string
> and continuation line breaks are not removed until after UTF-8 decoding
> the whole manifest, the diaeresis (umlaut, two points 

Re: 6202130: Need to handle UTF-8 values and break up lines longer than 72 bytes

[Philipp Kunz]

Hi,

I recently submitted two patches related to jar manifests and UTF-8 and
haven't got any reaction so far. I understand and appreciate that
everyone has not time for every wish and my enquiry is certainly not
urgent, but still, may I gently ask if I may continue to hope for any
progress, have missed anything, or if this of no interest at all?

Unfortunately the line breaks in the previous mail went bad which is
why I paste the text again below and hope it looks nicer this time.

Regards,
Philipp



[1] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064190.html
[2] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-December/064149.html


On Thu, 2019-12-26 at 17:50 +0100, Philipp Kunz wrote:
Hi,
The specification says, a line break in a manifest can occur before or
after a Unicode character encoded in UTF-8.
...  value: SPACE *otherchar newline *continuation 
continuation:  SPACE *otherchar newline...  otherchar: any UTF-8
character except NUL, CR and LF
The current implementation breaks manifest lines at 72 bytes regardless
of how the bytes around the break are part of a sequence of bytes
encoding a character. Code points may use up to four bytes when encoded
in UTF-8. Manifests with line breaks inside of sequences of bytes
encoding Unicode characters in UTF-8 with more than one bytes not only
are invalid UTF-8 but also look ugly in text editors. For example, a
manifest could look like this:
Manifest-Version: 1.0Some-Key: Some languages have decorated
characters, for example: espa? ?ol
Below code produces a result as seen above with some unexpected
question marks where the encoding is invalid:
import java.util.jar.Manifest;import java.util.jar.Attributes;import
static java.util.jar.Attributes.Name;
public class CharacterBrokenDemo1 {public static void main(String[]
args) throws Exception{Manifest mf = new
Manifest();Attributes attrs =
mf.getMainAttributes();attrs.put(Name.MANIFEST_VERSION,
"1.0");attrs.put(new Name("Some-Key"),  "Some
languages have decorated characters, " +   "for
example: español"); // or
"espa\u00D1ol"mf.write(System.out);}}
This is of course an example written with actual question marks to get
a valid text for this message. The trick here is that "Some-Key" to
"example :espa" amounts to exactly one byte less encoded in UTF-8 than
would fit on one line with the 72 byte limit so that the subsequent
character encoded with two bytes gets broken inside of the sequence of
two bytes for this character across a continuation line break.
When decoding the resulting bytes from UTF-8 as one whole string, the
two question marks will not fit together again even if the line break
with the continuation space is removed. However, Manifest::read removes
the continuation line breaks ("\r\n ") before decoding the manifest
header value from UTF-8 and hence can reproduce the original value.
Characters encoded in UTF-8 can not only span up to four bytes for one
code point each, there are also combining characters or classes thereof
or combining diacritical marks or whatever the appropriate term could
be, that combine more than one code point into what is usually
experienced and referred to as a character.
The term character really gets ambiguous at this point. I wouldn't know
what the specification actually refers to with that term "character".
So rather than diving in too much specification or any sorts of theory,
let's look at another example:
import java.util.jar.Manifest;import java.util.jar.Attributes;import
static java.util.jar.Attributes.Name;
public class DemoCharacterBroken2 {public static void main(String[]
args) throws Exception{Manifest mf = new
Manifest();Attributes attrs =
mf.getMainAttributes();attrs.put(Name.MANIFEST_VERSION,
"1.0");attrs.put(new Name("Some-Key"), " ".repeat(53) +
"Angstro\u0308m");mf.write(System.out);}}
which produces console output as follows:
Manifest-Version: 1.0Some-Key: Angstro ̈m
(In case this does not display well, the diaeresis is on the m on the
last line)
When the whole Manifest is decoded from UTF-8 as one big single string
and continuation line breaks are not removed until after UTF-8 decoding
the whole manifest, the diaeresis (umlaut, two points above, u0308)
apparently kind of jumps onto the following letter m because somehow it
cannot be combined with the preceding space. The UTF-8 decoder (all of
my editors I tried, not only Eclipse and its console view, also less,
gedit, cat and terminal) somehow tries to fix that but the diaeresis
may not necessarily jump back on the "o" where it originally belonged
by removing the continuation line break ("\r\n ") after UTF-8 decoding
has taken place, at least that did not work for me.
Hence, ideally combining diacritical marks should better not be
separated from whatever they combine with when breaking manifest lines
onto a 

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Alan Bateman]

On 06/02/2020 17:54, Seán Coffey wrote:

the current proposal is still:
http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/
Thanks. This mostly looks good to me, I'm just wondering if there is a 
better exception to use to carry the checked exception as 
UndeclaredThrowableException is specified for proxies.




I'd like to make the specification change in a follow on bug ID (if 
that works for people)

That's okay as that part wouldn't be appropriate to backport.

-Alan

Re: RFR: 8237484: Improve module system bootstrap

[Alan Bateman]

On 06/02/2020 12:35, Claes Redestad wrote:

Hi,

a few module bootstrap micro-optimizations.

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

This looks good to me.

-Alan.

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

the current proposal is still:
http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/

I'd like to make the specification change in a follow on bug ID (if that 
works for people)


Regards,
Sean.

On 06/02/20 17:49, Alan Bateman wrote:

On 06/02/2020 15:32, Seán Coffey wrote:
InitialContextFactory itself is an extremely simple interface with 
one method. I've browsed some code bases and could only find a small 
number of such Factories implementations with simple logic to return 
a Context. Applications will most likely delegate to the same Factory 
over and over also (albeit, it's all controlled by HashTable 
parameters). The new caching logic should help memory pressure here 
and not hinder it. I'm not seeing a major concern with current 
solution as a result.
Okay, although I could imagine a non-JDK InitialContextFactory 
implementation keeping a graph of objects alive. I saw your mail about 
separating the clarification in the APIs docs so does it mean the 
proposal on the table is the last webrev or is there a new version?


-Alan

Re: [14] RFR (XXS) 8238605: Correct the CLDR version number in cldr.md files

[Alan Bateman]

On 06/02/2020 16:50, naoto.s...@oracle.com wrote:

Hello,

Please review this extra small fix for this issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8238605/webrev.00/

It is simply updating the version number in cldr.md files, which 
should have been done with JDK-8231273.

This seems to be a "must-fix" to the license file so I think it's okay.

-Alan

Re: RFR: 8238599: Refactor and simplify implAddOpensToAllUnnamed

[Alan Bateman]

On 06/02/2020 16:39, Claes Redestad wrote:

Hi,

by refactoring implAddOpensToAllUnnamed to take the two sets that
comprise the packages we iterate over, we can make some simplifications,
pre-size HashMaps better etc, adding up to a nice little startup
improvement.

Webrev: http://cr.openjdk.java.net/~redestad/8238599/open.00/
Bug:    https://bugs.openjdk.java.net/browse/JDK-8238599
This looks good, just a minor nit that declaring emptySet without any 
context looks a bit strange. If you move it to below the comment "open 
specific packages in the system modules" then it will be clearer that 
it's used in this part of the method.


-Alan

Re: RFR: 8237878: Improve ModuleLoaderMap datastructures

[forax]

- Mail original -
> De: "Claes Redestad" 
> À: "Remi Forax" 
> Cc: "core-libs-dev" 
> Envoyé: Jeudi 6 Février 2020 14:48:38
> Objet: Re: RFR: 8237878: Improve ModuleLoaderMap datastructures

> Hi Rémi,
> 
> 
> On 2020-02-06 14:08, Remi Forax wrote:
>> Hi Claes,
>> In ArchivedModuleGraph, there is no point to take the mainModule as parameter
>> given that it should always be null.
> 
> right, I've been thinking this should be simplified for now. Once we
> implement support for archiving non-default scenarios the internal
> datastructure will need a refactoring anyhow, so let's cut things down
> to size.
> 
>> 
>> In ModuleLoaderMap, the constants PLATFORM_LOADER_INDEX and APP_LOADER_INDEX
>> should be moved inside the nested class Mapper given that there are only 
>> needed
>> by the Mapper
>> and mappingFunction should be moved inside the class Mapper too.
> 
> Done.
> 
>> 
>> in mappingFunction, i think that if you are using 'var', you should using
>> everywhere in the method,
>> to avoid to have a weird mix between local variables declared with and 
>> without
>> var.
>> so either map is not declared with var or resolvedModule and nm are declared
>> with var.
> 
> I disagree var usage should be an all or nothing ordeal, rather it
> should be used when the type information is readily available, e.g.,
> by being stated in detail on the RHS.
> 
> The exact type in the other cases would be somewhat muddied: maybe
> name() is "obviously" a String, but what type the elements in
> cf.modules() are is not at all obvious, and making code less obvious
> should be avoided.

If your variable have a name like resolvedModule then the type ResolvedModule 
doesn't give you more info.
Obviously, it doesn't work if the variable name is an abbreviation, but instead 
of not using var, i think it's better to change the variable name.

> 
> New webrev:
> 
> http://cr.openjdk.java.net/~redestad/8237878/open.01/
> 
> Re-running some tests..
> 
> /Claes

Rémi

> 
>> 
>> I believe that at some point in the future, let say just before the release 
>> of
>> 17, we should do a global pass and rewrite each class to use 'var', the same
>> way we have done with generics.
>> It will be nasty for backporting bugs but i think it's better than having a 
>> mix
>> between codes that use var and codes that doesn't use it.
>> 
>> Rémi
>> 
>> - Mail original -
>>> De: "Claes Redestad" 
>>> À: "core-libs-dev" 
>>> Envoyé: Jeudi 6 Février 2020 13:37:59
>>> Objet: RFR: 8237878: Improve ModuleLoaderMap datastructures
>> 
>>> Hi,
>>>
>>> refactor ModuleLoaderMap to allow the datastructure to be
>>> archived, then archive it.
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8237878/open.00/
>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8237878
>>>
>>> Testing: tier1-3
>>>
> >> /Claes

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Alan Bateman]

On 06/02/2020 15:32, Seán Coffey wrote:
InitialContextFactory itself is an extremely simple interface with one 
method. I've browsed some code bases and could only find a small 
number of such Factories implementations with simple logic to return a 
Context. Applications will most likely delegate to the same Factory 
over and over also (albeit, it's all controlled by HashTable 
parameters). The new caching logic should help memory pressure here 
and not hinder it. I'm not seeing a major concern with current 
solution as a result.
Okay, although I could imagine a non-JDK InitialContextFactory 
implementation keeping a graph of objects alive. I saw your mail about 
separating the clarification in the APIs docs so does it mean the 
proposal on the table is the last webrev or is there a new version?


-Alan

Re: [14] RFR (XXS) 8238605: Correct the CLDR version number in cldr.md files

[Joe Wang]

+1

Best,
Joe


On 2/6/20 8:50 AM, naoto.s...@oracle.com wrote:

Hello,

Please review this extra small fix for this issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8238605/webrev.00/

It is simply updating the version number in cldr.md files, which 
should have been done with JDK-8231273.


Naoto

Re: [15] RFR (xs) 8046362 IdentityHashMap.hash comments should be clarified

[Martin Buchholz]

lgtm

Knuth didn't like linear probing, but memory locality became far more
important over time, favoring linear probing.

I don't get what hash() is doing with the low bits. Maybe we want something
like mix32.

On Thu, Feb 6, 2020 at 1:01 AM Andrew Haley  wrote:

> On 1/28/20 11:41 PM, Stuart Marks wrote:
>
> > As an aside, I don't know whether the linear-probing (open
> > addressed) arrangement of IdentityHashMap is in fact faster than the
> > separate chaining approach of HashMap. Perhaps investigating that
> > could be a side project for someone.
>
> It always has been better in the past, but maybe if the collision rate
> is kept low it won't matter much. Deletion is horrible, though.
>
> > Changeset appended below.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8046362
>
> OK, thanks.
>
> --
> Andrew Haley  (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. 
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>
>

[14] RFR (XXS) 8238605: Correct the CLDR version number in cldr.md files

[naoto . sato]

Hello,

Please review this extra small fix for this issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8238605/webrev.00/

It is simply updating the version number in cldr.md files, which should 
have been done with JDK-8231273.


Naoto

Re: RFR: 8237967: No proper error message when --runtime-image points to non-existent path

[Alexey Semenyuk]

Andy,

defaultSupported() is defined, but not called form the test. What is the 
point of this method?


- Alexey

On 2/6/2020 8:42 AM, Andy Herrick wrote:
revised fir at [3] to check for non-existent resource-dir as well as 
runtime-image dir.


[3] http://cr.openjdk.java.net/~herrick/8237967/webrev.02/

/Andy

On 2/5/2020 7:46 PM, Andy Herrick wrote:

OK - this makes sense - will update tomorrow.

/Andy

On 2/5/2020 4:24 PM, Alexander Matveev wrote:

Hi Andy,

Fix looks good, but I think we should do same for --resource-dir. I 
do not think that non-existing directory should be same as empty. 
User might misspell directory name and it will not be clear for user 
why we not using information from --resource-dir.


Thanks,
Alexander

On 2/5/2020 10:23 AM, Andy Herrick wrote:
This simple fix [1] to jpackage bug [2] simply adds a proper error 
message when --runtime-image points to a non-existent directory path.


[1] http://cr.openjdk.java.net/~herrick/8237967/webrev.01/index.html

[2] https://bugs.openjdk.java.net/browse/JDK-8237967

/Andy

Re: RFR: 8237967: No proper error message when --runtime-image points to non-existent path

[Andy Herrick]

revised fir at [3] to check for non-existent resource-dir as well as 
runtime-image dir.


[3] http://cr.openjdk.java.net/~herrick/8237967/webrev.02/

/Andy

On 2/5/2020 7:46 PM, Andy Herrick wrote:

OK - this makes sense - will update tomorrow.

/Andy

On 2/5/2020 4:24 PM, Alexander Matveev wrote:

Hi Andy,

Fix looks good, but I think we should do same for --resource-dir. I 
do not think that non-existing directory should be same as empty. 
User might misspell directory name and it will not be clear for user 
why we not using information from --resource-dir.


Thanks,
Alexander

On 2/5/2020 10:23 AM, Andy Herrick wrote:
This simple fix [1] to jpackage bug [2] simply adds a proper error 
message when --runtime-image points to a non-existent directory path.


[1] http://cr.openjdk.java.net/~herrick/8237967/webrev.01/index.html

[2] https://bugs.openjdk.java.net/browse/JDK-8237967

/Andy

RFR: 8238599: Refactor and simplify implAddOpensToAllUnnamed

[Claes Redestad]

Hi,

by refactoring implAddOpensToAllUnnamed to take the two sets that
comprise the packages we iterate over, we can make some simplifications,
pre-size HashMaps better etc, adding up to a nice little startup
improvement.

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

Testing: tier1+2

Thanks!

/Claes

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

On 05/02/20 16:51, Daniel Fuchs wrote:

On 05/02/2020 15:31, Peter Levart wrote:
I think this is about allow the InitialContextFactory instance to be 
GC'ed when the thread is long lived. It might not be a concern for 
the LDAP or the other providers in the JDK.


-Alan


Ah, I see. You mean when the ClassLoader is long lived. So this is a 
normal matter of trying to release the InitialContextFactory and 
objects held by it when there is a memory pressure and not the matter 
of avoiding class loader leaks? In this case it would pay only if 
InitialContextFactory holds more memory than SoftReference itself...


Regards, Peter


Hi Peter,

Yes the concern is that the InitialContextFactory would be kept around
until the ClassLoader itself is GC'ed, even if the factory was used once
and not needed anymore. Could that become an issue in case the factory 
is loaded e.g. by the System ClassLoader?
But it's a good point that it might not be a concern if the 
InitialContextFactory instance in itself doesn't retain much memory.
InitialContextFactory itself is an extremely simple interface with one 
method. I've browsed some code bases and could only find a small number 
of such Factories implementations with simple logic to return a Context. 
Applications will most likely delegate to the same Factory over and over 
also (albeit, it's all controlled by HashTable parameters). The new 
caching logic should help memory pressure here and not hinder it. I'm 
not seeing a major concern with current solution as a result.


regards,
Sean.



best regards,

-- daniel

Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10

[Roger Riggs]

+1,

One is enough, but additional eyes are good too.

On 2/5/20 10:35 PM, Patrick Zhang OS wrote:

Does this require one more “yes” from any other reviewer?

And may I ask for any committer’s help to sponsor and push it (once approved)? 
Thanks in advance.

Regards
Patrick

From: Thomas Stüfe 
Sent: Wednesday, February 5, 2020 11:16 PM
To: Patrick Zhang OS 
Cc: core-libs-dev 
Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple 
definition" link errors with GCC10

All still good.

...Thomas

On Wed, Feb 5, 2020 at 3:42 PM Patrick Zhang OS 
mailto:patr...@os.amperecomputing.com>> wrote:
Thanks for your comments, Thomas.

I have no accurate knowledge regarding why parentPathv was initially written as 
a global var instead of a member in ChildStuff struct initialized in 
jspawnhelper.c. However the suggested change might not be very straight-forward 
as there are other references such as: Java_java_lang_ProcessImpl_init(), 
spawnChild() in libjava\ProcessImpl_md.c, etc. And this goes beyond my original 
intention to fix the build errors purely, and try best to avoid any side 
effect. So I would leave the further improvement to others, and keep it as-is. 
Thanks.

Updated the change a bit (the header comments): 
http://cr.openjdk.java.net/~qpzhang/8238380/webrev.02/

Regards
Patrick

From: Thomas Stüfe mailto:thomas.stu...@gmail.com>>
Sent: Wednesday, February 5, 2020 2:30 PM
To: Patrick Zhang OS 
mailto:patr...@os.amperecomputing.com>>
Cc: core-libs-dev 
mailto:core-libs-dev@openjdk.java.net>>
Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple 
definition" link errors with GCC10

Hi Patrick,

You patch looks alright.

But I wonder whether a cleaner solution would not be to add a parentPathv 
pointer to the ChildStuff structure and pass it down to JDK_execvpe that way, 
like we do for all other input arguments of that function. If it is an input 
argument, seems strange to pass it as global variable. I leave that for others 
to decide though, your patch certainly works.

Cheers, Thomas

On Tue, Feb 4, 2020 at 3:39 PM Patrick Zhang OS 
mailto:patr...@os.amperecomputing.com>> wrote:
Hi

I split the original patch into parts for corresponding function review, here 
is the very simple change to java.base/unix/native/libjava/childproc.c and .h.
Could core-libs-dev group help review and sponsor this? thanks in advance.

JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of 
JDK-8235903)
Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/
Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no regression 
found. I don’t have any specific function tests, so this is just a smoke test.

Regards
Patrick

From: Martin Buchholz 
mailto:marti...@google.com>>>
Sent: Monday, December 16, 2019 10:44 AM
To: Patrick Zhang OS 
mailto:patr...@os.amperecomputing.com>>>;
 net-dev 
mailto:net-...@openjdk.java.net>>>; 
OpenJDK 
mailto:security-...@openjdk.java.net>>>
Cc: core-libs-dev 
mailto:core-libs-dev@openjdk.java.net>>>
Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple 
definition" link errors

forwarded to other teams for review.

On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS 
mailto:patr...@os.amperecomputing.com>>>
 wrote:
Hi

Please review this patch, if it should be reviewed by any group other than 
core-libs, please help forward it. Thanks.

JBS: https://bugs.openjdk.java.net/browse/JDK-8235903
Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/

A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple 
definition" link errors when building the jdk tip.

[PATCH] PR85678: Change default to -fno-common
https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html

For example, the error message looks like:
* For target support_native_java.base_libjava_BUILD_LIBJAVA_link:
build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple 
definition of `parentPathv'
build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first 
defined here
collect2: error: ld returned 1 exit status

This was not an issue because the original default -fcommon allowed "global variables defined 
without an initializer" be handled as COMMON symbols, so it would not warn the problem like 
"same variable is accidentally defined in more than one compilation unit".

About -fcommon vs -fno-cmmon:
https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-common

Moving forward, building jdk with latest versions of GCC will trigger this error. 
Specifying 

Re: RFR: 8237878: Improve ModuleLoaderMap datastructures

[Claes Redestad]

Hi Rémi,


On 2020-02-06 14:08, Remi Forax wrote:

Hi Claes,
In ArchivedModuleGraph, there is no point to take the mainModule as parameter 
given that it should always be null.


right, I've been thinking this should be simplified for now. Once we
implement support for archiving non-default scenarios the internal
datastructure will need a refactoring anyhow, so let's cut things down
to size.



In ModuleLoaderMap, the constants PLATFORM_LOADER_INDEX and APP_LOADER_INDEX 
should be moved inside the nested class Mapper given that there are only needed 
by the Mapper
and mappingFunction should be moved inside the class Mapper too.


Done.



in mappingFunction, i think that if you are using 'var', you should using 
everywhere in the method,
to avoid to have a weird mix between local variables declared with and without 
var.
so either map is not declared with var or resolvedModule and nm are declared 
with var.


I disagree var usage should be an all or nothing ordeal, rather it
should be used when the type information is readily available, e.g.,
by being stated in detail on the RHS.

The exact type in the other cases would be somewhat muddied: maybe
name() is "obviously" a String, but what type the elements in
cf.modules() are is not at all obvious, and making code less obvious
should be avoided.

New webrev:

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

Re-running some tests..

/Claes



I believe that at some point in the future, let say just before the release of 
17, we should do a global pass and rewrite each class to use 'var', the same 
way we have done with generics.
It will be nasty for backporting bugs but i think it's better than having a mix 
between codes that use var and codes that doesn't use it.

Rémi

- Mail original -

De: "Claes Redestad" 
À: "core-libs-dev" 
Envoyé: Jeudi 6 Février 2020 13:37:59
Objet: RFR: 8237878: Improve ModuleLoaderMap datastructures



Hi,

refactor ModuleLoaderMap to allow the datastructure to be
archived, then archive it.

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

Testing: tier1-3

/Claes

Re: RFR: 8237878: Improve ModuleLoaderMap datastructures

[Remi Forax]

Hi Claes,
In ArchivedModuleGraph, there is no point to take the mainModule as parameter 
given that it should always be null.

In ModuleLoaderMap, the constants PLATFORM_LOADER_INDEX and APP_LOADER_INDEX 
should be moved inside the nested class Mapper given that there are only needed 
by the Mapper
and mappingFunction should be moved inside the class Mapper too.

in mappingFunction, i think that if you are using 'var', you should using 
everywhere in the method,
to avoid to have a weird mix between local variables declared with and without 
var.
so either map is not declared with var or resolvedModule and nm are declared 
with var.

I believe that at some point in the future, let say just before the release of 
17, we should do a global pass and rewrite each class to use 'var', the same 
way we have done with generics.
It will be nasty for backporting bugs but i think it's better than having a mix 
between codes that use var and codes that doesn't use it.

Rémi

- Mail original -
> De: "Claes Redestad" 
> À: "core-libs-dev" 
> Envoyé: Jeudi 6 Février 2020 13:37:59
> Objet: RFR: 8237878: Improve ModuleLoaderMap datastructures

> Hi,
> 
> refactor ModuleLoaderMap to allow the datastructure to be
> archived, then archive it.
> 
> Webrev: http://cr.openjdk.java.net/~redestad/8237878/open.00/
> Bug:https://bugs.openjdk.java.net/browse/JDK-8237878
> 
> Testing: tier1-3
> 
> /Claes

Re: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary

[Magnus Ihse Bursie]

On 2020-02-05 16:32, Erik Joelsson wrote:

Hello,

New webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.02/

Looks good.

/Magnus


On 2020-02-05 02:17, Langer, Christoph wrote:

Hi,

we've tested the patch and all reported failure scenarios we're aware 
of are fixed with that, so basically, ship it 


As for the code review part:
The patch I've tested had removed the "-1" in line 532, so that seems 
to be correct. As Magnus wrote, the pattern seems to be copied from 
the lines above. So I think in line 518, subtracting -1 from 
"sizeOfLastPathComponent" seems to be incorrect as well. So it could 
be corrected in the same fix, I guess.
Yes, I did indeed just copy the pattern, but it also seems you are 
right about the -1, so I fixed it in both locations. I also figured 
reusing the variables wasn't very nice, so add the "Alt" prefix in all 
of them.
And there's one other minor thing: I tried to execute JliLaunchTest 
for the bundle scenario and had to make some adaptions to the example 
call to "make test-only ..." in line 66 of 
test/jdk/tools/launcher/JliLaunchTest.java. I guess the example could 
be more generic if it were changed to:


66 // $ make test-only 
TEST=test/jdk/tools/launcher/JliLaunchTest.java \
67 // 
JDK_IMAGE_DIR=$PWD/images/jdk-bundle/jdk-15.jdk/Contents/Home


(e.g. use relative paths inside the build directory)


Right, the name of the output dir may change. I didn't intend the 
example to be verbatim correct for everyone (hence the "something 
like" wording) but I see your point. Changed it.


/Erik


Thanks
Christoph


-Original Message-
From: Magnus Ihse Bursie 
Sent: Mittwoch, 5. Februar 2020 10:54
To: Erik Joelsson ; core-libs-dev ; build-dev ; Langer,
Christoph 
Subject: Re: RFR: JDK-8238225: Issues reported after replacing 
symlink at

Contents/MacOS/libjli.dylib with binary

On 2020-02-04 18:45, Erik Joelsson wrote:

Does anyone have an opinion on this?

The solution seems sound to me. I'm having a hard time figuring out if
the string manipulations in java_md_macosx.m are correct; as Christoph
is saying, they might not be. I realize you have copied an existing
pattern, and is likely subject to constraints, but that does not 
make it

easier to read.

/Magnus

/Erik

On 2020-01-31 07:31, Erik Joelsson wrote:

In JDK-8235687 the MacOS bundle distribution of the JDK was changed
to conform to Apple requirements by changing
Contents/MacOS/libjli.dylib from a symlink into
../Home/lib/libjli.dylib to a copy of that file. The problem with
having a symlink there is that Contents/MacOS/libjli.dylib is the
declared CFBundleExecutable of the bundle and that executable may not
be a symlink. The history around why that particular library was put
there seems lost in ancient history. All we know is that it was there
when Apple donated the Mac port and according to this bug report,
there are users out there relying on it.

When changing Contents/MacOS/libjli.dylib to a copy, loading that
dylib and using it to launch a JVM no longer works. This patch fixes
that by making libjli.dylib aware of potentially being located there
and if so, finding the JDK home dir in ../Home.

I've also expanded the existing test for launching a JVM through
libjli.dylib directly to also test this location when possible. In
local testing, this will not be covered unless the user explicitly
specifies that the JDK under test should be the bundle image on the
command line like this:

$ make test-only TEST=open/test/jdk/tools/launcher/JliLaunchTest.java
JDK_IMAGE_DIR=$PWD/build/macosx-x64/images/jdk-bundle/jdk-

15.jdk/Contents/Home

But, at least in Oracle's distributed testing, the JDK on MacOS is
distributed in the bundle layout, so there this functionality will be
tested.

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

Webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.01/

/Erik

RFR: 8237878: Improve ModuleLoaderMap datastructures

[Claes Redestad]

Hi,

refactor ModuleLoaderMap to allow the datastructure to be
archived, then archive it.

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

Testing: tier1-3

/Claes

RFR: 8237484: Improve module system bootstrap

[Claes Redestad]

Hi,

a few module bootstrap micro-optimizations.

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

Testing: tier1-3

/Claes

RE: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10

[Langer, Christoph]

As far as another reviewer is needed - looks good to me, too 

/Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Thomas Stüfe
> Sent: Donnerstag, 6. Februar 2020 07:49
> To: Patrick Zhang OS 
> Cc: core-libs-dev 
> Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c
> "multiple definition" link errors with GCC10
> 
> I can sponsor this for you.
> 
> .. Thomas
> 
> On Thu, Feb 6, 2020, 04:35 Patrick Zhang OS
> 
> wrote:
> 
> > Does this require one more “yes” from any other reviewer?
> >
> >
> >
> > And may I ask for any committer’s help to sponsor and push it (once
> > approved)? Thanks in advance.
> >
> >
> >
> > Regards
> >
> > Patrick
> >
> >
> >
> > *From:* Thomas Stüfe 
> > *Sent:* Wednesday, February 5, 2020 11:16 PM
> > *To:* Patrick Zhang OS 
> > *Cc:* core-libs-dev 
> > *Subject:* Re: RFR: JDK-8238380:
> > java.base/unix/native/libjava/childproc.c "multiple definition" link errors
> > with GCC10
> >
> >
> >
> > All still good.
> >
> >
> >
> > ...Thomas
> >
> >
> >
> > On Wed, Feb 5, 2020 at 3:42 PM Patrick Zhang OS <
> > patr...@os.amperecomputing.com> wrote:
> >
> > Thanks for your comments, Thomas.
> >
> >
> >
> > I have no accurate knowledge regarding why parentPathv was initially
> > written as a global var instead of a member in ChildStuff struct
> > initialized in jspawnhelper.c. However the suggested change might not be
> > very straight-forward as there are other references such as:
> > Java_java_lang_ProcessImpl_init(), spawnChild() in
> > libjava\ProcessImpl_md.c, etc. And this goes beyond my original intention
> > to fix the build errors purely, and try best to avoid any side effect. So I
> > would leave the further improvement to others, and keep it as-is. Thanks.
> >
> >
> >
> > Updated the change a bit (the header comments):
> > http://cr.openjdk.java.net/~qpzhang/8238380/webrev.02/
> >
> >
> >
> > Regards
> >
> > Patrick
> >
> >
> >
> > *From:* Thomas Stüfe 
> > *Sent:* Wednesday, February 5, 2020 2:30 PM
> > *To:* Patrick Zhang OS 
> > *Cc:* core-libs-dev 
> > *Subject:* Re: RFR: JDK-8238380:
> > java.base/unix/native/libjava/childproc.c "multiple definition" link errors
> > with GCC10
> >
> >
> >
> > Hi Patrick,
> >
> >
> >
> > You patch looks alright.
> >
> >
> >
> > But I wonder whether a cleaner solution would not be to add a
> parentPathv
> > pointer to the ChildStuff structure and pass it down to JDK_execvpe that
> > way, like we do for all other input arguments of that function. If it is an
> > input argument, seems strange to pass it as global variable. I leave that
> > for others to decide though, your patch certainly works.
> >
> >
> >
> > Cheers, Thomas
> >
> >
> >
> > On Tue, Feb 4, 2020 at 3:39 PM Patrick Zhang OS <
> > patr...@os.amperecomputing.com> wrote:
> >
> > Hi
> >
> > I split the original patch into parts for corresponding function review,
> > here is the very simple change to java.base/unix/native/libjava/childproc.c
> > and .h.
> > Could core-libs-dev group help review and sponsor this? thanks in advance.
> >
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of
> > JDK-8235903)
> > Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/
> > Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no
> > regression found. I don’t have any specific function tests, so this is just
> > a smoke test.
> >
> > Regards
> > Patrick
> >
> > From: Martin Buchholz
> mailto:marti...@google.com>>
> > Sent: Monday, December 16, 2019 10:44 AM
> > To: Patrick Zhang OS  > patr...@os.amperecomputing.com>>; net-dev  d...@openjdk.java.net
> > >; OpenJDK  d...@openjdk.java.net
> > >
> > Cc: core-libs-dev  > core-libs-dev@openjdk.java.net>>
> > Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes
> "multiple
> > definition" link errors
> >
> > forwarded to other teams for review.
> >
> > On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS <
> >
> patr...@os.amperecomputing.com m>>
> > wrote:
> > Hi
> >
> > Please review this patch, if it should be reviewed by any group other than
> > core-libs, please help forward it. Thanks.
> >
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8235903
> > Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/
> >
> > A recent GCC patch (supposed to be in GCC 10) exposes a couple of
> > "multiple definition" link errors when building the jdk tip.
> >
> > [PATCH] PR85678: Change default to -fno-common
> > https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html
> >
> > For example, the error message looks like:
> > * For target support_native_java.base_libjava_BUILD_LIBJAVA_link:
> > build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple
> > definition of `parentPathv'
> > build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first
> > defined here
> > collect2: error: ld returned 1 exit status
> >
> > This 

Re: [15] RFR (xs) 8046362 IdentityHashMap.hash comments should be clarified

[Andrew Haley]

On 1/28/20 11:41 PM, Stuart Marks wrote:

> As an aside, I don't know whether the linear-probing (open
> addressed) arrangement of IdentityHashMap is in fact faster than the
> separate chaining approach of HashMap. Perhaps investigating that
> could be a side project for someone.

It always has been better in the past, but maybe if the collision rate
is kept low it won't matter much. Deletion is horrible, though.

> Changeset appended below.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8046362

OK, thanks.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671