Re: RFR JDK-8196748,tools/jar tests need to tolerate unrelated warnings

[David Holmes]

Hi Sherman,

On 10/03/2018 8:34 AM, Xueming Shen wrote:

Hi,

Please help codereview the change for JDK-8196748.

Issue: https://bugs.openjdk.java.net/browse/JDK-8196748
webrev: http://cr.openjdk.java.net/~sherman/8196748/webrev

Those tests are based on parsing the std in/err of the jar command.
It appears the "best wau" for now is to hardcode the filter to filter
out the specific jvm warning msg case by case.


Thanks for working on this and adding the new functionality to 
OutputAnalyzer, but note that:


+   private static final String jvmwarningmsg =
+   "Java HotSpot\\(TM\\) 64-Bit Server VM warning:.*";

is Oracle JDK specific and 64-bit and server VM specific! Other tests use:

Pattern.compile(".*VM warning.*")

to exclude all VM warnings. Sorry that wasn't clear from previous 
discussions.



Verified with the warning msg turned on as
http://cr.openjdk.java.net/~sherman/8196748/webrev.hs/src/hotspot/share/runtime/arguments.cpp.sdiff.html
and ran the jtreg with -XX:+FastTLABRefill.


That will not hit all cases as it requires the -XX:+FastTLABRefill to be 
passed through to all JVMs launched by tests. The original problem 
occurred with unconditional warnings that did not depend on a particular 
flag being specified on the command-line. All of those cases have now 
been fixed however so just reenabling the message doesn't achieve anything.


I tweaked the obsolete flag table to reintroduce the failure mode:

diff -r 2085742233ed src/hotspot/share/runtime/arguments.cpp
--- a/src/hotspot/share/runtime/arguments.cpp
+++ b/src/hotspot/share/runtime/arguments.cpp
@@ -516,7 +516,7 @@
   { "VMThreadHintNoPreempt",JDK_Version::jdk(11), 
JDK_Version::jdk(12), JDK_Version::jdk(13) },
   { "PrintSafepointStatistics", JDK_Version::jdk(11), 
JDK_Version::jdk(12), JDK_Version::jdk(13) },
   { "PrintSafepointStatisticsTimeout", JDK_Version::jdk(11), 
JDK_Version::jdk(12), JDK_Version::jdk(13) },
-  { "PrintSafepointStatisticsCount",JDK_Version::jdk(11), 
JDK_Version::jdk(12), JDK_Version::jdk(13) },
+  { "PrintSafepointStatisticsCount",JDK_Version::jdk(10), 
JDK_Version::jdk(11), JDK_Version::jdk(13) },


   // --- Deprecated alias flags (see also aliased_jvm_flags) - sorted 
by obsolete_in then expired_in:
   { "DefaultMaxRAMFraction",JDK_Version::jdk(8), 
JDK_Version::undefined(), JDK_Version::undefined() },

@@ -739,7 +739,7 @@
   if (!version_less_than(JDK_Version::current(), flag.obsolete_in)) {
 if (Flag::find_flag(flag.name) != NULL) {
   // Temporarily disable the warning: 8196739
-  // warning("Global variable for obsolete special flag entry 
\"%s\" should be removed", flag.name);
+  warning("Global variable for obsolete special flag entry 
\"%s\" should be removed", flag.name);

 }
   }
 }
@@ -749,7 +749,7 @@
   if (!version_less_than(JDK_Version::current(), flag.expired_in)) {
 if (Flag::find_flag(flag.name) != NULL) {
   // Temporarily disable the warning: 8196739
-  // warning("Global variable for expired flag entry \"%s\" 
should be removed", flag.name);
+  warning("Global variable for expired flag entry \"%s\" should 
be removed", flag.name);

 }
   }
 }

btw: tools/jar/LeadingGarbage.java is not failing in 11, so it is not 
touched.


It fails with my patch above - as per the original bug report.

Thanks,
David


Thanks,
Sherman

Re: RFR 8181594: Efficient and constant-time modular arithmetic

[Xuelei Fan]

On 2/26/2018 10:39 AM, Adam Petcher wrote:


http://cr.openjdk.java.net/~apetcher/8181594/webrev.01/

See inline below.

On 2/23/2018 12:46 PM, Xuelei Fan wrote:


ArrayUtil.java:
===
I'm not very sure how widely this utilities will be used in the 
future. Looks like only BigIntegerModuloP uses this classes.  I may 
prefer to define private methods for byte array swap in 
BigIntegerModuloP.


It is also used by XDHPublicKeyImpl (in the XDH code review). XDH public 
keys are represented as BigInteger, and I use the array reverse method 
to convert encoded keys to BigInteger.


If it is not widely used by other classes, please have these methods in 
the class where is get called.   The sun.security.util is exported to 
other modules as well, we may not want to add stuff into this package 
unless it is really necessary.




MutableIntegerModuloP.java
==
void conditionalSwapWith(MutableIntegerModuloP b, int swap);
As the 'swap' parameter can only be 0 or 1, could it be a boolean 
parameter?


