Re: RFR: JDK-8244634:, LoadLibraryW failed from tools/jpackage tests after JDK-8242302

[Andy Herrick]

OK - looks good.

/Andy

On 5/12/2020 11:42 AM, Alexey Semenyuk wrote:
Changed the patch to try AddDllDirectory() first and alter PATH as the 
last resort. Webrev at [1]


Log output:
---
$ env JPACKAGE_DEBUG=true ./JTwork/scratch/output/test/test.exe
[TRACE] applauncher.cpp:217: Entering AppLauncher::launch
[TRACE] applauncher.cpp:100: Launcher config file path: 
"C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\app\test.cfg"
[TRACE] winlauncher.cpp:70: Entering 
`anonymous-namespace'::loadDllWithAddDllDirectory
[TRACE] winlauncher.cpp:86: 
AddDllDirectory(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin): 
OK
[TRACE] winlauncher.cpp:94: 
LoadLibraryEx(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll, 
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS): 7FFBED20
[TRACE] winlauncher.cpp:0: Exiting 
`anonymous-namespace'::loadDllWithAddDllDirectory (entered at 
winlauncher.cpp:70)
[TRACE] jvmlauncher.cpp:177: JVM library: 
"C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll"

hello: Environment supports a display
jpackage test application
args.length: 0
-Dparam1=Some Param 1
-Dparam2=Some "Param" 2
-Dparam3=Some "Param" with " 3
hello: Output file: [appOutput.txt]
[TRACE] applauncher.cpp:0: Exiting AppLauncher::launch (entered at 
applauncher.cpp:217)

---

AddDllDirectory/LoadLibraryEx works. However not with 
LOAD_LIBRARY_SEARCH_USER_DIRS flag, but with wider 
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS.
Should we keep the code that would alter PATH in case 
AddDllDirectory() doesn't work?


[1] http://cr.openjdk.java.net/~asemenyuk/8244634/webrev.01

- Alexey

On 5/12/2020 10:33 AM, Alexey Semenyuk wrote:
> Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have all 
of these dll's already in the PATH (usually in C:\windows\system32) 
they can no longer be found ?

Yes.

> Is there a way you can link the launcher (e.g., something similar 
to RPATH on Unix systems) to look in the right place relative to the 
launcher?
The problem with failure to load jli.dll from the app launcher is 
that the system fails to load dependent dlls for jli.dll (MSVC 
run-time libs) from the directory with jli.dll even though the full 
path to jli.dll is specified in LoadLibrary() call. Launcher itself 
is statically linked with MSVC run-time libs. It it not an issue.


> Did you try to load jli.dll by specifying full path to jli.dll when 
calling LoadLibary?
Yes. Launcher loads jli.dll with the full path specified. The problem 
why LoadLibrary() fails to load it from the full path is that 
depending on OS/DLL loading settings LoadLibrary() would or would NOT 
look for dependent dlls in the directory where jli.dll is located.
The tricky part about AddDllDirectory() is that it would work for 
LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS only. Is this how 
Windows implicitly loads MSVC run-time libs before it completes 
explicit request to load jli.dll from the app launcher? I don't know.


PATH env variable will be altered only in case the first attempt to 
load jli.dll fails. Value of PATH can be restored after jli.dll is 
loaded. This piece of code can be added to the patch. I'll also give 
another try to AddDllDirectory() with LoadLibraryEx() call.


- Alexey

On 5/12/2020 8:31 AM, Kevin Rushforth wrote:
Is there a way you can link the launcher (e.g., something similar to 
RPATH on Unix systems) to look in the right place relative to the 
launcher? Otherwise, the solution with adding to the PATH seems OK 
to me.


-- Kevin


On 5/12/2020 5:22 AM, Andy Herrick wrote:
Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have 
all of these dll's already in the PATH (usually in 
C:\windows\system32) they can no longer be found ?


I don't like manually manipulating the PATH env variable either, 
but if so I don't see what else can be done short of putting the 
app exe in the bin dir of the runtime.


/Andy


On 5/11/2020 9:37 PM, Alexander Matveev wrote:

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try 
to load jli.dll by specifying full path to jli.dll when calling 
LoadLibary? If it does not work, then for AddDllDirectory() did 
you used LoadLibrary() or LoadLibraryEx() with 
LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use 
LoadLibraryEx() with flag when using AddDllDirectory().


Thanks,
Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

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

Fix failure to load jli.dll from app launcher. The fix is to 
append path to directory with jli.dll to PATH env variable and 
load jli.dll with altered value of PATH if the first attempt to 
load jli.dll without altering PATH fails.


- Alexey

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

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

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Alan Snyder]

You must have misunderstood. Even so, a snarky answer is uncalled for.

The specification of Object.equals() is violated by the example, but that 
outcome would be *required* by your version of the specification of 
Set.equals().

Do you really think that the specification of Object.equals() is unimportant? 
Lots of code relies on it.



> On May 12, 2020, at 2:37 PM, Stuart Marks  wrote:
> 
> 
> 
>>> The containsAll() and equals() methods both use the membership contract of 
>>> the receiver, not the argument. Unfortunately, the equals() specification 
>>> says,
>>> 
>>>Returns true if the specified object is also a set, the two sets have the
>>>same size, and every member of the specified set is contained in this set
>>>(or equivalently, every member of this set is contained in the specified
>>>set).
>>> 
>>> As should be clear from this discussion, the "equivalently" clause is 
>>> incorrect -- another spec bug.
>> Changing Set.equals() in this way would make Set inconsistent with Object.
>> Do you really think that is a good idea?
> 
>[example of asymmetry of equals]
> 
> Your example illustrates that the "equivalently" clause is incorrect. I 
> prefer specifications to have fewer incorrect statements.
> 
> s'marks
> 

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Alan Snyder]

But what kind of instance should be created by Set.copyOf?


> On May 12, 2020, at 2:30 PM, Stuart Marks  wrote:
> 
> 
> 
> On 5/9/20 7:48 PM, Alan Snyder wrote:
>> A small point… you might want to reconsider your analysis of Set.copyOf(), 
>> as it is a static method and there is no receiver.
> 
> Of course there is no receiver. For static factory methods, and for 
> constructors, I meant the newly created instance is the one whose membership 
> contract is used.
> 
> s'marks
> 

Re: RFR: JDK-8244634:, LoadLibraryW failed from tools/jpackage tests after JDK-8242302

[Alexander Matveev]

Hi Alexey,

Looks good. I do not see any issues with keeping code which alters PATH. 
I think it might be better to keep it, just in case.


Thanks,
Alexander

On 5/12/2020 8:42 AM, Alexey Semenyuk wrote:
Changed the patch to try AddDllDirectory() first and alter PATH as the 
last resort. Webrev at [1]


Log output:
---
$ env JPACKAGE_DEBUG=true ./JTwork/scratch/output/test/test.exe
[TRACE] applauncher.cpp:217: Entering AppLauncher::launch
[TRACE] applauncher.cpp:100: Launcher config file path: 
"C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\app\test.cfg"
[TRACE] winlauncher.cpp:70: Entering 
`anonymous-namespace'::loadDllWithAddDllDirectory
[TRACE] winlauncher.cpp:86: 
AddDllDirectory(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin): 
OK
[TRACE] winlauncher.cpp:94: 
LoadLibraryEx(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll, 
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS): 7FFBED20
[TRACE] winlauncher.cpp:0: Exiting 
`anonymous-namespace'::loadDllWithAddDllDirectory (entered at 
winlauncher.cpp:70)
[TRACE] jvmlauncher.cpp:177: JVM library: 
"C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll"

