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

[Alan Snyder]

> On May 8, 2020, at 6:46 PM, Jason Mehrens  wrote:
> 
> 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.

Hi Jason,

I agree that it is safe to say that existing code that is mixing collections 
with different membership semantics is already working around this issue, but 
from that I would conclude that it is hard to justify any changes that risk 
breaking existing code. It's the code that is not mixing membership semantics 
that I am concerned about breaking.

The main advantage of improving the framework is reducing the size of the 
tarpit that developers can fall into.

  Alan

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

[Alan Snyder]

I believe I can agree with you on a number of points.

• The specification is confusing.
• The problem areas involve the “membership contract”.
• There may have been some intent initially to support multiple 
membership semantics (this was new to me).
• There are inconsistencies in the specification (but we may disagree 
on where the inconsistencies are).

You say:

It's also somewhat odd to say that some collection implementations are 
conforming whereas others aren't.

Yes, it is odd, but I would say that the design is odd, and it is odd by choice.

Here’s the situation:

You have an abstract class or interface A and a concrete class C. The 
specification of class C depends upon certain parameters, such as the 
implementation of an abstract method or the choice of a type parameter or the 
choice of a constructor parameter. Some uses of C produce a specification that 
is consistent with the specification of A, but others produce specifications 
that are inconsistent with the specification of A.

As the designer of A and C, you have to make a choice:

• C does not extend or implement A. This choice makes the question of 
conformance moot. Even if a particular instance of C conforms to the 
specification of A, the type system prevents it from being used as an A. (I’m 
ignoring the extra complexity that arises if A is an interface and someone 
creates a subclass of C that implements A.)
• Define C to extend or implement A. This choice allows any instance of 
C to be used as an A, even the ones that do not conform to the specification of 
A.

The first choice avoids all of the problems we have been discussing, but it 
rules out cases that have no problems with non-conformance. Presumably on the 
grounds of greater utility, the designers of the Collection framework (or their 
successors) chose the second option for several concrete classes (the ones we 
have been discussing). Recognizing the issue of non-conformance, they chose to 
document it (often using bold type). Documentation is the best they could do 
given that it is not possible in general to detect improper use of 
non-conforming objects either at compile time or at run time.

If you believe that the possibility of non-conforming instances is inherently 
inconsistent and inconsistency must be fixed, then the only options are to 
remove the inheritance link or to weaken A so that C always conforms to A. I’m 
not sure that either option is practical in this case.

You quote the specification:

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.

This is exactly the case above, with A=Set and C=TreeSet.

(Actually, as the specification of TreeSet does not change the specifications 
of the basic methods in any significant way, it would more accurate to say that 
the TreeSet implementation does not conform to the TreeSet specification. That 
is certainly inconsistent, but it could be fixed by changing the specification 
of TreeSet, as you implied.)

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.

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

When I read the specification of Set, the only way I can make sense of it is 
that the specification of Set is defined in terms of mathematical sets.

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.

So, rather that *defining* set membership, the contains() method *reveals* it.

Other methods also reveal set membership, such as the iterator, or compute 
values that depend upon set membership, such as size() and hashCode(). They are 
defined in terms of “the elements of the set”, i.e., the mathematical set of 
instances that satisfies the equals rule.

The add() method specification also depends upon set membership, and it has the 
interesting property that it is defined to add at most one element 

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

[Jason Mehrens]

Stuart,

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

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.

Jason







From: core-libs-dev  on behalf of 
Stuart Marks 
Sent: Monday, May 4, 2020 7:25 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes



On 5/1/20 10:41 PM, Jason Mehrens wrote:
> 1. I assume you are using "c instanceof List" instead of "!(c instanceof 
> Set)" to correctly handle IdentitityHashMap.values()?  The instanceof List 
> seems like safe choice but it is too bad we can still fool that check by 
> wrapping List as an unmodifiableCollection.  If 
> splitIterator().characteristics() could tell us that the collection used 
> identity comparisons I think we would be able to determine if it was safe to 
> swap the ordering in the general case as we could check for IDENTITY, SORTED, 
> and comparator equality.