I couldn't come up with a way to implement this without branching when 
the swap parameter is boolean. See IntegerPolynomial.conditionalSwap to 
see how this is implemented in arithmetic with an int swap argument. If 
you (or anyone) can think of a way to do this with boolean, let me know.


I added a sentence to the comment above conditionalSwapWith that 
describes why it is an int instead of a boolean.


I did not get the point about the need to avoid branching.  Can you have 
more details?




Except the conditionalSwapWith() method, I did not get the points why 
we need a mutable version.  Would you please have more description of 
this requirement?


The comment above the class definition has this sentence:

"This interface can be used to improve performance and avoid the 
allocation of a large number of temporary objects."


Do you need more information than this in the comments? The performance 
motivation is so that a.add(b).multiply(c)... can be done without 
allocating a new buffer for each operation. For example, without mutable 
field elements, an X25519 point multiplication would allocate around 
4,300 temporary arrays totaling 350,000 bytes. If I remember correctly, 
switching the X25519 implementation to mutable field elements reduced 
the point multiplication time by about half.



I see your point.  The benefits is obviously.

OK, why you need the immutable version then? Sounds like the mutable 
version interface is sufficient, including performance.  If an immutable 
version is really needed, we can have the implementation making the 
decision.  Accordingly, the conditionalSwapWith() can be defined as 
optional method, if it is not required to be implemented in immutable 
implementation.


It's confusing to me that the immutable and mutable and the base 
versions/interfaces mixed together.  It would be nice if we can simplify 
the interface a little bit.


For internal APIs, sometimes we don't want the same quality level as 
public APIs.  I think this set of class will be widely used by new EC 
curves, ChaCha20/Poly1305, or more in the future.  It would be nice if 
we could do it good at the beginning.





IntegerModuloP_Base.java

default byte[] addModPowerTwo(IntegerModuloP_Base b, int len)
void addModPowerTwo(IntegerModuloP_Base b, byte[] result);

For the first sign of the method names, I thought it is to calculate 
as "(this + b) ^ 2 mod m". 


To be precise, it calculates "((this % p) + (b % p)) % 2^m" (where p is 
the prime that defines the field, and m is the desired length, in bits). 
Note that the addition here is normal integer addition (not addition in 
GF(p)).


This operation is not used in XDH, but it is used in Poly1305 to add the 
AES encryption of a nonce to a field element. So you can get more 
information about this operation by reading the Poly1305 paper/RFC.


I was not meant to say the function of the method.  I meant that the 
method name is a little bit misleading, not very straightforward to me.



Besides, what's the benefits of the two methods?  Could we just use:
  this.add(b).asByteArray()


No, because that would calculate "((this + b) mod p) mod 2^m". The value 
of (this + b) can be larger than p, so this would not produce the 
desired result.

 >>
I guess, but not very sure, it is for constant time calculation. If 
the function is required, could it be renamed as:


  // the result is inside of the size range
  IntegerModuloP addModSize(IntegerModuloP_Base b, int size)
Or
  // the result is wrapped if outside of the size range
  IntegerModuloP addOnWrap(IntegerModuloP_Base b, int size)

and the use may look like:
  this.addModSize(b, size).asByteArray()



Any attempt to perform the addition in IntegerModuloP and then pull out 
the byte array will not work.
Does it mean if I perform a addition, and cannot get the byte array in 
the following step?

 that = this.add(b);
 byte[] 

Re: [PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup

[Peter Levart]

Hi,

On 03/02/18 18:15, Paul Sandoz wrote:

Thanks!
Paul.


On Mar 2, 2018, at 9:11 AM, Vladimir Ivanov  
wrote:



On 3/2/18 8:01 PM, Paul Sandoz wrote:

Here’s an update Ben and I tweaked:
   
http://cr.openjdk.java.net/~psandoz/jdk/buffer-reachability-fence/webrev/index.html
I think this looks good but would still like to double check with Vladimir that 
the @ForceInline is not problematic.

I confirm that my previous analysis [1] still applies when method is marked w/ 
@ForceInline.

Best regards,
Vladimir Ivanov

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



I was going to suggest to add a test for that JIT assumption, but I see 
there's already a test called ReachabilityFenceTest that should catch a 
change in JIT behavior that would break reachabilityFence(). I spotted a 
flaw in that test. See method fenced():


    public static boolean fenced() {
    AtomicBoolean finalized = new AtomicBoolean();
    MyFinalizeable o = new MyFinalizeable(finalized);

    for (int i = 0; i < LOOP_ITERS; i++) {
    if (finalized.get()) break;
    if (i > WARMUP_LOOP_ITERS) {
    System.gc();
    System.runFinalization();
    }
    }

    Reference.reachabilityFence(o);

    return finalized.get();
    }


The last two statements should be reversed or else the test could 
produce a false alarm:



        boolean fin = finalized.get();
        Reference.reachabilityFence(o);
        return fin;


Regards, Peter