hello: Environment supports a display
jpackage test application
args.length: 0
-Dparam1=Some Param 1
-Dparam2=Some "Param" 2
-Dparam3=Some "Param" with " 3
hello: Output file: [appOutput.txt]
[TRACE] applauncher.cpp:0: Exiting AppLauncher::launch (entered at 
applauncher.cpp:217)

---

AddDllDirectory/LoadLibraryEx works. However not with 
LOAD_LIBRARY_SEARCH_USER_DIRS flag, but with wider 
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS.
Should we keep the code that would alter PATH in case 
AddDllDirectory() doesn't work?


[1] http://cr.openjdk.java.net/~asemenyuk/8244634/webrev.01

- Alexey

On 5/12/2020 10:33 AM, Alexey Semenyuk wrote:
> Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have all 
of these dll's already in the PATH (usually in C:\windows\system32) 
they can no longer be found ?

Yes.

> Is there a way you can link the launcher (e.g., something similar 
to RPATH on Unix systems) to look in the right place relative to the 
launcher?
The problem with failure to load jli.dll from the app launcher is 
that the system fails to load dependent dlls for jli.dll (MSVC 
run-time libs) from the directory with jli.dll even though the full 
path to jli.dll is specified in LoadLibrary() call. Launcher itself 
is statically linked with MSVC run-time libs. It it not an issue.


> Did you try to load jli.dll by specifying full path to jli.dll when 
calling LoadLibary?
Yes. Launcher loads jli.dll with the full path specified. The problem 
why LoadLibrary() fails to load it from the full path is that 
depending on OS/DLL loading settings LoadLibrary() would or would NOT 
look for dependent dlls in the directory where jli.dll is located.
The tricky part about AddDllDirectory() is that it would work for 
LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS only. Is this how 
Windows implicitly loads MSVC run-time libs before it completes 
explicit request to load jli.dll from the app launcher? I don't know.


PATH env variable will be altered only in case the first attempt to 
load jli.dll fails. Value of PATH can be restored after jli.dll is 
loaded. This piece of code can be added to the patch. I'll also give 
another try to AddDllDirectory() with LoadLibraryEx() call.


- Alexey

On 5/12/2020 8:31 AM, Kevin Rushforth wrote:
Is there a way you can link the launcher (e.g., something similar to 
RPATH on Unix systems) to look in the right place relative to the 
launcher? Otherwise, the solution with adding to the PATH seems OK 
to me.


-- Kevin


On 5/12/2020 5:22 AM, Andy Herrick wrote:
Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have 
all of these dll's already in the PATH (usually in 
C:\windows\system32) they can no longer be found ?


I don't like manually manipulating the PATH env variable either, 
but if so I don't see what else can be done short of putting the 
app exe in the bin dir of the runtime.


/Andy


On 5/11/2020 9:37 PM, Alexander Matveev wrote:

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try 
to load jli.dll by specifying full path to jli.dll when calling 
LoadLibary? If it does not work, then for AddDllDirectory() did 
you used LoadLibrary() or LoadLibraryEx() with 
LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use 
LoadLibraryEx() with flag when using AddDllDirectory().


Thanks,
Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

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

Fix failure to load jli.dll from app launcher. The fix is to 
append path to directory with jli.dll to PATH env variable and 
load jli.dll with altered value of PATH if the first attempt to 
load jli.dll without altering PATH fails.

Re: RFR: JDK-8244758,: DMG bundler ignores --install-dir option.

[Alexander Matveev]

Hi Andy,

Looks good.

Thanks,
Alexander

On 5/12/2020 6:56 AM, Andy Herrick wrote:

Please review jpackage fix for issue [1] at [2].

The change allows the specified --install-dir (instead of always 
/Applications) to be suggested for drag target of a DMG image.


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

[2] - http://cr.openjdk.java.net/~herrick/8244758

/Andy

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Stuart Marks]

The containsAll() and equals() methods both use the membership contract of the 
receiver, not the argument. Unfortunately, the equals() specification says,


   Returns true if the specified object is also a set, the two sets have the
   same size, and every member of the specified set is contained in this set
   (or equivalently, every member of this set is contained in the specified
   set).

As should be clear from this discussion, the "equivalently" clause is 
incorrect -- another spec bug.


Changing Set.equals() in this way would make Set inconsistent with Object.
Do you really think that is a good idea?


[example of asymmetry of equals]

Your example illustrates that the "equivalently" clause is incorrect. I prefer 
specifications to have fewer incorrect statements.


s'marks

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Stuart Marks]

On 5/9/20 7:48 PM, Alan Snyder wrote:

A small point… you might want to reconsider your analysis of Set.copyOf(), as 
it is a static method and there is no receiver.


Of course there is no receiver. For static factory methods, and for 
constructors, I meant the newly created instance is the one whose membership 
contract is used.


s'marks

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Stuart Marks]

You say:

The issue of membership semantics is part of the original design.

I agree, but only to the point of identifying inconsistent membership semantics 
as a source of non-conformance. What is not part of the original design is 
allowing a broader range of membership semantics to be conformant, which I 
assume is what you want.


I'm not using the concept of "conformance" so that's unrelated to what I want.


Another point of disagreement involves the specification of membership, 
especially in the case of Set, which I believe is where the problems arise.

I’m not completely sure what you are thinking, but it sounds like you believe 
that membership is defined by the implementation of the contains() method. I 
believe that is incorrect (i.e., not implied by the current specification).


No, I don't believe that membership is defined by the implementation of 
contains(). I'm not sure where that came from; it might be misapprehension or 
miscommunication.



Consider the “more formal” definition of contains():

More formally, returns true if and only if this set contains an element e 
such that Objects.equals(o, e).

Clearly, it would make no sense to define the contains() method in terms of 
itself. But it does make sense to say that the current state of a Set is a 
mathematical set of instances such that no pair of instances returns true from 
e1.equals(e2). I believe that is what the specification is trying to say, 
however imperfectly.


Yes, this much makes sense. Note that this embodies the membership contract of 
typical equals-based sets. Unfortunately, other sets (such as SortedSet) make 
some statements that imply that membership is determined by the comparator, they 
don't make any statments about how this affects contains(). The specification 
for SortedSet.contains(o) should say,


Returns true if and only if this set contains an element e such that
compare(o, e) == 0.

That it doesn't is yet another spec bug.


Consider the classic example, a TreeSet of Strings that is case-insensitive. If 
I add one string “hello” to an empty TreeSet, how many elements does it 
contain? The specification of add() says one. The size() method returns one. 
The iterator returns one element. But the contains() method returns true for 
multiple non-equal objects. Oops.


The reason this seems wrong is that you're calling contains() on two "non-equal" 
objects, but your statement's use of "non-equal" means you're using a different 
notion of equivalence from that used by the TreeSet. Since the TreeSet from your 
example uses case-insensitivity, those multiple "non-equal" objects on which 
you're calling contains() are in fact equivalent, so the result makes sense.



What I conclude from this exercise:


Does this mean you're close to conclusing this argument? :-)

I'll call out just one of your conclusions, since the other seem to be based on 
an incorrect assumption that I believe that the membership semantics are defined 
by the implementation of the contains() method, or that I'm proposing to do so. 
That conclusion is:



The problems of non-conforming collections are larger than you think. It is 
*not* the case that all of the basic axioms still apply.



Well I don't know how you know how large I think the problem is, but I'll agree 
it's pretty large. It's certainly much larger than the changeset that is 
currently under review.


Regarding the basic axioms, I'll return to some wording from SortedSet:

The behavior of a sorted set is well-defined even if its ordering is
inconsistent with equals; it just fails to obey the general contract
of the Set interface.

The "well-defined" clause means that the basic axioms apply.