I'm using "instance List", not for the reason of IdentityHashMap.values()
specifically (though that's a good example), but mainly to try to be minimal.
While I think getting the semantics right takes priority, the change does impact
performance, so I want to reintroduce the performance heuristic in the safest
manner possible. Checking for !Set seems dangerous, as there might be any number
of Collections out there that aren't Sets and that aren't consistent with
equals. Checking for instanceof List seemed like the safest and most minimal
thing to do.

In fact, I'm not even sure how safe List is! It's certainly possible for someone
to have a List that isn't consistent with equals. Such a thing might violate the
List contract, but that hasn't stopped people from implementing such things
(outside the JDK).

I also toyed around with various additional tests for when it would be
profitable to switch iteration to the smaller collection. This could be applied
to a variety of consistent-with-equals set implementations in the JDK. The
benefits of swapping the iteration is more modest in these cases compared to
List, however. Avoiding repeated List.contains() helps avoid quasi-quadratic
(O(M*N)) performance. Swapping iteration order of sets gets us only the smaller
of O(M) vs O(N), which is still linear.

Also, as you noted, this heuristic is easily defeated by things like the
collection wrappers.

> 2. Should code applied to HashSet.removeAll also be applied to 
> HashMap.keySet().removeAll and HashMap.entrySet().removeAll?  
> Collections::newSetFromMap will end up having different behavior if it is not 
> consistently applied.

I think the *results* will be consistent, but the *performance* might not be
consistent.

But these cases could result in pathological performance if removeAll(list) is
called, so I'm a bit concerned about them. If we agree that "instanceof List" is
a reasonable heuristic, then I don't have any objection in principle to adding
it to these locations as well. But I want to be careful about sprinkling ad hoc
policies like this around the JDK.

I note that the erroneous size-based optimization was copied into, and therefore
needs to be removed from ConcurrentSkipListSet (JDK-8218945) and the set views
of CHM (JDK-8219069). I'd more concerned about getting these cleaned up in the
short term.

> 3. Not to derail this thread but do think we need a new JIRA ticket to 
> address Collections.disjoint?  Seems like it has similar sins of calling size 
> and using "!(c2 instanceof Set)" but I don't want to muddy the waters by 
> covering any solutions to that method in this thread.

Yeah I'm not sure what to do about Collections.disjoint().

Note that there are some 

Re: JDK-8226810: An other case and a small change suggestion

[Johannes Kuhn]

Thanks.

I think strcpy(ret+2, "1252") vs. strcpy(ret, "Cp1252")  is a just 
matter of style. I would prefer the later, as it makes the intent clear.

But not my call.

Do you have any info how I can change the detected codepage there? I 
wrote a small C program that basically just does that part and printf it.
In my limited tests (windows likes to require a restart after each 
configuration change) I did not find a way to influence that.


An other thing to consider is if Cp65001 should be treated as UTF-8 in 
that function?
(As said before, locale is not my expertise. Can that function with that 
LCSID even return 65001?)
I can see how things go wrong if it returns 65001 as locale, so... could 
be a safe change? (I'm sure that things break if that function returns 
65001.)


Then there is the other part:
The mismatch between the comment in jni_util.c/newSizedStringJava and 
the implementation on the Java side.
There is no fallback to iso-8859-1. If new String(byte[]) is called 
before the system properties are installed, then this will lead to a 
NullPointerException.
And there is a code path that leads to exactly that - newPlatformString 
is called from the initialization of the properties. [1]
And if the encoding returned by the windows function is not supported, 
then it will call new String(byte[]) - during system property 
initialization.


- Johannes

[1]: 
https://hg.openjdk.java.net/jdk/jdk/file/d40d865753fb/src/java.base/share/native/libjava/System.c#l207


On 08-May-20 18:27, naoto.s...@oracle.com wrote:

Ditto. Good catch!

I am not sure the fix would address the issue in 8226810 (cannot 
confirm it either, as my Windows box is at my office where I cannot 
enter at the moment :-), but this definitely looks like a bug. I would 
change the additional line to "strcpy(ret+2, "1252");" as Cp is copied 
in the following switch.


Naoto



On 5/7/20 5:50 AM, Alan Bateman wrote:

On 07/05/2020 12:37, Johannes Kuhn wrote:

:

In the end, I don't know what causes the bug, or how I can replicate 
it.

I think I did find a likely suspect.
Good sleuthing. I don't what the conditions are for GetLocaleInfo to 
fail but it does look like that would return possibly non-terminated 
garbage starting with "CP" so we should at least fix that.


The issue in JDK-8226810 might be something else. One of the 
submitters to that issue did engage and provided enough information 
to learn that the locale is zh_CN and also reported that it was 
failing for GB18030. GB18030 is not in java.base so that at least 
explained that report.


-Alan

Re: RFR: JDK-8244620: test failure in WinUpgradeUUIDTest

[Andy Herrick]

Revised webrev at [3] - this fixes the test problem that was caused by 
the test assuming wix tools are on all systems.


/Andy

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

On 5/7/2020 4:23 PM, Andy Herrick wrote:

please review fix for issue [1] at [2].

/Andy

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

[2] - http://cr.openjdk.java.net/~herrick/8244620/webrev.01/

Re: JDK-8226810: An other case and a small change suggestion

[Brent Christian]

On 5/7/20 5:50 AM, Alan Bateman wrote:

On 07/05/2020 12:37, Johannes Kuhn wrote:

:

In the end, I don't know what causes the bug, or how I can replicate it.
I think I did find a likely suspect.

>
Good sleuthing. I don't what the conditions are for GetLocaleInfo to 
fail but it does look like that would return possibly non-terminated 
garbage starting with "CP" so we should at least fix that.


I agree.  I can file a bug and sponsor this small change.

-Brent

Re: JDK-8226810: An other case and a small change suggestion

[naoto . sato]

Hi Johannes,

On 5/8/20 1:37 PM, Johannes Kuhn wrote:

Thanks.

I think strcpy(ret+2, "1252") vs. strcpy(ret, "Cp1252")  is a just 
matter of style. I would prefer the later, as it makes the intent clear.

But not my call.


I thought the former was clearer, as at that point, Cp/MS/GB part is not 
initialized in normal cases. It follows that pattern.




Do you have any info how I can change the detected codepage there? I 
wrote a small C program that basically just does that part and printf it.
In my limited tests (windows likes to require a restart after each 
configuration change) I did not find a way to influence that.


Have you tried changing the Windows System Locale to Japanese? I am 
pretty sure the code will return MS932.




An other thing to consider is if Cp65001 should be treated as UTF-8 in 
that function?
(As said before, locale is not my expertise. Can that function with that 
LCSID even return 65001?)
I can see how things go wrong if it returns 65001 as locale, so... could 
be a safe change? (I'm sure that things break if that function returns 
65001.)


Yes it should return UTF-8, which is not implemented. If the code page 
is 65001, then the following switch should put UTF-8 as the default charset.




Then there is the other part:
The mismatch between the comment in jni_util.c/newSizedStringJava and 
the implementation on the Java side.
There is no fallback to iso-8859-1. If new String(byte[]) is called 
before the system properties are installed, then this will lead to a 
NullPointerException.
And there is a code path that leads to exactly that - newPlatformString 
is called from the initialization of the properties. [1]
And if the encoding returned by the windows function is not supported, 
then it will call new String(byte[]) - during system property 
initialization.


I would expect there shouldn't be a mismatch, i.e., all the default 
system locale in Windows should return *known* default charset. 
Returning UTF-8 in java_props_md.c should resolve this.


Naoto



- Johannes

[1]: 
https://hg.openjdk.java.net/jdk/jdk/file/d40d865753fb/src/java.base/share/native/libjava/System.c#l207


On 08-May-20 18:27, naoto.s...@oracle.com wrote:

Ditto. Good catch!

I am not sure the fix would address the issue in 8226810 (cannot 
confirm it either, as my Windows box is at my office where I cannot 
enter at the moment :-), but this definitely looks like a bug. I would 
change the additional line to "strcpy(ret+2, "1252");" as Cp is copied 
in the following switch.


Naoto



On 5/7/20 5:50 AM, Alan Bateman wrote:

On 07/05/2020 12:37, Johannes Kuhn wrote:

:

In the end, I don't know what causes the bug, or how I can replicate 
it.

I think I did find a likely suspect.
Good sleuthing. I don't what the conditions are for GetLocaleInfo to 
fail but it does look like that would return possibly non-terminated 
garbage starting with "CP" so we should at least fix that.


The issue in JDK-8226810 might be something else. One of the 
submitters to that issue did engage and provided enough information 
to learn that the locale is zh_CN and also reported that it was 
failing for GB18030. GB18030 is not in java.base so that at least 
explained that report.


-Alan

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

[Stuart Marks]

On 5/6/20 5:05 PM, Alan Snyder wrote:
Let me clarify. I was referring to the non-support by the framework, which 
restricts membership semantics:

...[specification text regarding equals()]...


OK, thanks for the clarification.

Although the framework provides implementation classes that MAY be used to 
create nonconforming collections, these classes are called out as potentially 
non-conforming. For example:


[IdentityHashMap]

*This class is /not/ a general-purpose |Map| implementation! While this class 
implements the |Map| interface, it intentionally violates |Map's| general 
contract, which mandates the use of the |equals| method when comparing objects. 
This class is designed for use only in the rare cases wherein reference-equality 
semantics are required.*


And in many places, the implications of non-conformance are also called out:

*While the object returned by this method implements the |Collection| interface, 
it does /not/ obey |Collection's| general contract. Like its backing map, the 
collection returned by this method defines element equality as 
reference-equality rather than object-equality. This affects the behavior of its 
|contains|, |remove| and |containsAll| methods.*


The phrase “affects the behavior” is open ended. It means all bets are off.


No, all bets are not off.

"All bets are off" isn't an unreasonable interpretation of the current wording 
of the specification, but in fact the specification is incredibly poorly worded 
and confusing. Plus there are several out-and-out bugs in the spec.


It's also somewhat odd to say that some collection implementations are 
conforming whereas others aren't. Implementations can have bugs that render them 
non-conformant to a specification. In this case, though, parts of the 
specification contradict each other. You could pick part A of the spec and say 
that part B is "non-conformant" but that really doesn't make sense if you 
consider the totality of the specification. The fact is that the spec as a whole 
is self-inconsistent. I intend to fix that.


My point is that (fully) supporting custom membership semantics changes a 
fundamental property of the original design and should be approached as a 
redesign. It is not a matter of fixing a series of issues one at a time, as you 
discover them. Calling these issues semantic bugs when there is no violation of 
the specification is leading you down the wrong path, in my opinion.


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

If you look at the history, the collections framework was added in JDK 1.2, and 
the JDK 1.2 specification includes both Set and SortedSet. There is the wording 
in the SortedSet specification that is very similar to what exists today: 
[edited for brevity]


Note that the ordering maintained by a sorted set must be consistent with
equals if the sorted set is to correctly implement the Set interface.
This is so because the Set interface is defined in terms of the equals
operation, but a sorted set performs all element comparisons using its
compareTo (or compare) method. 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 original designers (including Josh Bloch, and probably others) clearly 
thought about these issues enough to put that wording in the specification. I 
don't think they thought through the full implications of this, or at least they 
didn't write it down, hence the vague wording above.


In addition, the issue was forgotten, and over time, bugs were introduced in the 
system that made things worse. For example, the "optimization" that is the root 
cause of the bug we are discussing (JDK-6394757) was introduced in JDK 1.3. In 
the comments on this bug report [1] Bloch is quoted as saying


The spec clearly states that removeAll "Removes all this collection's
elements that are also contained in the specified collection." So the
current behavior is not just unpredictable, it's broken. It worked until
1.3, and was broken in 1.3-1.5. Sigh...

(And indeed, AbstractCollection.removeAll does precisely what the
Collection.removeAll spec demands)

[1] 
https://bugs.openjdk.java.net/browse/JDK-6394757?focusedCommentId=12242803=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12242803


Another bug, a spec bug this time, was introduced later. (I haven't searched the 
history to find out exactly when.) The TreeSet.add() method specification says 
this: [2]


Adds the specified element to this set if it is not already present. More
formally, adds the specified element e to this set if the set contains no
element e2 such that Objects.equals(e, e2). If this set already contains the
element, the call leaves the set unchanged and returns false.

[2] 
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/util/TreeSet.html#add(E)

Re: RFR: 8244624: Improve handling of JarFile META-INF resources

[Claes Redestad]

I updated open.01 with a reworked microbenchmark that uses
JarFile.getInputStream to trigger the "maybe instantiate
the JAR verifier" logic.

This exercise the code being optimized in this patch better,
while also being a little bit more realistic. Improvements around
1.3x-1.5x across tested jar variants, along with 20-35% allocation
reductions. See the source for sampled results.

Thanks!

/Claes

On 2020-05-08 12:57, Claes Redestad wrote:

Hi Max,

On 2020-05-08 09:08, Weijun Wang wrote:

JarFile.java:

734: extra new line?


Fixed


930: Remove or rewrite the comment.


Did even better: now that I find the position of the manifest during
initCEN, this method should always call JUZFA.getManifest(this, false);
- which is both a simplification and an optimization.



ZipFile.java:

49: seems not used


Fixed



Please add links to each other between 
src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java::isBlockOrSF 
and 
src/java.base/share/classes/java/util/zip/ZipFile.java::isSignatureRelated. 
The 2nd method can also be static.


I assume you cannot put ZipFile::isSignatureRelated into 
SignatureFileVerifier.java, right?


Right, that wouldn't be great for either since ZipFile operates directly
on UTF-8 encoded bytes for performance, while SignatureFileVerifier
works on Strings.

What we can do though is to add in an assert that the result of
ZF::isSignatureRelated matches that of SFV::isBlockOrSF - which should
ensure. I also added a note to SignatureFileVerifier::isBlockOfSF to
keep these in sync:

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

Testing: re-running tier1+2



Thanks,
Max

p.s. How do you run the benchmark test? Both locally and on mach5.


See doc/testing.md

Basically:
Configure with --with-jmh (jib does this automatically)
make build-microbenchmark
$JDK/bin/java -jar build/$CONF/images/test/micro/benchmarks.jar JarFileMeta

Thanks!

/Claes


On May 8, 2020, at 5:28 AM, Claes Redestad 
 wrote:


Hi,

currently during ZipFile creation, we create an int[] array of pointers
to META-INF entries. These are then retrieved from three different
places in JarFile.

However, JarFile is only interested in some combination a few things:
the existence of and name of the META-INF/MANIFEST file, the existence
of and the names of various signature related files, i.e., files in
META-INF that have a suffix such as .EC, .SF, .RSA and .DSA

Refactoring the contract between JarFile and ZipFile means we can filter
out such entries that we're not interested when opening the file, and
also remove the need to create the String for each entries unless we
actually want them:

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

This reduces retained footprint of Jar-/ZipFile by slimming down or
removing the Source.metanames array entirely, and a significant speed-up
in some cases.

In the provided microbenchmark, getManifestFromJarWithManifest and
getManifestFromJarWithSignatureFiles doesn't call
JUZFA.getMetaInfEntryNames but still see small (~5%) speed-up decrease
in allocations.

getManifestFromJarWithNoManifest exercise JUZFA.getMetaInfEntryNames in
the baseline, and now calls JUZFA.getManifestName. Result is a 1.25x
speed-up and 30% reduction in allocations. While unrealistic (most JARs
have a META-INF/MANIFEST.MF), this speed-up will translate to a few
places - such as when loading classes from potentially-signed JAR files.

Testing: tier1-2

Thanks!

/Claes

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

[Joe Wang]

Hi Fernando,

While you add the comment (about compile/run), you may want to update 
the original comment about the test as well as it mentioned "see run.sh" 
that is now gone.


A minor comment on the header:  a space between the years would be good, 
that is "2014,2020," -> 2014, 2020,


-Joe

On 5/8/2020 10:49 AM, Fernando Guallini wrote:

Sure, I will add some of the explanation as a comment. Thank you Daniel!


On 8 May 2020, at 18:39, Daniel Fuchs  wrote:

Hi Fernando,

Ah - I see the keypoint is the:
  39  * @clean org.w3c.dom.*
OK, I missed that. Then I agree - ignore my previous comments.

Maybe the explanations below should be added to the test in some
comment to help future maintainers (and reviewers).

best regards,

-- daniel


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!

On 8 May 2020, at 15:49, Daniel Fuchs mailto:daniel.fu...@oracle.com>> wrote:

On 08/05/2020 15:40, Alan Bateman wrote:

   31 /*
   32  * @test
   33  * @bug 8035437
   34  * @summary Tests that java.lang.AbstractMethodError is not thrown when
   35  * serializing improper version of DocumentImpl class.
   36  * @library /test/lib
   * @modules javax.xml/org.w3c.dom
   *  javax.xml/com.sun.org.apache.xerces.internal.dom
   40  * @run main/othervm --patch-module java.xml=${test.class.path} 
AbstractMethodErrorTest
   41  */

(not 100% sure the @modules is even needed)

I wouldn't expect to need --patch-module here. Instead maybe it could be changed to use 
@compile/module=java.xml ... and jtreg should compile and run the overrides "as 
if" they are in the java.xml module. There are a couple of examples of this in the 
test suite that might help get this going. No need for javax.xml/org.w3c.dom as that 
package is already exported.

Right. Copy paste error. The --patch-module shouldn't be needed
anywhere. Good point about @compile - the main class
AbstractMethodErrorTest is not in the patched module, so
the patched classes may not get compiled if you don't force
their compilation.

Thanks for the correction Alan!

-- daniel

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

[Brent Christian]

Hi, Philipp.  Sorry for the further delay.

Just to step back a bit:

The current manifest-writing behavior dates back to JDK 1.2 (at least), 
so compatibility is an overriding concern.  It is unfortunate that 
multi-byte characters weren't better accounted for in the placement of 
line breaks from the beginning.


But typically in cases like this, we would update the specification to 
fit the long-standing behavior, and avoid the risk of breaking 
applications in the wild.


We can test that manifests written with your changes can be read by the 
JDK.  But there is also manifest-reading code external to the JDK.  Some 
examples, that would need to be investigated:


jarmanifest
https://pypi.org/project/jarmanifest/

Chromium
https://chromium.googlesource.com/external/googleappengine/python/+/7e0ab775c587657f0f93a3134f2db99e46bb98bd/google/appengine/tools/jarfile.py

Maven
http://maven.apache.org/shared/maven-archiver/index.html

Apache Ant
https://en.wikipedia.org/wiki/JAR_(file_format)#Apache_Ant_Zip/JAR_support

IDEs would be another good place to check, and maybe also consumers of 
JavaEE WAR / EAR files.



Performance is also a consideration.  JMH benchmarks would be needed to 
confirm that reading the new manifest is not slower, and to gauge any 
regression in Manifest writing performance.



So that is the work that would need to be done to support this change.

The question then will be if the benefit of this change (seen only 
outside of running code) outweighs the risk of changing behavior that 
has not been an issue for 20+ years.  It seems unlikely that a strong 
enough case could be made.


-Brent

On 4/13/20 10:29 AM, Philipp Kunz wrote:

Hi Naoto,
You are absolutely right to raise the question. I've also thought about
this but haven't come up so far with a compellingly elegant solution,
at least not yet.
If only String.isLatin1 was public that would come in very handy.
Character or anything else I looked at cannot tell if a string is
ascii. BreakIterator supports iterating backwards so we could start at
the potential line end but that requires a start position that is a
boundary to start with and that is not obviously possible due to the
regional indicators and probably other code points requiring stateful
processing. Same with regexes and I wouldn't know how to express groups
that could count bytes. It does not even seem to be possible to guess
any number of characters to start searching for a boundary because of
the statefulness. Even the most simple approach to detect latin1
Strings requires an encoding or a regex such as "[^\\p{ASCII}]" which
essentially is another inefficient loop. It also does not work to
encode the string into UTF-8 in a single pass because then it is not
known which grapheme boundary matches to which byte position. I also
tried to write the header names and the ": " delimiter without breaking
first but it did not seem to significantly affect performance.
UTF_8.newEncoder cannot process single surrogates, admittedly an edge
case, but required for compatibility. I added a fast path, see patch,
the best way I could think of. Did I miss a better way to tell ascii
strings from others?
What I found actually improving performance is based on the
consideration that strings are composed of primitive chars that will be
encoded into three bytes each maximum always that up to 24 characters
can be passed to writeChar72 at a time reducing the number of
writeChar72 and in turn String.getBytes invocations. The number of
characters that can be passed to writeChar72 is at most the number of
bytes remaining on the current manifest line (linePos) divided by three
but at least one. See attached patch.
Regards,Philipp

On Mon, 2020-03-30 at 14:31 -0700, naoto.s...@oracle.com wrote:

Hi Philipp,
Sorry for the delay.
On 3/25/20 11:52 AM, Philipp Kunz wrote:

Hi Naoto,
See another patch attached with Locale.ROOT for the BreakIterator.
I will be glad to hear of any feedback.


I wonder how your change affects the performance, as it will do
String.getBytes(UTF-8) per each character. I think this can
definitely be improved by adding some fastpath, e.g., for ASCII. The
usage of the BreakIterator is fine, though.

There is another patch [1] around dealing with manifest attributes
during application launch. It certainly is related to 6202130 but
feels like a distinct set of changes with a slightly different
concern. Any opinion as how to proceed with that one?


I am not quite sure which patch you are referring to, but I agree
that creating an issue would not hurt.
Naoto

Regards,Philipp


[1]
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-February/064720.html


On Mon, 2020-03-23 at 09:05 -0700, naoto.s...@oracle.com wrote:

Hi Philipp,
Right, Locale.ROOT is more appropriate here by definition, though
theimplementation is the same.
Naoto
On 3/21/20 5:19 AM, Philipp Kunz wrote:

Hi Naoto and everyone,
There are almost as many occurrences of Locale.ROOT as
Locale.US whichmade me 

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

[Fernando Guallini]

Sure, I will add some of the explanation as a comment. Thank you Daniel!

> On 8 May 2020, at 18:39, Daniel Fuchs  wrote:
> 
> Hi Fernando,
> 
> Ah - I see the keypoint is the:
>  39  * @clean org.w3c.dom.*
> OK, I missed that. Then I agree - ignore my previous comments.
> 
> Maybe the explanations below should be added to the test in some
> comment to help future maintainers (and reviewers).
> 
> best regards,
> 
> -- daniel
> 
> 
> 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!
>>> On 8 May 2020, at 15:49, Daniel Fuchs >> > wrote:
>>> 
>>> On 08/05/2020 15:40, Alan Bateman wrote:
>   31 /*
>   32  * @test
>   33  * @bug 8035437
>   34  * @summary Tests that java.lang.AbstractMethodError is not thrown 
> when
>   35  * serializing improper version of DocumentImpl class.
>   36  * @library /test/lib
>   * @modules javax.xml/org.w3c.dom
>   *  javax.xml/com.sun.org.apache.xerces.internal.dom
>   40  * @run main/othervm --patch-module java.xml=${test.class.path} 
> AbstractMethodErrorTest
>   41  */
> 
> (not 100% sure the @modules is even needed)
 I wouldn't expect to need --patch-module here. Instead maybe it could be 
 changed to use @compile/module=java.xml ... and jtreg should compile and 
 run the overrides "as if" they are in the java.xml module. There are a 
 couple of examples of this in the test suite that might help get this 
 going. No need for javax.xml/org.w3c.dom as that package is already 
 exported.
>>> 
>>> Right. Copy paste error. The --patch-module shouldn't be needed
>>> anywhere. Good point about @compile - the main class
>>> AbstractMethodErrorTest is not in the patched module, so
>>> the patched classes may not get compiled if you don't force
>>> their compilation.
>>> 
>>> Thanks for the correction Alan!
>>> 
>>> -- daniel
> 

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

[Daniel Fuchs]

Hi Fernando,

Ah - I see the keypoint is the:
  39  * @clean org.w3c.dom.*
OK, I missed that. Then I agree - ignore my previous comments.

Maybe the explanations below should be added to the test in some
comment to help future maintainers (and reviewers).

best regards,

-- daniel


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!


On 8 May 2020, at 15:49, Daniel Fuchs > wrote:


On 08/05/2020 15:40, Alan Bateman wrote:

  31 /*
  32  * @test
  33  * @bug 8035437
  34  * @summary Tests that java.lang.AbstractMethodError is not 
thrown when

  35  * serializing improper version of DocumentImpl class.
  36  * @library /test/lib
  * @modules javax.xml/org.w3c.dom
  *  javax.xml/com.sun.org.apache.xerces.internal.dom
  40  * @run main/othervm --patch-module java.xml=${test.class.path} 
AbstractMethodErrorTest

  41  */

(not 100% sure the @modules is even needed)
I wouldn't expect to need --patch-module here. Instead maybe it could 
be changed to use @compile/module=java.xml ... and jtreg should 
compile and run the overrides "as if" they are in the java.xml 
module. There are a couple of examples of this in the test suite that 
might help get this going. No need for javax.xml/org.w3c.dom as that 
package is already exported.


Right. Copy paste error. The --patch-module shouldn't be needed
anywhere. Good point about @compile - the main class
AbstractMethodErrorTest is not in the patched module, so
the patched classes may not get compiled if you don't force
their compilation.

Thanks for the correction Alan!

-- daniel

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

[Fernando Guallini]

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!


> On 8 May 2020, at 15:49, Daniel Fuchs  wrote:
> 
> On 08/05/2020 15:40, Alan Bateman wrote:
>>>   31 /*
>>>   32  * @test
>>>   33  * @bug 8035437
>>>   34  * @summary Tests that java.lang.AbstractMethodError is not thrown when
>>>   35  * serializing improper version of DocumentImpl class.
>>>   36  * @library /test/lib
>>>   * @modules javax.xml/org.w3c.dom
>>>   *  javax.xml/com.sun.org.apache.xerces.internal.dom
>>>   40  * @run main/othervm --patch-module java.xml=${test.class.path} 
>>> AbstractMethodErrorTest
>>>   41  */
>>> 
>>> (not 100% sure the @modules is even needed)
>> I wouldn't expect to need --patch-module here. Instead maybe it could be 
>> changed to use @compile/module=java.xml ... and jtreg should compile and run 
>> the overrides "as if" they are in the java.xml module. There are a couple of 
>> examples of this in the test suite that might help get this going. No need 
>> for javax.xml/org.w3c.dom as that package is already exported.
> 
> Right. Copy paste error. The --patch-module shouldn't be needed
> anywhere. Good point about @compile - the main class
> AbstractMethodErrorTest is not in the patched module, so
> the patched classes may not get compiled if you don't force
> their compilation.
> 
> Thanks for the correction Alan!
> 
> -- daniel

Re: JDK-8226810: An other case and a small change suggestion

[naoto . sato]

Ditto. Good catch!

I am not sure the fix would address the issue in 8226810 (cannot confirm 
it either, as my Windows box is at my office where I cannot enter at the 
moment :-), but this definitely looks like a bug. I would change the 
additional line to "strcpy(ret+2, "1252");" as Cp is copied in the 
following switch.


Naoto



On 5/7/20 5:50 AM, Alan Bateman wrote:

On 07/05/2020 12:37, Johannes Kuhn wrote:

:

In the end, I don't know what causes the bug, or how I can replicate it.
I think I did find a likely suspect.
Good sleuthing. I don't what the conditions are for GetLocaleInfo to 
fail but it does look like that would return possibly non-terminated 
garbage starting with "CP" so we should at least fix that.


The issue in JDK-8226810 might be something else. One of the submitters 
to that issue did engage and provided enough information to learn that 
the locale is zh_CN and also reported that it was failing for GB18030. 
GB18030 is not in java.base so that at least explained that report.


-Alan

RFR(s): 8244659: Improve ZipFile.getInputStream

[Volker Simonis]

 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   4096
avgt3 20.938 ±   0.577   us/op
ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm   4096
avgt3   4945.793 ±   0.493B/op
ZipFileGetInputStream.readAllBytes:·gc.count 4096
avgt3102.000counts
ZipFileGetInputStream.readAllBytes:·gc.time  4096
avgt3 25.000ms
ZipFileGetInputStream.readAllBytes  16384
avgt3 51.348 ±   2.600   us/op
ZipFileGetInputStream.readAllBytes:·gc.alloc.rate.norm  16384
avgt3  

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

[Daniel Fuchs]

On 08/05/2020 15:40, Alan Bateman wrote:

  31 /*
  32  * @test
  33  * @bug 8035437
  34  * @summary Tests that java.lang.AbstractMethodError is not 
thrown when

  35  * serializing improper version of DocumentImpl class.
  36  * @library /test/lib
  * @modules javax.xml/org.w3c.dom
  *  javax.xml/com.sun.org.apache.xerces.internal.dom
  40  * @run main/othervm --patch-module java.xml=${test.class.path} 
AbstractMethodErrorTest

  41  */

(not 100% sure the @modules is even needed)
I wouldn't expect to need --patch-module here. Instead maybe it could be 
changed to use @compile/module=java.xml ... and jtreg should compile and 
run the overrides "as if" they are in the java.xml module. There are a 
couple of examples of this in the test suite that might help get this 
going. No need for javax.xml/org.w3c.dom as that package is already 
exported.


Right. Copy paste error. The --patch-module shouldn't be needed
anywhere. Good point about @compile - the main class
AbstractMethodErrorTest is not in the patched module, so
the patched classes may not get compiled if you don't force
their compilation.

Thanks for the correction Alan!

-- daniel

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

[Alan Bateman]

On 08/05/2020 15:16, Daniel Fuchs wrote:

:

then:

  31 /*
  32  * @test
  33  * @bug 8035437
  34  * @summary Tests that java.lang.AbstractMethodError is not 
thrown when

  35  * serializing improper version of DocumentImpl class.
  36  * @library /test/lib
  * @modules javax.xml/org.w3c.dom
  *  javax.xml/com.sun.org.apache.xerces.internal.dom
  40  * @run main/othervm --patch-module java.xml=${test.class.path} 
AbstractMethodErrorTest

  41  */

(not 100% sure the @modules is even needed)
I wouldn't expect to need --patch-module here. Instead maybe it could be 
changed to use @compile/module=java.xml ... and jtreg should compile and 
run the overrides "as if" they are in the java.xml module. There are a 
couple of examples of this in the test suite that might help get this 
going. No need for javax.xml/org.w3c.dom as that package is already 
exported.


-Alan

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

[Daniel Fuchs]

Hi Fernando,

I believe that if you simply move the patched file
under a directory named javax.xml, then you can get
rid of all the @compile in the test.
Starting from your patch:

cd  test/jdk/javax/xml/jaxp/common/8035437
mkdir java.xml
hg move org javax.xml
hg move com javax.xml

then:

  31 /*
  32  * @test
  33  * @bug 8035437
  34  * @summary Tests that java.lang.AbstractMethodError is not thrown 
when

  35  * serializing improper version of DocumentImpl class.
  36  * @library /test/lib
  * @modules javax.xml/org.w3c.dom
  *  javax.xml/com.sun.org.apache.xerces.internal.dom
  40  * @run main/othervm --patch-module java.xml=${test.class.path} 
AbstractMethodErrorTest

  41  */

(not 100% sure the @modules is even needed)

best regards,

-- daniel

On 08/05/2020 13:58, Fernando Guallini wrote:

Hi all,

Please, review the following change:

webrev: http://cr.openjdk.java.net/~fyuan/fernando/8209774/webrev.00/ 

Testbug: https://bugs.openjdk.java.net/browse/JDK-8209774 


Change details:
- Refactor shell test to java

That test was originally created to check XERCESJ-1007 bug fix, that 
java.lang.AbstractMethodError is not thrown when patching JDK boot class 
DocumentImpl with its own.

Kind regards,
Fernando

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

[Fernando Guallini]

Hi all,

Please, review the following change:

webrev: http://cr.openjdk.java.net/~fyuan/fernando/8209774/webrev.00/ 

Testbug: https://bugs.openjdk.java.net/browse/JDK-8209774 


Change details:
- Refactor shell test to java

That test was originally created to check XERCESJ-1007 bug fix, that 
java.lang.AbstractMethodError is not thrown when patching JDK boot class 
DocumentImpl with its own. 

Kind regards,
Fernando

Re: RFR: 8244624: Improve handling of JarFile META-INF resources

[Claes Redestad]

Hi Max,

On 2020-05-08 09:08, Weijun Wang wrote:

JarFile.java:

734: extra new line?


Fixed


930: Remove or rewrite the comment.


Did even better: now that I find the position of the manifest during
initCEN, this method should always call JUZFA.getManifest(this, false);
- which is both a simplification and an optimization.



ZipFile.java:

49: seems not used


Fixed



Please add links to each other between 
src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java::isBlockOrSF
 and 
src/java.base/share/classes/java/util/zip/ZipFile.java::isSignatureRelated. The 
2nd method can also be static.

I assume you cannot put ZipFile::isSignatureRelated into 
SignatureFileVerifier.java, right?


Right, that wouldn't be great for either since ZipFile operates directly
on UTF-8 encoded bytes for performance, while SignatureFileVerifier
works on Strings.

What we can do though is to add in an assert that the result of
ZF::isSignatureRelated matches that of SFV::isBlockOrSF - which should
ensure. I also added a note to SignatureFileVerifier::isBlockOfSF to
keep these in sync:

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

Testing: re-running tier1+2



Thanks,
Max

p.s. How do you run the benchmark test? Both locally and on mach5.


See doc/testing.md

Basically:
Configure with --with-jmh (jib does this automatically)
make build-microbenchmark
$JDK/bin/java -jar build/$CONF/images/test/micro/benchmarks.jar JarFileMeta

Thanks!

/Claes



On May 8, 2020, at 5:28 AM, Claes Redestad  wrote:

Hi,

currently during ZipFile creation, we create an int[] array of pointers
to META-INF entries. These are then retrieved from three different
places in JarFile.

However, JarFile is only interested in some combination a few things:
the existence of and name of the META-INF/MANIFEST file, the existence
of and the names of various signature related files, i.e., files in
META-INF that have a suffix such as .EC, .SF, .RSA and .DSA

Refactoring the contract between JarFile and ZipFile means we can filter
out such entries that we're not interested when opening the file, and
also remove the need to create the String for each entries unless we
actually want them:

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

This reduces retained footprint of Jar-/ZipFile by slimming down or
removing the Source.metanames array entirely, and a significant speed-up
in some cases.

In the provided microbenchmark, getManifestFromJarWithManifest and
getManifestFromJarWithSignatureFiles doesn't call
JUZFA.getMetaInfEntryNames but still see small (~5%) speed-up decrease
in allocations.

getManifestFromJarWithNoManifest exercise JUZFA.getMetaInfEntryNames in
the baseline, and now calls JUZFA.getManifestName. Result is a 1.25x
speed-up and 30% reduction in allocations. While unrealistic (most JARs
have a META-INF/MANIFEST.MF), this speed-up will translate to a few
places - such as when loading classes from potentially-signed JAR files.

Testing: tier1-2

Thanks!

/Claes

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports - (core libraries)

[Magnus Ihse Bursie]

On 2020-05-07 23:27, Mikael Vidstedt wrote:



On May 7, 2020, at 7:52 AM, Kumar Srinivasan  wrote:

Hi Mikael,

I may have created solinux when the macosx port was merged and in an effort to 
reduce the CPP conditionals.
solinux = solaris + linux  ie. Vanilla unix code vs Darwin code IIRC has 
Objective-C, MacOSX specific thread initialization etc.

Thank you for the background - it all makes sense now! :)
If the file is truly only used on linux, it should be moved to 
java.base/linux/native/libjli instead. (And be renamed.) If it's still 
used on other unix platforms (read: AIX) it should probably be renamed 
java_md_unix.h instead, and kept in place.


If this change should be done in a follow-up fix or as part of JEP 381, 
I leave to you to decide.


(edit: *actually looking at the files* ... )

Hm. The include block looks like this:
#if defined(_AIX)
#include "java_md_aix.h"
#endif

#ifdef MACOSX
#include "java_md_macosx.h"
#else  /* !MACOSX */
#include "java_md_solinux.h"
#endif /* MACOSX */
#endif /* JAVA_MD_H */

It would probably make sense to make this a three-pronged include with 
separate files for aix, macosx and linux, but that will probably require 
stuff to migrate from java_md_solinux.h to java_md_aix.h, or at the very 
least, proper testing on AIX. Recommend filing follow-up issue to sort 
this out.


/Magnus



I looked over the launcher related code looks ok to me.

I did notice the \n endings for the messages that Brent pointed out.

Thank you for the review! The line ending should be fixed in webrev.01.

Cheers,
Mikael


On May 6, 2020, at 9:43 PM, Mikael Vidstedt mailto:mikael.vidst...@oracle.com>> wrote:


I have always wondered what “solinux” is supposed to mean - though not enough 
to actually ask anybody :)

I’ll file a follow-up enhancement to cover renaming the files.

Thank you for the review!

Cheers,
Mikael


On May 4, 2020, at 7:59 AM, Roger Riggs mailto:roger.ri...@oracle.com>> wrote:

Hi Michael,

Looks good.

Maybe just a future cleanup to rename files, since the "...so..." is refering 
to solaris.

src/java.base/unix/native/libjli/java_md_solinux.h
src/java.base/unix/native/libjli/java_md_solinux.h

Regards, Roger


On 5/4/20 4:49 AM, Alan Bateman wrote:

On 04/05/2020 06:12, Mikael Vidstedt wrote:

Please review this change which implements part of JEP 381:

JBS: 
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8244224data=02%7C01%7Ckusrinivasan%40vmware.com%7Ce48bd44f0c484e6fd85608d7f243f1ff%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637244245954061434sdata=W5VvD1owIGoUcbcRkcwixXGPkFLjHUFof2v6cxMqchk%3Dreserved=0
 

webrev: 
https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~mikael%2Fwebrevs%2F8244224%2Fwebrev.00%2Fcorelibs%2Fopen%2Fwebrev%2Fdata=02%7C01%7Ckusrinivasan%40vmware.com%7Ce48bd44f0c484e6fd85608d7f243f1ff%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637244245954061434sdata=tQZIjuF5cIPs%2FNqTRtY2WtmwAgwa0iv205wQ9vk0vOQ%3Dreserved=0
 

JEP: 
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8241787data=02%7C01%7Ckusrinivasan%40vmware.com%7Ce48bd44f0c484e6fd85608d7f243f1ff%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637244245954061434sdata=rDvBE%2BJ2F1IIFC9fbA92rs0KZNgYPg0JZM2ynqp7mcs%3Dreserved=0
 



Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