However, I'll admit that the current wording of the spec does not support this! 
I'm assuming that there will be future changes to SortedSet et. al. that 
strengthen its notion of membership contract differing from other sets, and 
further that its add(), contains(), remove(), etc. methods all need to be 
adjusted to use this different membership contract.


Sorry if this wasn't clear. I know I've said this elsewhere, but possibly not on 
this thread or its predecessors.


s'marks

Re: RFR: 8244855 : Remove unused "getParent" function from Windows jni_util_md.c

[Brent Christian]

Ah, thanks.  I meant to check that, then it slipped my mind.

-Brent

On 5/12/20 12:17 PM, naoto.s...@oracle.com wrote:

Looks good, with the change to copyright year to "2020."

Naoto

On 5/12/20 12:12 PM, Lance @ Oracle wrote:

+1

Best,
Lance



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

On May 12, 2020, at 3:05 PM, Brent Christian 
 wrote:


Hi,

Please review this change to remove the unused "getParent()" function 
from jni_util_md.c on Windows.


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

Automated build+test job is in progress.
The diff is as follows:

diff -r ee4bd700b772 src/java.base/windows/native/libjava/jni_util_md.c
--- a/src/java.base/windows/native/libjava/jni_util_md.c    Tue May 
12 11:20:34 2020 -0700
+++ b/src/java.base/windows/native/libjava/jni_util_md.c    Tue May 
12 12:00:39 2020 -0700

@@ -31,17 +31,6 @@
#include "jni.h"
#include "jni_util.h"

-static void getParent(const TCHAR *path, TCHAR *dest) {
-    char* lastSlash = max(strrchr(path, '\\'), strrchr(path, '/'));
-    if (lastSlash == NULL) {
-    *dest = 0;
-    return;
-    }
-    if (path != dest)
-    strcpy(dest, path);
-    *lastSlash = 0;
-}
-
void* getProcessHandle() {
 return (void*)GetModuleHandle(NULL);
}

Thanks,
-Brent

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Stuart Marks]

On 5/8/20 6:46 PM, Jason Mehrens wrote:

I assume Deque/Queue would suffer the same issue as List.  It would be nice to 
some how ensure ArrayDeque.contains is not called as far as the heuristic goes. 
 Looks like PriorityQueue uses equals for contains but that is not to say there 
are not other queue implementations in the wild that do something different.


The difficulty here is how far to take the heuristic. The goal here is not to 
guarantee that behavior can never be O(M*N) but to catch what seem to be the 
most common cases. I think the most common case is where the argument is a List, 
but there are lots of possibilities that we'd inevitably miss.



The more I think about it in general, it seems like your task would be easier 
if you could declare:
1. AbstractCollection.removeAll should assume this.contains is O(N) and iterate 
over this and check contains on arg.
2. AbstractSet.removeAll should iterate over argument and assume that 
this.contains/remove is at least O(log (N)) and assume argument.contains is 
O(N).
3. Concrete implementations of removeAll know the cost of their contains 
method.  If it is known to be O(N) then walk this otherwise walk the arg.
4. Include an example in removeAll that says if membership semantics differ between 
sets use: source.removeIf(e -> removals.contains(e)); or removals.forEach(e -> 
source.remove(e)); instead.  If they don't differ then use removeAll.


This sort of makes sense from a performance standpoint, but it doesn't address 
the semantic issues I raised in my reply the other day to Jens Lideström. 
Specifically, we don't want Set.removeAll or AbstractSet.removeAll to disagree 
with Collection.removeAll and AbstractCollection.removeAll. We also want 
removeAll() to be the complement of retainAll().



Given this has been around since JDK 1.3 would it be safe to assume that code 
that is mixing collections with different membership semantics is already 
working around this issue and therefore the risk is a bit lower in changing the 
spec?  Lots of concrete implementations already override removeAll anyway.


I'm not sure that's true. People keep running into either the performance 
problem or the semantic problem. Sometimes people live with bugs for a long time 
and can't figure it out. "Yeah, removeAll sometimes gives this weird result. I 
don't know why. Maybe I just don't understand Java."


s'marks

Re: RFR: 8244855 : Remove unused "getParent" function from Windows jni_util_md.c

[naoto . sato]

Looks good, with the change to copyright year to "2020."

Naoto

On 5/12/20 12:12 PM, Lance @ Oracle wrote:

+1

Best,
Lance



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


On May 12, 2020, at 3:05 PM, Brent Christian  wrote:

Hi,

Please review this change to remove the unused "getParent()" function from 
jni_util_md.c on Windows.

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

Automated build+test job is in progress.
The diff is as follows:

diff -r ee4bd700b772 src/java.base/windows/native/libjava/jni_util_md.c
--- a/src/java.base/windows/native/libjava/jni_util_md.cTue May 12 11:20:34 
2020 -0700
+++ b/src/java.base/windows/native/libjava/jni_util_md.cTue May 12 12:00:39 
2020 -0700
@@ -31,17 +31,6 @@
#include "jni.h"
#include "jni_util.h"

-static void getParent(const TCHAR *path, TCHAR *dest) {
-char* lastSlash = max(strrchr(path, '\\'), strrchr(path, '/'));
-if (lastSlash == NULL) {
-*dest = 0;
-return;
-}
-if (path != dest)
-strcpy(dest, path);
-*lastSlash = 0;
-}
-
void* getProcessHandle() {
 return (void*)GetModuleHandle(NULL);
}

Thanks,
-Brent

Re: RFR: 8244855 : Remove unused "getParent" function from Windows jni_util_md.c

[Lance @ Oracle]

+1

Best,
Lance



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

> On May 12, 2020, at 3:05 PM, Brent Christian  
> wrote:
> 
> Hi,
> 
> Please review this change to remove the unused "getParent()" function from 
> jni_util_md.c on Windows.
> 
> https://bugs.openjdk.java.net/browse/JDK-8244855
> 
> Automated build+test job is in progress.
> The diff is as follows:
> 
> diff -r ee4bd700b772 src/java.base/windows/native/libjava/jni_util_md.c
> --- a/src/java.base/windows/native/libjava/jni_util_md.cTue May 12 
> 11:20:34 2020 -0700
> +++ b/src/java.base/windows/native/libjava/jni_util_md.cTue May 12 
> 12:00:39 2020 -0700
> @@ -31,17 +31,6 @@
> #include "jni.h"
> #include "jni_util.h"
> 
> -static void getParent(const TCHAR *path, TCHAR *dest) {
> -char* lastSlash = max(strrchr(path, '\\'), strrchr(path, '/'));
> -if (lastSlash == NULL) {
> -*dest = 0;
> -return;
> -}
> -if (path != dest)
> -strcpy(dest, path);
> -*lastSlash = 0;
> -}
> -
> void* getProcessHandle() {
> return (void*)GetModuleHandle(NULL);
> }
> 
> Thanks,
> -Brent

RFR: 8244855 : Remove unused "getParent" function from Windows jni_util_md.c

[Brent Christian]

Hi,

Please review this change to remove the unused "getParent()" function 
from jni_util_md.c on Windows.


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

Automated build+test job is in progress.
The diff is as follows:

diff -r ee4bd700b772 src/java.base/windows/native/libjava/jni_util_md.c
--- a/src/java.base/windows/native/libjava/jni_util_md.c	Tue May 12 
11:20:34 2020 -0700
+++ b/src/java.base/windows/native/libjava/jni_util_md.c	Tue May 12 
12:00:39 2020 -0700

@@ -31,17 +31,6 @@
 #include "jni.h"
 #include "jni_util.h"

-static void getParent(const TCHAR *path, TCHAR *dest) {
-char* lastSlash = max(strrchr(path, '\\'), strrchr(path, '/'));
-if (lastSlash == NULL) {
-*dest = 0;
-return;
-}
-if (path != dest)
-strcpy(dest, path);
-*lastSlash = 0;
-}
-
 void* getProcessHandle() {
 return (void*)GetModuleHandle(NULL);
 }

Thanks,
-Brent

Re: RFR: 8244853 - The static build of libextnet is missing the JNI_OnLoad_extnet function

[Alan Bateman]

Looks okay to me.

On 12/05/2020 19:46, Bob Vandette wrote:

BUG:

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

Please review this simple fix for JDK 15 which adds the required 
JNI_OnLoad_extnet function to the libextnet.a
static library when it is built.

the JDK 15 make static-libs-image target currently builds this static library 
but it is not spec compliant and
causes the GraalVM native-image utility to fail generating executables due to 
the lack of the OnLoad symbol.


CHANGE:

diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c 
b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
--- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
+++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
@@ -34,6 +34,11 @@
  #include "jdk_net_LinuxSocketOptions.h"
  
  /*

+ * Declare library specific JNI_Onload entry if static build
+ */
+DEF_STATIC_JNI_OnLoad
+
+/*
   * Class: jdk_net_LinuxSocketOptions
   * Method:setQuickAck
   * Signature: (II)V
diff --git a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c 
b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
--- a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
+++ b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
@@ -32,6 +32,11 @@
  #include 
  #include "jni_util.h"
  
+/*

+ * Declare library specific JNI_Onload entry if static build
+ */
+DEF_STATIC_JNI_OnLoad
+
  static jint socketOptionSupported(jint sockopt) {
  jint one = 1;
  jint rv, s;
diff --git a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c 
b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
--- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
+++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
@@ -32,6 +32,11 @@
  static int initialized = 0;
  
  /*

+ * Declare library specific JNI_Onload entry if static build
+ */
+DEF_STATIC_JNI_OnLoad
+
+/*
   * Class: jdk_net_SolarisSocketOptions
   * Method:init
   * Signature: ()V

RFR: 8244853 - The static build of libextnet is missing the JNI_OnLoad_extnet function

[Bob Vandette]

BUG:

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

Please review this simple fix for JDK 15 which adds the required 
JNI_OnLoad_extnet function to the libextnet.a
static library when it is built.

the JDK 15 make static-libs-image target currently builds this static library 
but it is not spec compliant and
causes the GraalVM native-image utility to fail generating executables due to 
the lack of the OnLoad symbol.


CHANGE:

diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c 
b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
--- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
+++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
@@ -34,6 +34,11 @@
 #include "jdk_net_LinuxSocketOptions.h"
 
 /*
+ * Declare library specific JNI_Onload entry if static build
+ */
+DEF_STATIC_JNI_OnLoad
+
+/*
  * Class: jdk_net_LinuxSocketOptions
  * Method:setQuickAck
  * Signature: (II)V
diff --git a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c 
b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
--- a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
+++ b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
@@ -32,6 +32,11 @@
 #include 
 #include "jni_util.h"
 
+/*
+ * Declare library specific JNI_Onload entry if static build
+ */
+DEF_STATIC_JNI_OnLoad
+
 static jint socketOptionSupported(jint sockopt) {
 jint one = 1;
 jint rv, s;
diff --git a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c 
b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
--- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
+++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
@@ -32,6 +32,11 @@
 static int initialized = 0;
 
 /*
+ * Declare library specific JNI_Onload entry if static build
+ */
+DEF_STATIC_JNI_OnLoad
+
+/*
  * Class: jdk_net_SolarisSocketOptions
  * Method:init
  * Signature: ()V

Re: RFR: 8244767 - Potential non-terminated string in getEncodingInternal() on Windows

[Brent Christian]

Thanks - change is pushed.  -B

On 5/11/20 5:26 PM, naoto.s...@oracle.com wrote:

+1

Naoto

On 5/11/20 5:25 PM, Brian Burkhalter wrote:

Hi Brent,

It looks OK to me.

Brian

On May 11, 2020, at 4:36 PM, Brent Christian 
 wrote:


Please review this small fix in Windows native code:

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

Webrev: http://cr.openjdk.java.net/~bchristi/8244767/webrev-00/ 



As reported on this thread[1], the getEncodingInternal() function has 
a potential unterminated string in the case that the GetLocaleInfo() 
Windows function fails.  In this case, the default switch() case will 
write "Cp" to the beginning of the 'ret' buffer, but the rest of the 
buffer remains uninitialized and unterminated.


The fix is to strcpy() the default codepage into 'ret'.

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Jason Mehrens]

HashSet/TreeSet could do what ConcurrentHashMap/ConcurrentSkipListSet do by 
using the "this contains that and that contains this" logic.

  Comparator cc = String.CASE_INSENSITIVE_ORDER;
Set s1 = new ConcurrentHashMap().keySet("");
Set s2 = new ConcurrentSkipListSet<>(cc);
s1.add("hello");
s2.add("Hello");
System.out.println(s1.equals(s2));
System.out.println(s2.equals(s1));
System.out.println(s1.hashCode() == s2.hashCode());
===
false
false
false


From: core-libs-dev  on behalf of Alan 
Snyder 
Sent: Tuesday, May 12, 2020 11:27 AM
To: Stuart Marks
Cc: core-libs-dev
Subject: Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes



> On May 8, 2020, at 1:49 PM, Stuart Marks  wrote:
>
> The containsAll() and equals() methods both use the membership contract of 
> the receiver, not the argument. Unfortunately, the equals() specification 
> says,
>
>Returns true if the specified object is also a set, the two sets have the
>same size, and every member of the specified set is contained in this set
>(or equivalently, every member of this set is contained in the specified
>set).
>
> As should be clear from this discussion, the "equivalently" clause is 
> incorrect -- another spec bug.

Changing Set.equals() in this way would make Set inconsistent with Object.
Do you really think that is a good idea?




Comparator cc = (a, b) -> a.compareToIgnoreCase(b);

Set s1 = new HashSet<>();

Set s2 = new TreeSet<>(cc);

s1.add("hello");

s2.add("Hello");

s1.equals(s2) -> false

s2.equals(s1) -> true

s1.hashCode() == s2.hashCode() -> false

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Alan Snyder]

> On May 8, 2020, at 1:49 PM, Stuart Marks  wrote:
> 
> The containsAll() and equals() methods both use the membership contract of 
> the receiver, not the argument. Unfortunately, the equals() specification 
> says,
> 
>Returns true if the specified object is also a set, the two sets have the
>same size, and every member of the specified set is contained in this set
>(or equivalently, every member of this set is contained in the specified
>set).
> 
> As should be clear from this discussion, the "equivalently" clause is 
> incorrect -- another spec bug.

Changing Set.equals() in this way would make Set inconsistent with Object.
Do you really think that is a good idea?




Comparator cc = (a, b) -> a.compareToIgnoreCase(b);

Set s1 = new HashSet<>();

Set s2 = new TreeSet<>(cc);

s1.add("hello");

s2.add("Hello");

s1.equals(s2) -> false

s2.equals(s1) -> true

s1.hashCode() == s2.hashCode() -> false

Re: RFR: JDK-8244634:, LoadLibraryW failed from tools/jpackage tests after JDK-8242302

[Alexey Semenyuk]

Changed the patch to try AddDllDirectory() first and alter PATH as the 
last resort. Webrev at [1]


Log output:
---
$ env JPACKAGE_DEBUG=true ./JTwork/scratch/output/test/test.exe
[TRACE] applauncher.cpp:217: Entering AppLauncher::launch
[TRACE] applauncher.cpp:100: Launcher config file path: 
"C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\app\test.cfg"
[TRACE] winlauncher.cpp:70: Entering 
`anonymous-namespace'::loadDllWithAddDllDirectory
[TRACE] winlauncher.cpp:86: 
AddDllDirectory(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin): 
OK
[TRACE] winlauncher.cpp:94: 
LoadLibraryEx(C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll, 
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS): 7FFBED20
[TRACE] winlauncher.cpp:0: Exiting 
`anonymous-namespace'::loadDllWithAddDllDirectory (entered at 
winlauncher.cpp:70)
[TRACE] jvmlauncher.cpp:177: JVM library: 
"C:\Users\test\8244634\jdk-15_b22\JTwork\scratch\output\test\runtime\bin\jli.dll"

hello: Environment supports a display
jpackage test application
args.length: 0
-Dparam1=Some Param 1
-Dparam2=Some "Param" 2
-Dparam3=Some "Param" with " 3
hello: Output file: [appOutput.txt]
[TRACE] applauncher.cpp:0: Exiting AppLauncher::launch (entered at 
applauncher.cpp:217)

---

AddDllDirectory/LoadLibraryEx works. However not with 
LOAD_LIBRARY_SEARCH_USER_DIRS flag, but with wider 
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS.
Should we keep the code that would alter PATH in case AddDllDirectory() 
doesn't work?


[1] http://cr.openjdk.java.net/~asemenyuk/8244634/webrev.01

- Alexey

On 5/12/2020 10:33 AM, Alexey Semenyuk wrote:
> Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have all 
of these dll's already in the PATH (usually in C:\windows\system32) 
they can no longer be found ?

Yes.

> Is there a way you can link the launcher (e.g., something similar to 
RPATH on Unix systems) to look in the right place relative to the 
launcher?
The problem with failure to load jli.dll from the app launcher is that 
the system fails to load dependent dlls for jli.dll (MSVC run-time 
libs) from the directory with jli.dll even though the full path to 
jli.dll is specified in LoadLibrary() call. Launcher itself is 
statically linked with MSVC run-time libs. It it not an issue.


> Did you try to load jli.dll by specifying full path to jli.dll when 
calling LoadLibary?
Yes. Launcher loads jli.dll with the full path specified. The problem 
why LoadLibrary() fails to load it from the full path is that 
depending on OS/DLL loading settings LoadLibrary() would or would NOT 
look for dependent dlls in the directory where jli.dll is located.
The tricky part about AddDllDirectory() is that it would work for 
LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS only. Is this how 
Windows implicitly loads MSVC run-time libs before it completes 
explicit request to load jli.dll from the app launcher? I don't know.


PATH env variable will be altered only in case the first attempt to 
load jli.dll fails. Value of PATH can be restored after jli.dll is 
loaded. This piece of code can be added to the patch. I'll also give 
another try to AddDllDirectory() with LoadLibraryEx() call.


- Alexey

On 5/12/2020 8:31 AM, Kevin Rushforth wrote:
Is there a way you can link the launcher (e.g., something similar to 
RPATH on Unix systems) to look in the right place relative to the 
launcher? Otherwise, the solution with adding to the PATH seems OK to 
me.


-- Kevin


On 5/12/2020 5:22 AM, Andy Herrick wrote:
Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have 
all of these dll's already in the PATH (usually in 
C:\windows\system32) they can no longer be found ?


I don't like manually manipulating the PATH env variable either, but 
if so I don't see what else can be done short of putting the app exe 
in the bin dir of the runtime.


/Andy


On 5/11/2020 9:37 PM, Alexander Matveev wrote:

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try 
to load jli.dll by specifying full path to jli.dll when calling 
LoadLibary? If it does not work, then for AddDllDirectory() did you 
used LoadLibrary() or LoadLibraryEx() with 
LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use 
LoadLibraryEx() with flag when using AddDllDirectory().


Thanks,
Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

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

Fix failure to load jli.dll from app launcher. The fix is to 
append path to directory with jli.dll to PATH env variable and 
load jli.dll with altered value of PATH if the first attempt to 
load jli.dll without altering PATH fails.


- Alexey

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

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

Re: RFR: JDK-8244758,: DMG bundler ignores --install-dir option.

[Alexey Semenyuk]

Looks good.

- Alexey

On 5/12/2020 9:56 AM, Andy Herrick wrote:

Please review jpackage fix for issue [1] at [2].

The change allows the specified --install-dir (instead of always 
/Applications) to be suggested for drag target of a DMG image.


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

[2] - http://cr.openjdk.java.net/~herrick/8244758

/Andy

Re: RFR(s): 8244659: Improve ZipFile.getInputStream

[Claes Redestad]

On 2020-05-12 16:07, Martin Buchholz wrote:



On Tue, May 12, 2020 at 6:42 AM Claes Redestad 
mailto:claes.redes...@oracle.com>> wrote:


Hi Volker,

I think this look like a nice improvement!

One high-level concern I have with the design is that this will add and
retain (at least) one 64k buffer to each Jar-/ZipFile we've read a
stream from. We routinely see apps reading classes from 100s of jar
files on their class path, so this could add noticeable overhead to the
baseline retained memory usage of such applications.


At Google, it's common to run java applications with 10,000 small jar 
files on the classpath.


Ouch! That could mean a 600Mb+ footprint increase. Not very nice.

Making the "quiescent cost" of each ZipFile object as small as possible 
is important.
OTOH we currently retain the byte[] of the zip file central directory 
indefinitely.


Striking the right trade-off is hard - and footprint has often gotten
the shortest stick.




Have you considered other strategies such as making the cache global?
Since a (the?) common usage pattern is likely a single thread repeatedly
reading resources from a series of jar files, contention on such a
global cache is likely going to be very low, while likely reducing the
total number of buffers we have to allocate and retain to single-digit
numbers. I don't insist on a re-design, but it shouldn't be hard to
prototype and run some numbers on it.


Java resource management is still hard!


... but fun?!

/Claes

Re: RFR: JDK-8244634:, LoadLibraryW failed from tools/jpackage tests after JDK-8242302

[Alexey Semenyuk]

> Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have all of 
these dll's already in the PATH (usually in C:\windows\system32) they 
can no longer be found ?

Yes.

> Is there a way you can link the launcher (e.g., something similar to 
RPATH on Unix systems) to look in the right place relative to the launcher?
The problem with failure to load jli.dll from the app launcher is that 
the system fails to load dependent dlls for jli.dll (MSVC run-time libs) 
from the directory with jli.dll even though the full path to jli.dll is 
specified in LoadLibrary() call. Launcher itself is statically linked 
with MSVC run-time libs. It it not an issue.


> Did you try to load jli.dll by specifying full path to jli.dll when 
calling LoadLibary?
Yes. Launcher loads jli.dll with the full path specified. The problem 
why LoadLibrary() fails to load it from the full path is that depending 
on OS/DLL loading settings LoadLibrary() would or would NOT look for 
dependent dlls in the directory where jli.dll is located.
The tricky part about AddDllDirectory() is that it would work for 
LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS only. Is this how 
Windows implicitly loads MSVC run-time libs before it completes explicit 
request to load jli.dll from the app launcher? I don't know.


PATH env variable will be altered only in case the first attempt to load 
jli.dll fails. Value of PATH can be restored after jli.dll is loaded. 
This piece of code can be added to the patch. I'll also give another try 
to AddDllDirectory() with LoadLibraryEx() call.


- Alexey

On 5/12/2020 8:31 AM, Kevin Rushforth wrote:
Is there a way you can link the launcher (e.g., something similar to 
RPATH on Unix systems) to look in the right place relative to the 
launcher? Otherwise, the solution with adding to the PATH seems OK to me.


-- Kevin


On 5/12/2020 5:22 AM, Andy Herrick wrote:
Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have all 
of these dll's already in the PATH (usually in C:\windows\system32) 
they can no longer be found ?


I don't like manually manipulating the PATH env variable either, but 
if so I don't see what else can be done short of putting the app exe 
in the bin dir of the runtime.


/Andy


On 5/11/2020 9:37 PM, Alexander Matveev wrote:

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try to 
load jli.dll by specifying full path to jli.dll when calling 
LoadLibary? If it does not work, then for AddDllDirectory() did you 
used LoadLibrary() or LoadLibraryEx() with 
LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use 
LoadLibraryEx() with flag when using AddDllDirectory().


Thanks,
Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

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

Fix failure to load jli.dll from app launcher. The fix is to append 
path to directory with jli.dll to PATH env variable and load 
jli.dll with altered value of PATH if the first attempt to load 
jli.dll without altering PATH fails.


- Alexey

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

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

Re: [8u] RFR(S): 8244548: JDK 8u: sun.misc.Version.jdkUpdateVersion() returns wrong result

[Severin Gehwolf]

Hi Andrew,

Thanks for looking at this.

On Tue, 2020-05-12 at 14:07 +0100, Andrew Haley wrote:
> On 5/7/20 4:09 PM, Severin Gehwolf wrote:
> > jvm_version_info.jvm_version currently holds this quadruplet:
> > 
> > Most significant 8 bits => major version, followed by 8 bits => minor
> > version, followed by 8 bits => micro version, followed by 8 bits =>
> > build version. Note that JVM minor version represents the update
> > version as passed in via configure and the micro version is currently
> > not used (always 0). See vm_version.cpp lines 100-102 where only major,
> > minor and build number are ever been set. Knowing this, we can still
> > preserve the same behavior after patch by defining JVM_VERSION_MICRO to
> > 0 for any version.
> 
> This is tricky. JVM_GetVersionInfo is a function exported by
> libjvm.so, and the version is simply encoded as
> 
> unsigned int Abstract_VM_Version::jvm_version() {
>   return ((Abstract_VM_Version::vm_major_version() & 0xFF) << 24) |
>  ((Abstract_VM_Version::vm_minor_version() & 0xFF) << 16) |
>  (Abstract_VM_Version::vm_build_number() & 0xFF);
> }
> 
> I guess we could argue that this is for JVM-JDK internal use only, and
> no-one else cares.
> 
> Or we could encode it in a different way such that at least we have a
> jvm_version that is monotonically increasing, perhaps by (ab)using the
> lower 8 bits of the version, the vm_build_number. But I guess I'm
> being paranoid, and no tools are going to care about minor versions
> anyway,even if they do call JVM_GetVersionInfo.

Yes, this is indeed tricky. The trouble is that
Abstract_VM_Version::vm_build_number() actually holds the configured
JDK build number, we can't really use it. It's already being used.

(gdb)
99  
100   _vm_major_version = atoi(vm_major_ver);
101   _vm_minor_version = atoi(vm_minor_ver);
102   _vm_build_number  = atoi(vm_build_num);
103 
104   os::free(vm_version);
105   _initialized = true;
106 }
107 
108 #if defined(_LP64)
(gdb) p _vm_build_number 
$1 = 2

The only bits which seem unused are bits 8 through 16 of this unsigned
int. So even if JVM_GetVersionInfo *is* being used somewhere in the
wild, it *should* continue to work. Hence, the proposal to now also use
those unused bits for the minor version.

Thoughts?

Thanks,
Severin

Re: RFR: JDK-8244634:, LoadLibraryW failed from tools/jpackage tests after JDK-8242302

[Scott Palmer]

I think that sorry if things is usually handled in an “Application Manifest” on 
Windows

https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests

Scott

> On May 12, 2020, at 8:33 AM, Kevin Rushforth  
> wrote:
> 
> Is there a way you can link the launcher (e.g., something similar to RPATH 
> on Unix systems) to look in the right place relative to the launcher? 
> Otherwise, the solution with adding to the PATH seems OK to me.
> 
> -- Kevin
> 
> 
>> On 5/12/2020 5:22 AM, Andy Herrick wrote:
>> Is the problem that by removing the copy of the microsoft dlls from the 
>> applications bin directory, then on machines that do not have all of these 
>> dll's already in the PATH (usually in C:\windows\system32) they can no 
>> longer be found ?
>> 
>> I don't like manually manipulating the PATH env variable either, but if so I 
>> don't see what else can be done short of putting the app exe in the bin dir 
>> of the runtime.
>> 
>> /Andy
>> 
>> 
>>> On 5/11/2020 9:37 PM, Alexander Matveev wrote:
>>> Hi Alexey,
>>> 
>>> Updating PATH does not look like good solution to me. Did you try to load 
>>> jli.dll by specifying full path to jli.dll when calling LoadLibary? If it 
>>> does not work, then for AddDllDirectory() did you used LoadLibrary() or 
>>> LoadLibraryEx() with LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you 
>>> need to use LoadLibraryEx() with flag when using AddDllDirectory().
>>> 
>>> Thanks,
>>> Alexander
>>> 
>>> On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:
 Please review fix [2] for jpackage bug [1].
 
 Fix failure to load jli.dll from app launcher. The fix is to append path 
 to directory with jli.dll to PATH env variable and load jli.dll with 
 altered value of PATH if the first attempt to load jli.dll without 
 altering PATH fails.
 
 - Alexey
 
 [1] https://bugs.openjdk.java.net/browse/JDK-8244634
 
 [2] http://cr.openjdk.java.net/~asemenyuk/8244634/webrev.00
 
>>> 
> 

Re: RFR(s): 8244659: Improve ZipFile.getInputStream

[Martin Buchholz]

On Tue, May 12, 2020 at 6:42 AM Claes Redestad 
wrote:

> Hi Volker,
>
> I think this look like a nice improvement!
>
> One high-level concern I have with the design is that this will add and
> retain (at least) one 64k buffer to each Jar-/ZipFile we've read a
> stream from. We routinely see apps reading classes from 100s of jar
> files on their class path, so this could add noticeable overhead to the
> baseline retained memory usage of such applications.
>

At Google, it's common to run java applications with 10,000 small jar files
on the classpath.
Making the "quiescent cost" of each ZipFile object as small as possible is
important.
OTOH we currently retain the byte[] of the zip file central directory
indefinitely.



>
> Have you considered other strategies such as making the cache global?
> Since a (the?) common usage pattern is likely a single thread repeatedly
> reading resources from a series of jar files, contention on such a
> global cache is likely going to be very low, while likely reducing the
> total number of buffers we have to allocate and retain to single-digit
> numbers. I don't insist on a re-design, but it shouldn't be hard to
> prototype and run some numbers on it.
>

Java resource management is still hard!

RFR: JDK-8244758,: DMG bundler ignores --install-dir option.

[Andy Herrick]

Please review jpackage fix for issue [1] at [2].

The change allows the specified --install-dir (instead of always 
/Applications) to be suggested for drag target of a DMG image.


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

[2] - http://cr.openjdk.java.net/~herrick/8244758

/Andy

Re: RFR(s): 8244659: Improve ZipFile.getInputStream

[Claes Redestad]

Hi Volker,

I think this look like a nice improvement!

One high-level concern I have with the design is that this will add and
retain (at least) one 64k buffer to each Jar-/ZipFile we've read a
stream from. We routinely see apps reading classes from 100s of jar
files on their class path, so this could add noticeable overhead to the
baseline retained memory usage of such applications.

Have you considered other strategies such as making the cache global?
Since a (the?) common usage pattern is likely a single thread repeatedly
reading resources from a series of jar files, contention on such a
global cache is likely going to be very low, while likely reducing the
total number of buffers we have to allocate and retain to single-digit
numbers. I don't insist on a re-design, but it shouldn't be hard to
prototype and run some numbers on it.

Minor random comments:

Since you're not assigning null to bufferCache anywhere, this field
could be final and the null-check in releaseBuffer removed.

Pre-existing, but I wonder if there's a good reason to assign null to
the inflaterCache in the run() method. Seems like trying to do the GCs
job.. It could probably be removed, the field made final and the null
check removed in the same way.

On 2020-05-08 17:36, Volker Simonis wrote:

  Hi,

can I please have a review for the following small enhancement which
improves the speed of reading from ZipFile.getInputStream() by ~5%:

http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659/
https://bugs.openjdk.java.net/browse/JDK-8244659

ZipFile.getInputStream() tries to find a good size for sizing the internal
buffer of the underlying InflaterInputStream. This buffer is used to read
the compressed data from the associated InputStream. Unfortunately,
ZipFile.getInputStream() uses CENLEN (i.e. the uncompressed size of a
ZipEntry) instead of CENSIZ (i.e. the compressed size of a ZipEntry) to
configure the input buffer and thus unnecessarily wastes memory, because
the corresponding, compressed input data is at most CENSIZ bytes long.

After fixing this and doing some benchmarks, I realized that a much bigger
problem is the continuous allocation of new, temporary input buffers for
each new input stream. Assuming that a zip files usually has hundreds if
not thousands of ZipEntries, I think it makes sense to cache these input
buffers. Fortunately, ZipFile already has a built-in mechanism for such
caching which it uses for caching the Inflaters needed for each new input
stream. In order to cache the buffers as well, I had to add a new ,
package-private constructor to InflaterInputStream. I'm not sure if it
makes sense to make this new constructor public, to enable other users of
InflaterInputStream to pre-allocate the buffer. If you think so, I'd be
happy to do that change and open a CSR for this issue.


This could be interesting for some non-ZipFile use cases such as reading
gzipped content from network streams - but I think considering making it
public should be done separately - along with some use case to motivate
it - and not hold back this RFE.

Thanks!

/Claes



Adding a cache for input stream buffers increases the speed of reading
ZipEntries from an InputStream by roughly 5% (see benchmark results below).
More importantly, it also decreases the memory consumption for each call to
ZipFile.getInputStream() which can be quite significant if many ZipEntries
are read from a ZipFile. One visible effect of caching the input buffers is
that the manual JTreg test java/util/zip/ZipFile/TestZipFile.java, which
regularly failed on my desktop with an OutOfMemoryError before, now
reliably passes (this tests calls ZipFile.getInputStream() excessively).

I've experimented with different buffer sizes (even with buffer sizes
depending on the size of the compressed ZipEntries), but couldn't see any
difference so I decided to go with a default buffer size of 65536 which
already was the maximal buffer size in use before my change.

I've also added a shortcut to Inflater which prevents us doing a native
call down to libz's inflate() method every time we call Inflater.inflate()
with "input == ZipUtils.defaultBuf" which is the default for every newly
created Inflater and for Inflaters after "Inflater.reset()" has been called
on them.

Following some JMH benchmark results which show the time and memory used to
read all bytes from a ZipEntry before and after this change. The 'size'
parameter denotes the uncompressed size of the corresponding ZipEntries.

In the "BEFORE" numbers, when looking at the "gc.alloc.rate.norm" values,
you can see the anomaly caused by using CENLEN instead of CENSIZ in
ZipFile.getInputStream(). I.e. getInputStream() chooses to big buffers
because it looks at the uncompressed ZipEntry sizes which are ~ 6 times
bigger than the compressed sizes. Also, the old implementation capped
buffers bigger than 65536 to 8192 bytes.

The memory savings for a call to getInputStream() are obviously the effect
of repetadly calling 

Re: [8u] RFR(S): 8244548: JDK 8u: sun.misc.Version.jdkUpdateVersion() returns wrong result

[Andrew Haley]

On 5/7/20 4:09 PM, Severin Gehwolf wrote:
> jvm_version_info.jvm_version currently holds this quadruplet:
>
> Most significant 8 bits => major version, followed by 8 bits => minor
> version, followed by 8 bits => micro version, followed by 8 bits =>
> build version. Note that JVM minor version represents the update
> version as passed in via configure and the micro version is currently
> not used (always 0). See vm_version.cpp lines 100-102 where only major,
> minor and build number are ever been set. Knowing this, we can still
> preserve the same behavior after patch by defining JVM_VERSION_MICRO to
> 0 for any version.

This is tricky. JVM_GetVersionInfo is a function exported by
libjvm.so, and the version is simply encoded as

unsigned int Abstract_VM_Version::jvm_version() {
  return ((Abstract_VM_Version::vm_major_version() & 0xFF) << 24) |
 ((Abstract_VM_Version::vm_minor_version() & 0xFF) << 16) |
 (Abstract_VM_Version::vm_build_number() & 0xFF);
}

I guess we could argue that this is for JVM-JDK internal use only, and
no-one else cares.

Or we could encode it in a different way such that at least we have a
jvm_version that is monotonically increasing, perhaps by (ab)using the
lower 8 bits of the version, the vm_build_number. But I guess I'm
being paranoid, and no tools are going to care about minor versions
anyway,even if they do call JVM_GetVersionInfo.

-- 
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

Re: RFR(s): 8244659: Improve ZipFile.getInputStream

[Volker Simonis]

Sure, here it is:

http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659.01/

Fortunately, it still applies cleanly :)
It also passed the zip-related JTreg tests and submit repo.

On Mon, May 11, 2020 at 10:27 PM Lance Andersen 
wrote:

> Hi Volker,
>
> Could you update your patch now that Claes’s changes are back as I think
> that would make it easier to review
>
> Thank you!
>
> On May 8, 2020, at 11:36 AM, Volker Simonis 
> wrote:
>
> Hi,
>
> can I please have a review for the following small enhancement which
> improves the speed of reading from ZipFile.getInputStream() by ~5%:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2020/8244659/
> https://bugs.openjdk.java.net/browse/JDK-8244659
>
> ZipFile.getInputStream() tries to find a good size for sizing the internal
> buffer of the underlying InflaterInputStream. This buffer is used to read
> the compressed data from the associated InputStream. Unfortunately,
> ZipFile.getInputStream() uses CENLEN (i.e. the uncompressed size of a
> ZipEntry) instead of CENSIZ (i.e. the compressed size of a ZipEntry) to
> configure the input buffer and thus unnecessarily wastes memory, because
> the corresponding, compressed input data is at most CENSIZ bytes long.
>
> After fixing this and doing some benchmarks, I realized that a much bigger
> problem is the continuous allocation of new, temporary input buffers for
> each new input stream. Assuming that a zip files usually has hundreds if
> not thousands of ZipEntries, I think it makes sense to cache these input
> buffers. Fortunately, ZipFile already has a built-in mechanism for such
> caching which it uses for caching the Inflaters needed for each new input
> stream. In order to cache the buffers as well, I had to add a new ,
> package-private constructor to InflaterInputStream. I'm not sure if it
> makes sense to make this new constructor public, to enable other users of
> InflaterInputStream to pre-allocate the buffer. If you think so, I'd be
> happy to do that change and open a CSR for this issue.
>
> Adding a cache for input stream buffers increases the speed of reading
> ZipEntries from an InputStream by roughly 5% (see benchmark results below).
> More importantly, it also decreases the memory consumption for each call to
> ZipFile.getInputStream() which can be quite significant if many ZipEntries
> are read from a ZipFile. One visible effect of caching the input buffers is
> that the manual JTreg test java/util/zip/ZipFile/TestZipFile.java, which
> regularly failed on my desktop with an OutOfMemoryError before, now
> reliably passes (this tests calls ZipFile.getInputStream() excessively).
>
> I've experimented with different buffer sizes (even with buffer sizes
> depending on the size of the compressed ZipEntries), but couldn't see any
> difference so I decided to go with a default buffer size of 65536 which
> already was the maximal buffer size in use before my change.
>
> I've also added a shortcut to Inflater which prevents us doing a native
> call down to libz's inflate() method every time we call Inflater.inflate()
> with "input == ZipUtils.defaultBuf" which is the default for every newly
> created Inflater and for Inflaters after "Inflater.reset()" has been called
> on them.
>
> Following some JMH benchmark results which show the time and memory used to
> read all bytes from a ZipEntry before and after this change. The 'size'
> parameter denotes the uncompressed size of the corresponding ZipEntries.
>
> In the "BEFORE" numbers, when looking at the "gc.alloc.rate.norm" values,
> you can see the anomaly caused by using CENLEN instead of CENSIZ in
> ZipFile.getInputStream(). I.e. getInputStream() chooses to big buffers
> because it looks at the uncompressed ZipEntry sizes which are ~ 6 times
> bigger than the compressed sizes. Also, the old implementation capped
> buffers bigger than 65536 to 8192 bytes.
>
> The memory savings for a call to getInputStream() are obviously the effect
> of repetadly calling getInputStream() on the same zip file (becuase only in
> that case, the caching of the input buffers pays of). But as I wrote
> before, I think it is common to have mor then a few entries in a zip file
> and even if not, the overhead of caching is minimal compared to the
> situation we had before the change.
>
> Thank you and best regards,
> Volker
>
> = BEFORE 8244659 =
> Benchmark  (size)
> Mode  Cnt  Score Error   Units
> ZipFileGetInputStream.readAllBytes   1024
> avgt3 13.577 ±   0.540   us/op
> ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm   1024
> avgt3   1872.673 ±   0.317B/op
> ZipFileGetInputStream.readAllBytes:·gc.count 1024
> avgt3 57.000counts
> ZipFileGetInputStream.readAllBytes:·gc.time  1024
> avgt3 15.000ms
> ZipFileGetInputStream.readAllBytes 

Re: RFR: JDK-8244634:, LoadLibraryW failed from tools/jpackage tests after JDK-8242302

[Kevin Rushforth]

Is there a way you can link the launcher (e.g., something similar to 
RPATH on Unix systems) to look in the right place relative to the 
launcher? Otherwise, the solution with adding to the PATH seems OK to me.


-- Kevin


On 5/12/2020 5:22 AM, Andy Herrick wrote:
Is the problem that by removing the copy of the microsoft dlls from 
the applications bin directory, then on machines that do not have all 
of these dll's already in the PATH (usually in C:\windows\system32) 
they can no longer be found ?


I don't like manually manipulating the PATH env variable either, but 
if so I don't see what else can be done short of putting the app exe 
in the bin dir of the runtime.


/Andy


On 5/11/2020 9:37 PM, Alexander Matveev wrote:

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try to 
load jli.dll by specifying full path to jli.dll when calling 
LoadLibary? If it does not work, then for AddDllDirectory() did you 
used LoadLibrary() or LoadLibraryEx() with 
LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use 
LoadLibraryEx() with flag when using AddDllDirectory().


Thanks,
Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

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

Fix failure to load jli.dll from app launcher. The fix is to append 
path to directory with jli.dll to PATH env variable and load jli.dll 
with altered value of PATH if the first attempt to load jli.dll 
without altering PATH fails.


- Alexey

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

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

Re: RFR: JDK-8244634:, LoadLibraryW failed from tools/jpackage tests after JDK-8242302

[Andy Herrick]

Is the problem that by removing the copy of the microsoft dlls from the 
applications bin directory, then on machines that do not have all of 
these dll's already in the PATH (usually in C:\windows\system32) they 
can no longer be found ?


I don't like manually manipulating the PATH env variable either, but if 
so I don't see what else can be done short of putting the app exe in the 
bin dir of the runtime.


/Andy


On 5/11/2020 9:37 PM, Alexander Matveev wrote:

Hi Alexey,

Updating PATH does not look like good solution to me. Did you try to 
load jli.dll by specifying full path to jli.dll when calling 
LoadLibary? If it does not work, then for AddDllDirectory() did you 
used LoadLibrary() or LoadLibraryEx() with 
LOAD_LIBRARY_SEARCH_USER_DIRS? According to doc you need to use 
LoadLibraryEx() with flag when using AddDllDirectory().


Thanks,
Alexander

On 5/11/2020 4:36 PM, Alexey Semenyuk wrote:

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

Fix failure to load jli.dll from app launcher. The fix is to append 
path to directory with jli.dll to PATH env variable and load jli.dll 
with altered value of PATH if the first attempt to load jli.dll 
without altering PATH fails.


- Alexey

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

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

Re: RFR: JDK-8209774 - [TESTBUG]Refactor shell test javax/xml/jaxp/common/8035437/run.sh to java

[Fernando Guallini]

Thanks for your comments.
Below you can find a new web rev version that includes a test description in a 
comment:
http://cr.openjdk.java.net/~fyuan/fernando/8209774/webrev.01/ 


I will also create a JBS ticket to revisit this test and remove it if obsolete, 
if that is fine with you.

-Fernando

> On 11 May 2020, at 17:27, Joe Wang  wrote:
> 
> 
> 
> On 5/11/2020 2:32 AM, Alan Bateman wrote:
>> On 08/05/2020 18:19, Fernando Guallini wrote:
>>> Hi Daniel and Alan,
>>> @compile/module=java.xml was my first try, but for the nature of this test, 
>>> it didn't work. The reason is that the original shell test does the 
>>> following:
>>> - Compiles it’s own version of Node and Document interfaces
>>> - Compiles DocumentImpl patching java.xml with those 2 interfaces
>>> - Executes the AbstractMethodErrorTest patching the java.xml module *only 
>>> with DocumentImpl* patch(not including Document and Node)
>>> 
>>> It does that by keeping the patches output in different folders. That’s 
>>> important otherwise AbstractMethodErrorTest doesn’t compile, because it 
>>> references some methods not declared in its custom interfaces, and it seems 
>>> to be coded this way to reproduce the original bug. That is also the reason 
>>> why I added the *@clean* command to remove Document and Node *before* 
>>> running the test.
>>> 
>>> But when using *@compile/module=java.xml*, under the hood, JTREG generates 
>>> a folder named “patches/java.xml/..” including all the compiled classes 
>>> under it. Unfortunately, the temporary interfaces can’t be removed because 
>>> *@clean* does not know how to resolve the path "/patches/java.xml/..”.
>>> 
>>> To sum up, this test creates a temporary java.xml patch that needs to be 
>>> ignored after compiling *DocumentImpl. *I am using —patch-module because 
>>> it’s more flexible than @compile/module
>>> *
>>> *
>>> Hope I explained myself!
>> This may be a question for Joe Wang but I'm curious if this need to 
>> override/upgrade W3C DOM API classes dates back to when it was an endorsed 
>> standard that could be overridden with the endorsed standards override 
>> mechanism. The @bug on the test suggests otherwise but I'm curious if it 
>> make any sense with JDK 9+. This doesn't impact the good work to replace the 
>> script of course, I'm just curious how separately compilation issue arises 
>> here.
> 
> I agree with you, Alan, that this test is obsolete. I just haven't thought of 
> getting rid of it. But you're right, it can be removed instead.
> 
> -Joe
> 
>> 
>> -Alan
>> 
>