I took a pass over the changes. I agree its a bit tedious. I'm sure there will 
be a few follow up issues as there are opportunities for cleanup in several 
areas. Just a few comments/questions from a first pass.

ExtendedSocketOption.SO_FLOW_SLA is the Solaris specific socket option that was 
terminally deprecated in 14. The patch removes the implementation but leave the 
API (SO_FLOW_SA and 

Re: RFR: 8244624: Improve handling of JarFile META-INF resources

[Weijun Wang]

JarFile.java:

734: extra new line?
930: Remove or rewrite the comment.

ZipFile.java:

49: seems not used

Please add links to each other between 
src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java::isBlockOrSF
 and 
src/java.base/share/classes/java/util/zip/ZipFile.java::isSignatureRelated. The 
2nd method can also be static.

I assume you cannot put ZipFile::isSignatureRelated into 
SignatureFileVerifier.java, right?

Thanks,
Max

p.s. How do you run the benchmark test? Both locally and on mach5.

> On May 8, 2020, at 5:28 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> currently during ZipFile creation, we create an int[] array of pointers
> to META-INF entries. These are then retrieved from three different
> places in JarFile.
> 
> However, JarFile is only interested in some combination a few things:
> the existence of and name of the META-INF/MANIFEST file, the existence
> of and the names of various signature related files, i.e., files in
> META-INF that have a suffix such as .EC, .SF, .RSA and .DSA
> 
> Refactoring the contract between JarFile and ZipFile means we can filter
> out such entries that we're not interested when opening the file, and
> also remove the need to create the String for each entries unless we
> actually want them:
> 
> Bug:https://bugs.openjdk.java.net/browse/JDK-8244624
> Webrev: http://cr.openjdk.java.net/~redestad/8244624/open.00/
> 
> This reduces retained footprint of Jar-/ZipFile by slimming down or
> removing the Source.metanames array entirely, and a significant speed-up
> in some cases.
> 
> In the provided microbenchmark, getManifestFromJarWithManifest and
> getManifestFromJarWithSignatureFiles doesn't call
> JUZFA.getMetaInfEntryNames but still see small (~5%) speed-up decrease
> in allocations.
> 
> getManifestFromJarWithNoManifest exercise JUZFA.getMetaInfEntryNames in
> the baseline, and now calls JUZFA.getManifestName. Result is a 1.25x
> speed-up and 30% reduction in allocations. While unrealistic (most JARs
> have a META-INF/MANIFEST.MF), this speed-up will translate to a few
> places - such as when loading classes from potentially-signed JAR files.
> 
> Testing: tier1-2
> 
> Thanks!
> 
> /Claes

Re: Was functional-Try already discussed?

[Justin Dekeyser]

I realized it may still not be clear! Sorry,

My dream Try would be something like this:

```
import static java.util.Try.on;
import static java.util.Try.safe;

private Bar convert (Foo foo) throws ConversionException { ... }

Stream foos;
Stream bars =
foos.map(on(ConversionException.class)::of).map(safe(this::convert)).filter(Try::notInFailure).map(Try::get);
```
This is the algebra I would like to be able to write down! Basically,
Try.on would return a monad of Try< . , ConversionException>, while the
Try.safe method sanitizes a method:
```
@FunctionalInterface interface TryFunction { V
apply(U args) throws E; }
 Function> safe (TryFunction f) { ... }
```

The Try could be enriched with many interpolability facilities, but its
essence would be to catch the provided exception.

Best regards,

Justin Dekeyser

On Fri, May 8, 2020 at 1:23 AM Justin Dekeyser 
wrote:

> Dear Dmytro,
>
> Thank you very much for the time you have spent answering my email.
> I am sorry if I was not clear in my previous sending. I will try to
> elaborate.
>
> In my various situations, I find people that are convinced that checked
> exceptions are too hard to manage, especially beacause it does not fit well
> with streams and Optional.
>
> I think a Try class, as usually defined, may be a good utilitary class to
> have in the java.utils library, in the same way Optional does.
>
> In my opinion, the covered feature is an enhancement, fills a gap in
> functional style flow suggested by stream and optional, is business
> agnostic, technology independant, figths against the anti pattern people
> adopt by defining unchecked exceptions, so for all these reasons, I think
> one can propose a solution as standard.
>
> Was the Try already discussed somewhere? What does Optional conceptually
> brings, that Try could not?
>
> Best regards,
>
> Justin Dekeyser
>
> On Thursday, May 7, 2020, dmytro sheyko 
> wrote:
>
>> Hi Justin,
>>
>> This is not quite clear what exactly you are talking about. In any case
>> why do you think it should be a part of OpenJDK? Can't it be just a
>> separate library?
>>
>> Regards,
>> Dmytro
>>
>> On Wed, May 6, 2020 at 8:31 AM Justin Dekeyser 
>> wrote:
>>
>>> Hi everyone,
>>>
>>> I am new to the OpenJDK community and I'm using Java fora small decade
>>> now,
>>> first as a hobby, then in my academic studies and my PhD in maths, and
>>> now
>>> as a professional developer in IT companies. I'm quite active on forum to
>>> help people, I've helped teaching students in the university, I read a
>>> lot
>>> of posts on blogs, and so many times I'm facing people having trouble
>>> with
>>> checked exceptions.
>>>
>>> The situation in my current job makes clarifies what I mean. People
>>> usually
>>> like declaring their own exception super class as "BusinessException",
>>> then
>>> deriving it in many subclasses to describe more accurate failure reasons.
>>> The problem they face is that when they declare their class as *checked
>>> Exception*, they cannot use the mechanism of Optional and Stream, for an
>>> obvious reason. Usually they prefer Optional and Stream, so they end up
>>> by
>>> subclassing RuntimeException.
>>>
>>> I find it so sad because, when interfacing services, you often forget
>>> about
>>> declaring those unchecked exceptions, and the client is not aware of
>>> what's
>>> happening! I think you loose all the benefit of the exception mechanism
>>> in
>>> Java here for a very bad reason.
>>>
>>> In my job I finally rectified (partially, because the code base is huge)
>>> the situation by implementing a "functional" Try in the same spirit than
>>> the Optional. People are quite happy with it. I can invest more time in
>>> its
>>> development but I think this small library could help more people about
>>> turning their exception in something clean again. So maybe it could be
>>> useful for the whole community. (I already discussed it with my boss,
>>> there
>>> will be no copyright problem.)
>>>
>>> There exists similar projects around the world (in the Vavr lib for
>>> example) and Scala offers this as a basic feature, which basically means
>>> that people could appreciate the enhancement!
>>>
>>> I am wondering if the Try interface would be interesting for OpenJDK, if
>>> they have been discussions about it, and what were the decisions about
>>> such
>>> a library? In my opinion, it may be a good complement to the Optional and
>>> the Stream interfaces to allow a functional style while keeping this cool
>>> feature of checked exceptions.
>>>
>>> Best regards,
>>>
>>> Justin Dekeyser
>>>
>>