Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]

[Alan Bateman]

On Tue, 13 Oct 2020 19:07:33 GMT, Volker Simonis  wrote:

>> ### Summary
>> 
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>> 
>> In general it is not safe to directly write a ZipEntry obtained from 
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
>> and write it to the `ZipOutputStream` as
>> follows:
>>  ZipEntry entry;
>>  ZipInputStream zis = new ZipInputStream(...);
>>  ZipOutputStream zos = new ZipOutputStream(...);
>>  while((entry = zis.getNextEntry()) != null) {
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> The problem with this code is that the zip file format does not record the 
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio 
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ 
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a 
>> `ZipException` with the following message:
>>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
>> got 7 bytes)
>>  
>> The correct way of copying all entries from one zip file into another 
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>>  while((entry = zis.getNextEntry()) != null) {
>>  ZipEntry newEntry = new ZipEntry(entry.getName());
>>  zos.putNextEntry(newEntry);
>>  zis.transferTo(zos);
>>  }
>> or:
>>  while((entry = zis.getNextEntry()) != null) {
>>  entry.setCompressedSize(-1);
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> Unfortunately, there's a lot of user code out there which gets this wrong 
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
>> :).  ### Description  So while this has
>> clearly been a problem before, it apparently wasn't painful enough to 
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate 
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative 
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled 
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the 
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib 
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression 
>> levels but just a zip file created with
>> another zlib version.  I'd therefore like to propose the following 
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> -  ignore the compressed size if it was implicitly determined from the zip 
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>> 
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
>> `JarOutputStream.putNextEntry()` to explain the
>>   problem and why `putNextEntry()` will ignore the compressed size of a 
>> `ZipEntry` if that was set implicitely when
>>   reading that entry from a `ZipFile` or `ZipInputStream`.
>> 
>> 
>> ### Technical Details
>> 
>> A zip file consists of a stream of File Entries followed by a Central 
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other 
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are 
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger 

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v3]

[Kim Barrett]

> Finally returning to this review that was started in April 2020.  I've
> recast it as a github PR.  I think the security concern raised by Gil
> has been adequately answered.
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
> 
> Please review a new function: java.lang.ref.Reference.refersTo.
> 
> This function is needed to test the referent of a Reference object without
> artificially extending the lifetime of the referent object, as may happen
> when calling Reference.get.  Some garbage collectors require extending the
> lifetime of a weak referent when accessed, in order to maintain collector
> invariants.  Lifetime extension may occur with any collector when the
> Reference is a SoftReference, as calling get indicates recent access.  This
> new function also allows testing the referent of a PhantomReference, which
> can't be accessed by calling get.
> 
> The new function uses native methods whose implementations are in the VM so
> they can use the Access API.  It is the intent that these methods will be
> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
> implemented yet.  Bear that in mind before rushing off to change existing
> uses of Reference.get.
> 
> There are two native methods involved, one in Reference and an override in
> PhantomReference, both package private in java.lang.ref. The reason for this
> split is to simplify the intrinsification. This is a change from the version
> from April 2020; that version had a single native method in Reference,
> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
> However, adding support for that category in the compilers adds significant
> implementation effort and complexity. Splitting avoids that complexity.
> 
> Testing:
> mach5 tier1
> Locally (linux-x64) verified the new test passes with various garbage 
> collectors.

Kim Barrett has updated the pull request incrementally with three additional 
commits since the last revision:

 - simplify test
 - cleanup nits from Mandy
 - use Object instead of TestObject

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/498/files
  - new: https://git.openjdk.java.net/jdk/pull/498/files/28f2d1b3..efa40d71

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=498=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=498=01-02

  Stats: 51 lines in 2 files changed: 13 ins; 25 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/498.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/498/head:pull/498

PR: https://git.openjdk.java.net/jdk/pull/498

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v2]

[Kim Barrett]

> On Oct 12, 2020, at 3:25 PM, Per Liden  wrote:
> 
> On Mon, 12 Oct 2020 15:56:59 GMT, Kim Barrett  wrote:
> 
>>> test/hotspot/jtreg/gc/TestReferenceRefersTo.java line 186:
>>> 
 184:
 185: expectNotValue(testWeak2, testObject3, "testWeak2");
 186: expectValue(testWeak3, testObject3, "testWeak3");
>>> 
>>> It looks to me like the `expectNotCleared()` and `expectNotValue()` methods 
>>> can be removed. All expect-tests can be
>>> done with just `expectCleared()` and `expectValue()`.
>> 
>> I don't think so.  For example,
>> `expectNotCleared(testWeak1, "testWeak1")`
>> is not testing the same thing as
>> `expectValue(testWeak1, testObject1, "testWeak1")`
>> If the implementation is correct they will indeed always be consistent. But 
>> they could be different with an incorrect
>> implementation.
> 
> Doesn't that just depend on how you implement 
> `expectValue()`/`expectCleared()`? You could for example implement
> `expectValue()`/`expectCleared()` to be supersets of 
> `expectNotCleared()`/`expectNotValue()`, which I think would make
> this easier to read. Something like this:
> 
> private static void expectValue(...) {
>if (ref.refersTo(null)) {
>fail("expected " + which + " to not be cleared");
>}
> 
>if (!ref.refersTo(value)) {
>fail(which + " refers to unexpected value");
>}
> }
> 
> private static void expectCleared(...) {
>if (ref.refersTo(new TestObject(5))) {
>fail(which + " refers to unexpected value");
>}
> 
>if (!ref.refersTo(null)) {
>fail("expected " + which + " to be cleared");
>}
> }

I didn't understand what you were previously proposing.  Now I think I see.

I've demoted expectNotValue to a helper for the other expectMumble
functions.  This allowed cleaning up the checking blocks, so each one makes
one call to an expectMumble for each test reference.

However, expectNotCleared can't be eliminated in favor of just using
expectValue.  In the places where expectNotCleared is used, the expected
value is unavailable; the associated testObjectN variable has been nulled.

Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v4]

[CoreyAshford]

On Tue, 13 Oct 2020 20:59:01 GMT, CoreyAshford 
 wrote:

> 
> > Did you already find a 2nd reviewer for the PPC64 part?
> 
> Your original comment said "2nd review", so I thought you meant you need to 
> review it again after the changes. So, no,
> I haven't looked for or found a second reviewer. Any suggestions? The folks 
> on the team here have been busy with other
> work.

I am actively asking for some help here, so maybe within a few days I can get a 
2nd reviewer.

-

PR: https://git.openjdk.java.net/jdk/pull/293

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Alexey Semenyuk]

On Tue, 13 Oct 2020 14:48:40 GMT, Andy Herrick  wrote:

>> JDK-8252870: Finalize (remove "incubator" from) jpackage
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8252870: Finalize (remove "incubator" from) jpackage
> - reverting two auto-generated files, and changing module-info to "@since 
> 16"

Marked as reviewed by asemenyuk (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: 8247666: Support Lambda proxy classes in static CDS archive [v5]

[Mandy Chung]

On Wed, 14 Oct 2020 00:40:14 GMT, Ioi Lam  wrote:

>> Calvin Cheung has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now
>> contains ten commits:
>>  - fix minimal vm build issues
>>  - Merge branch 'master' into 8247666
>>  - Merge branch 'master' into 8247666
>>  - 1. Symbolic encoding of lambda proxy resolution. An example of 
>> @lambda-proxy in the classlist:
>>@lambda-proxy: LambHello run ()Ljava/lang/Runnable; ()V 6 LambHello 
>> lambdabash ()V ()V
>>2. Removed BadCPIndex.java; added LambdaProxyClassList.java test.
>>3. Mandy's comments.
>>  - Merge branch 'master' into 8247666
>>  - exclude archived lambda proxy classes in the classlist
>>  - updated systemDictionaryShared.[c|h]pp based on suggestions from Ioi
>>  - fix extraneous whitespace
>>  - 8247666 (initial commit)
>
> Changes requested by iklam (Reviewer).

>  `@lambda-proxy: LambHello run ()Ljava/lang/Runnable; ()V 6 LambHello 
> lambda$main$0 ()V ()V`

Can you explain the format?

-

PR: https://git.openjdk.java.net/jdk/pull/364

Re: RFR: 8223347: Integration of Vector API (Incubator) [v4]

[Ekaterina Pavlova]

On Wed, 14 Oct 2020 00:34:04 GMT, Sandhya Viswanathan 
 wrote:

>> There are several gc tests crashed in panama-vector tier3 testing which 
>> seems are not observed in openjdk repo.
>> The crashes look like:
>> #  assert(oopDesc::is_oop(obj)) failed: not an oop: 0xfff1
>> #
>> # JRE version: Java(TM) SE Runtime Environment (16.0+3) (fastdebug build 
>> 16-panama+3-216)
>> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 16-panama+3-216, 
>> mixed mode, sharing, tiered, compressed oops,
>> g1 gc, linux-amd64) # Problematic frame:
>> # V  [libjvm.so+0xd8ef94]  HandleArea::allocate_handle(oop)+0x144
>> 
>> and the issue is actually tracked by JDK-8233199.
>> 
>> This issue needs to be at least analyzed before integrating Vector API.
>
> @katyapav Is the failure observed on vector-unstable branch of panama-vector?
> The code in this pull request is from vector-unstable branch.
> The bug report https://bugs.openjdk.java.net/browse/JDK-8233199 refers to 
> repo-valhalla and not
> panama-vector:vector-unstable. @PaulSandoz is doing final testing of the pull 
> request today before integration tomorrow
> hopefully.

@sviswa7 you are right, the failure is observed on vector-unstable branch of 
panama-vector.
I referred to JDK-8233199 because it seems both panama-vector and valhalla-repo 
have the same issue/crash.
@PaulSandoz also mentioned that panama-vector was not in sync with master and 
this is perhaps the issue is in
vector-unstable. He said that he tested the PR separately and didn't observe 
this issue in the PR.

-

PR: https://git.openjdk.java.net/jdk/pull/367

Re: RFR: 8247666: Support Lambda proxy classes in static CDS archive [v5]

[Ioi Lam]

On Tue, 13 Oct 2020 23:08:22 GMT, Calvin Cheung  wrote:

>> Following up on archiving lambda proxy classes in dynamic CDS archive
>> ([JDK-8198698](https://bugs.openjdk.java.net/browse/JDK-8198698)), this RFE 
>> adds the functionality of archiving of
>> lambda proxy classes in static CDS archive.
>> When the -XX:DumpLoadedClassList is enabled, the constant pool index related 
>> to LambdaMetafactory that are resolved
>> during application execution will be included in the classlist. The entry 
>> for a lambda proxy class in a class list will
>> be of the following format:
>> `@lambda-proxy:  `
>> 
>> e.g.
>> `@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 233`
>> `@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 355`
>> 
>> When dumping a CDS archive using the -Xshare:dump and 
>> -XX:ExtraSharedClassListFile options, when the above
>> `@lambda-proxy` entry is encountered while parsing the classlist, we will 
>> resolve the corresponding constant pool
>> indices (233 and 355 in the above example). As a result, lambda proxy 
>> classes will be generated for the constant pool
>> entries, and will be cached using a similar mechanism to JDK-8198698.  
>> During dumping, there is check on the cp index
>> and on the created BootstrapInfo using the cp index. VM will exit with an 
>> error message if the check has failed.
>> During runtime when looking up a lambda proxy class, the lookup will be 
>> perform on the static CDS archive and if not
>> found, then lookup from the dynamic archive if one is specified.  (Only name 
>> change (IsDynamicDumpingEnabled ->
>> IsCDSDumpingEnabled) is involved in the core-libs code.)
>> Testing: tiers 1,2,3,4.
>> 
>> Performance results (javac on HelloWorld on linux-x64):
>> Results of " perf stat -r 40 bin/javac -J-Xshare:on 
>> -J-XX:SharedArchiveFile=javac2.jsa Bench_HelloWorld.java "
>>1:   2228016795  2067752708 (-160264087)  -377.760   349.110 
>> (-28.650)  -
>>2:   2223051476  2063016483 (-160034993)  -374.580   350.620 
>> (-23.960)  
>>3:   2225908334  2067673847 (-158234487)  -375.220   350.990 
>> (-24.230)  
>>4:   2225835999  2064596883 (-161239116)  -374.670   349.840 
>> (-24.830)  
>>5:   2226005510  2061694332 (-164311178)  -373.512   351.120 
>> (-22.392)  
>>6:   2225574949  2062657482 (-162917467)  -374.710   348.380 
>> (-26.330)  -
>>7:   2224702424  2064634122 (-160068302)  -373.670   349.510 
>> (-24.160)  
>>8:   2226662277  2066301134 (-160361143)  -375.350   349.790 
>> (-25.560)  
>>9:   2226761470  2063162795 (-163598675)  -374.260   351.290 
>> (-22.970)  
>>   10:   2230149089  2066203307 (-163945782)  -374.760   350.620 
>> (-24.140)  
>> 
>> 2226266109  2064768307 (-161497801)  -374.848   350.126 
>> (-24.722)  
>> instr delta =   -161497801-7.2542%
>> time  delta =  -24.722 ms -6.5951%
>
> Calvin Cheung has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now
> contains ten commits:
>  - fix minimal vm build issues
>  - Merge branch 'master' into 8247666
>  - Merge branch 'master' into 8247666
>  - 1. Symbolic encoding of lambda proxy resolution. An example of 
> @lambda-proxy in the classlist:
>@lambda-proxy: LambHello run ()Ljava/lang/Runnable; ()V 6 LambHello 
> lambdabash ()V ()V
>2. Removed BadCPIndex.java; added LambdaProxyClassList.java test.
>3. Mandy's comments.
>  - Merge branch 'master' into 8247666
>  - exclude archived lambda proxy classes in the classlist
>  - updated systemDictionaryShared.[c|h]pp based on suggestions from Ioi
>  - fix extraneous whitespace
>  - 8247666 (initial commit)

Changes requested by iklam (Reviewer).

src/hotspot/share/interpreter/linkResolver.cpp line 34:

> 32: #include "classfile/symbolTable.hpp"
> 33: #include "classfile/systemDictionary.hpp"
> 34: #include "classfile/systemDictionaryShared.hpp"

Are all the new includes necessary?

src/hotspot/share/memory/archiveUtils.cpp line 321:

> 319: }
> 320:   }
> 321: }

I think if two threads try call ArchiveUtils::log_to_classlist at the same 
time, the output may be interleaved.

classlist_file is a fileStream, which uses fwrite to write to the file. In 
theory, if you write the whole line at once,
the output should be thread safe (at least on POSIX and Windows). But in your 
case, you would need to first get the
whole line into a buffer, and then write it out at once.

I think it would be safer, and more convenient, to use a lock to ensure thready 
safety. Maybe we can convert all uses
of classlist_file->print() to something like

class ClassListWriter {
static fileStream* _classlist_file;
MutexLocker locker;
public:
   

Re: RFR: 8223347: Integration of Vector API (Incubator) [v4]

[Sandhya Viswanathan]

On Tue, 13 Oct 2020 21:29:52 GMT, Ekaterina Pavlova 
 wrote:

>> Build changes look good.
>
> There are several gc tests crashed in panama-vector tier3 testing which seems 
> are not observed in openjdk repo.
> The crashes look like:
> #  assert(oopDesc::is_oop(obj)) failed: not an oop: 0xfff1
> #
> # JRE version: Java(TM) SE Runtime Environment (16.0+3) (fastdebug build 
> 16-panama+3-216)
> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 16-panama+3-216, 
> mixed mode, sharing, tiered, compressed oops,
> g1 gc, linux-amd64) # Problematic frame:
> # V  [libjvm.so+0xd8ef94]  HandleArea::allocate_handle(oop)+0x144
> 
> and the issue is actually tracked by JDK-8233199.
> 
> This issue needs to be at least analyzed before integrating Vector API.

@katyapav Is the failure observed on vector-unstable branch of panama-vector?
The code in this pull request is from vector-unstable branch.
The bug report https://bugs.openjdk.java.net/browse/JDK-8233199 refers to 
repo-valhalla and not
panama-vector:vector-unstable. @PaulSandoz is doing final testing of the pull 
request today before integration tomorrow
hopefully.

-

PR: https://git.openjdk.java.net/jdk/pull/367

Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v10]

[Vladimir Kozlov]

On Sat, 10 Oct 2020 08:34:23 GMT, Richard Reingruber  wrote:

>> Hi,
>> 
>> this is the continuation of the review of the implementation for:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8227745
>> https://bugs.openjdk.java.net/browse/JDK-8233915
>> 
>> It allows for JIT optimizations based on escape analysis even if JVMTI 
>> agents acquire capabilities to access references
>> to objects that are subject to such optimizations, e.g. scalar replacement. 
>> The implementation reverts such
>> optimizations just before access very much as when switching from JIT 
>> compiled execution to the interpreter, aka
>> "deoptimization".  Webrev.8 was the last one before before the transition to 
>> Git/Github:
>> 
>> http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/
>> 
>> Thanks, Richard.
>
> Richard Reingruber has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now
> contains 21 commits:
>  - The constructor of StackFrameStream takes more parameters after JDK-8253180
>  - Merge branch 'master' into JDK-8227745
>  - Merge branch 'master' into JDK-8227745
>  - Merge branch 'master' into JDK-8227745
>  - Factorized fragment out of EscapeBarrier::deoptimize_objects_internal into 
> new method in compiledVFrame.
>  - More smaller changes proposed by Serguei.
>  - jvmtiDeferredUpdates.hpp: remove forward declarations.
>  - jvmtiDeferredLocalVariable: move member variables to the beginning of the 
> class definition.
>  - jvmtiDeferredUpdates.hpp: add/remove empty lines and improve indentation.
>  - Merge branch 'master' into JDK-8227745
>  - ... and 11 more: 
> https://git.openjdk.java.net/jdk/compare/aaa0a2a0...06b139a9

Good.

-

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/119

Re: RFR: 8251989: Hex formatting and parsing utility [v2]

[Tagir F . Valeev]

On Tue, 13 Oct 2020 19:51:30 GMT, Roger Riggs  wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> Review comments and suggestions welcome.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Various code review comments, rename UpperCase and LowerCase methods to 
> match Character, remove unnecessary Class name
>   qualifications, etc.

Marked as reviewed by tvaleev (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Andy Herrick]

On Tue, 13 Oct 2020 23:28:24 GMT, Alexander Matveev  
wrote:

>> Andy Herrick has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   JDK-8252870: Finalize (remove "incubator" from) jpackage
>> - reverting two auto-generated files, and changing module-info to 
>> "@since 16"
>
> Marked as reviewed by almatvee (Committer).

>  src/jdk.jpackage/macosx/classes/module-info.java.extra
> jdk.jpackage.internal.MacAppBundler,
> jdk.jpackage.internal.MacAppStoreBundler,
not related to this change directly, but even in 14 there was no 
MacAppStoreBundler

-

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Alexander Matveev]

On Tue, 13 Oct 2020 14:48:40 GMT, Andy Herrick  wrote:

>> JDK-8252870: Finalize (remove "incubator" from) jpackage
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8252870: Finalize (remove "incubator" from) jpackage
> - reverting two auto-generated files, and changing module-info to "@since 
> 16"

Marked as reviewed by almatvee (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Kevin Rushforth]

On Tue, 13 Oct 2020 14:48:40 GMT, Andy Herrick  wrote:

>> JDK-8252870: Finalize (remove "incubator" from) jpackage
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8252870: Finalize (remove "incubator" from) jpackage
> - reverting two auto-generated files, and changing module-info to "@since 
> 16"

Looks good. I double-checked all of the file renames and all are exactly as 
expected. The diffs got a bit confused in a
couple cases, where it thought that a renamed and modified file was copied from 
somewhere else (see inline comments),
but that can happen given that git doesn't actually track renames and copies).

-

Marked as reviewed by kcr (Author).

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Kevin Rushforth]

On Tue, 13 Oct 2020 21:30:05 GMT, Alexander Matveev  
wrote:

>> Andy Herrick has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   JDK-8252870: Finalize (remove "incubator" from) jpackage
>> - reverting two auto-generated files, and changing module-info to 
>> "@since 16"
>
> src/jdk.jpackage/linux/classes/module-info.java.extra line 29:
> 
>> 27: jdk.jpackage.internal.LinuxAppBundler,
>> 28: jdk.jpackage.internal.LinuxDebBundler,
>> 29: jdk.jpackage.internal.LinuxRpmBundler;
> 
> Not sure why it was changed. Looks like git recorded file renaming 
> incorrectly.

Yes, sometime GIt / Github get a bit confused about identifying rename/copy 
when generating a diff for a renamed file
with changes. The result is correct, but the diff looks odd.

> src/jdk.jpackage/macosx/classes/module-info.java.extra line 30:
> 
>> 28: jdk.jpackage.internal.MacAppStoreBundler,
>> 29: jdk.jpackage.internal.MacDmgBundler,
>> 30: jdk.jpackage.internal.MacPkgBundler;
> 
> Looks like another rename issue.

Yes (or more precisely another issue displaying the diffs for a renamed file 
with changes).

-

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: 8247666: Support Lambda proxy classes in static CDS archive [v5]

[Calvin Cheung]

> Following up on archiving lambda proxy classes in dynamic CDS archive
> ([JDK-8198698](https://bugs.openjdk.java.net/browse/JDK-8198698)), this RFE 
> adds the functionality of archiving of
> lambda proxy classes in static CDS archive.
> When the -XX:DumpLoadedClassList is enabled, the constant pool index related 
> to LambdaMetafactory that are resolved
> during application execution will be included in the classlist. The entry for 
> a lambda proxy class in a class list will
> be of the following format:
> `@lambda-proxy:  `
> 
> e.g.
> `@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 233`
> `@lambda-proxy: test/java/lang/invoke/MethodHandlesGeneralTest 355`
> 
> When dumping a CDS archive using the -Xshare:dump and 
> -XX:ExtraSharedClassListFile options, when the above
> `@lambda-proxy` entry is encountered while parsing the classlist, we will 
> resolve the corresponding constant pool
> indices (233 and 355 in the above example). As a result, lambda proxy classes 
> will be generated for the constant pool
> entries, and will be cached using a similar mechanism to JDK-8198698.  During 
> dumping, there is check on the cp index
> and on the created BootstrapInfo using the cp index. VM will exit with an 
> error message if the check has failed.
> During runtime when looking up a lambda proxy class, the lookup will be 
> perform on the static CDS archive and if not
> found, then lookup from the dynamic archive if one is specified.  (Only name 
> change (IsDynamicDumpingEnabled ->
> IsCDSDumpingEnabled) is involved in the core-libs code.)
> Testing: tiers 1,2,3,4.
> 
> Performance results (javac on HelloWorld on linux-x64):
> Results of " perf stat -r 40 bin/javac -J-Xshare:on 
> -J-XX:SharedArchiveFile=javac2.jsa Bench_HelloWorld.java "
>1:   2228016795  2067752708 (-160264087)  -377.760   349.110 
> (-28.650)  -
>2:   2223051476  2063016483 (-160034993)  -374.580   350.620 
> (-23.960)  
>3:   2225908334  2067673847 (-158234487)  -375.220   350.990 
> (-24.230)  
>4:   2225835999  2064596883 (-161239116)  -374.670   349.840 
> (-24.830)  
>5:   2226005510  2061694332 (-164311178)  -373.512   351.120 
> (-22.392)  
>6:   2225574949  2062657482 (-162917467)  -374.710   348.380 
> (-26.330)  -
>7:   2224702424  2064634122 (-160068302)  -373.670   349.510 
> (-24.160)  
>8:   2226662277  2066301134 (-160361143)  -375.350   349.790 
> (-25.560)  
>9:   2226761470  2063162795 (-163598675)  -374.260   351.290 
> (-22.970)  
>   10:   2230149089  2066203307 (-163945782)  -374.760   350.620 
> (-24.140)  
> 
> 2226266109  2064768307 (-161497801)  -374.848   350.126 
> (-24.722)  
> instr delta =   -161497801-7.2542%
> time  delta =  -24.722 ms -6.5951%

Calvin Cheung has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now
contains ten commits:

 - fix minimal vm build issues
 - Merge branch 'master' into 8247666
 - Merge branch 'master' into 8247666
 - 1. Symbolic encoding of lambda proxy resolution. An example of @lambda-proxy 
in the classlist:
   @lambda-proxy: LambHello run ()Ljava/lang/Runnable; ()V 6 LambHello 
lambdabash ()V ()V
   2. Removed BadCPIndex.java; added LambdaProxyClassList.java test.
   3. Mandy's comments.
 - Merge branch 'master' into 8247666
 - exclude archived lambda proxy classes in the classlist
 - updated systemDictionaryShared.[c|h]pp based on suggestions from Ioi
 - fix extraneous whitespace
 - 8247666 (initial commit)

-

Changes: https://git.openjdk.java.net/jdk/pull/364/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=364=04
  Stats: 1839 lines in 32 files changed: 1767 ins; 19 del; 53 mod
  Patch: https://git.openjdk.java.net/jdk/pull/364.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/364/head:pull/364

PR: https://git.openjdk.java.net/jdk/pull/364

Re: RFR: 8223347: Integration of Vector API (Incubator) [v4]

[Ekaterina Pavlova]

On Mon, 12 Oct 2020 12:56:10 GMT, Erik Joelsson  wrote:

>> Paul Sandoz has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now
>> contains ten commits:
>>  - Merge master
>>  - Fix related to merge
>>  - HotspotIntrinsicCandidate to IntrinsicCandidate
>>  - Merge master
>>  - Fix permissions
>>  - Fix permissions
>>  - Merge master
>>  - Vector API new files
>>  - Integration of Vector API (Incubator)
>
> Build changes look good.

There are several gc tests crashed in panama-vector tier3 testing which seems 
are not observed in openjdk repo.
The crashes look like:
#  assert(oopDesc::is_oop(obj)) failed: not an oop: 0xfff1
#
# JRE version: Java(TM) SE Runtime Environment (16.0+3) (fastdebug build 
16-panama+3-216)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 16-panama+3-216, mixed 
mode, sharing, tiered, compressed oops,
g1 gc, linux-amd64) # Problematic frame:
# V  [libjvm.so+0xd8ef94]  HandleArea::allocate_handle(oop)+0x144

and the issue is actually tracked by JDK-8233199.

This issue needs to be at least analyzed before integrating Vector API.

-

PR: https://git.openjdk.java.net/jdk/pull/367

Re: RFR: 8251989: Hex formatting and parsing utility [v2]

[Marcono1234]

On Tue, 13 Oct 2020 16:24:32 GMT, Roger Riggs  wrote:

>> src/java.base/share/classes/java/util/HexFormat.java line 1015:
>> 
>>> 1013:  */
>>> 1014: @Override
>>> 1015: public String toString() {
>> 
>> Might be useful to also include the class name?
>
> The caller may need to provide their own context for the output.

I was thinking of the case where the caller invokes it by accident (due to 
incorrect code, e.g. `"..." + hexFormat`
instead of `"..." + hexFormat.toHexDigits(...)`) and then wonders what 
"uppercase: true, ..." means or where it even
comes from. For example `java.util.Optional` also includes "Optional[...]".

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Alexander Matveev]

On Tue, 13 Oct 2020 14:48:40 GMT, Andy Herrick  wrote:

>> JDK-8252870: Finalize (remove "incubator" from) jpackage
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8252870: Finalize (remove "incubator" from) jpackage
> - reverting two auto-generated files, and changing module-info to "@since 
> 16"

src/jdk.jpackage/linux/classes/module-info.java.extra line 29:

> 27: jdk.jpackage.internal.LinuxAppBundler,
> 28: jdk.jpackage.internal.LinuxDebBundler,
> 29: jdk.jpackage.internal.LinuxRpmBundler;

Not sure why it was changed. Looks like git recorded file renaming incorrectly.

src/jdk.jpackage/macosx/classes/module-info.java.extra line 30:

> 28: jdk.jpackage.internal.MacAppStoreBundler,
> 29: jdk.jpackage.internal.MacDmgBundler,
> 30: jdk.jpackage.internal.MacPkgBundler;

Looks like another rename issue.

src/jdk.jpackage/windows/classes/module-info.java.extra line 29:

> 27: jdk.jpackage.internal.WinAppBundler,
> 28: jdk.jpackage.internal.WinExeBundler,
> 29: jdk.jpackage.internal.WinMsiBundler;

Another rename issue.

-

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: 8251989: Hex formatting and parsing utility [v2]

[Marcono1234]

On Tue, 13 Oct 2020 19:51:30 GMT, Roger Riggs  wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> Review comments and suggestions welcome.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Various code review comments, rename UpperCase and LowerCase methods to 
> match Character, remove unnecessary Class name
>   qualifications, etc.

test/jdk/java/util/HexFormat/HexFormatTest.java line 383:

> 381:
> 382: @Test
> 383: static void testVariableLength() {

This test does not contain any assertions.

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: 8251989: Hex formatting and parsing utility [v2]

[Marcono1234]

On Tue, 13 Oct 2020 16:15:51 GMT, Roger Riggs  wrote:

>> test/jdk/java/util/HexFormat/HexFormatTest.java line 406:
>> 
>>> 404:
>>> 405: int byteVal = hex.fromHexDigits(byteStr);
>>> 406: assert(byteStr.equals("7f"));
>> 
>> Why use regular `assert` statements in this test method? TestNG's methods 
>> likely yield more useful results.
>
> These code fragments are the examples used in the javadoc; making a simple 
> copy/paste from working code to the javadoc.
> In the example code `assert`, a Java language construct is cleaner than 
> showing testng assertions).

Yes using test framework specific code in the example code is probably not a 
good idea and the `assert` statement there
makes the intention clear.

However, for unit tests I assume it can be a little bit problematic if the 
tests are compiled with assertions disabled.
Additionally if one of the `assert` statements fail you would only get an 
`AssertionError` without much additional
feedback, while test framework assertion methods usually provide pretty 
detailed messages and have good IDE support.
For example `assertEquals("a", "b")` would yield something like
> Expected "a" but was "b"

which makes it obvious what the issue is.

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v4]

[CoreyAshford]

On Tue, 13 Oct 2020 19:56:42 GMT, Martin Doerr  wrote:

> Hi Corey, thanks for taking some stuff out of the “too short” path. There may 
> be a performance regression when decoding
> many short arrays because of the stub call overhead and the usage of the 
> slower part of the Java implementation. We
> could do it a little better in many cases to compute the maximum possible 
> iteration count i: i = (sl - sp) / block_size
> if (i * block_size > sl - 12) i-- if (i <= 0) return 0 What do you think?

Are you thinking of a case where that produces a higher iteration count?  It 
looks effectively the same to me.

> I don’t think branch prediction hints are helpful for the “too short” check.

My thinking is that most of the time when the intrinsic is called, it will not 
take the early exit, but I suppose when
it is processing a sub-block_size buffer, it will return early every time.  I 
will remove the hints.

> And we should better use CCR1 instead of CCR2 which is specified as 
> non-volatile.

Ah, I should have checked the calling conventions.  I thought all of the CR* 
regs are volatile.  I will fix that.

>  Did you already find a 2nd reviewer for the PPC64 part?

Your original comment said "2nd review", so I thought you meant you need to 
review it again after the changes.  So, no,
I haven't looked for or found a second reviewer.  Any suggestions?  The folks 
on the team here have been busy with
other work.

Btw, I'm off today, so I will push commits to the above-mentioned issues 
tomorrow.

-

PR: https://git.openjdk.java.net/jdk/pull/293

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]

[Lance Andersen]

On Tue, 13 Oct 2020 19:07:33 GMT, Volker Simonis  wrote:

>> ### Summary
>> 
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>> 
>> In general it is not safe to directly write a ZipEntry obtained from 
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
>> and write it to the `ZipOutputStream` as
>> follows:
>>  ZipEntry entry;
>>  ZipInputStream zis = new ZipInputStream(...);
>>  ZipOutputStream zos = new ZipOutputStream(...);
>>  while((entry = zis.getNextEntry()) != null) {
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> The problem with this code is that the zip file format does not record the 
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio 
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ 
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a 
>> `ZipException` with the following message:
>>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
>> got 7 bytes)
>>  
>> The correct way of copying all entries from one zip file into another 
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>>  while((entry = zis.getNextEntry()) != null) {
>>  ZipEntry newEntry = new ZipEntry(entry.getName());
>>  zos.putNextEntry(newEntry);
>>  zis.transferTo(zos);
>>  }
>> or:
>>  while((entry = zis.getNextEntry()) != null) {
>>  entry.setCompressedSize(-1);
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> Unfortunately, there's a lot of user code out there which gets this wrong 
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
>> :).  ### Description  So while this has
>> clearly been a problem before, it apparently wasn't painful enough to 
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate 
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative 
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled 
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the 
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib 
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression 
>> levels but just a zip file created with
>> another zlib version.  I'd therefore like to propose the following 
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> -  ignore the compressed size if it was implicitly determined from the zip 
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>> 
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
>> `JarOutputStream.putNextEntry()` to explain the
>>   problem and why `putNextEntry()` will ignore the compressed size of a 
>> `ZipEntry` if that was set implicitely when
>>   reading that entry from a `ZipFile` or `ZipInputStream`.
>> 
>> 
>> ### Technical Details
>> 
>> A zip file consists of a stream of File Entries followed by a Central 
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other 
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are 
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger 

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]

[Lance Andersen]

On Tue, 13 Oct 2020 19:07:33 GMT, Volker Simonis  wrote:

>> ### Summary
>> 
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>> 
>> In general it is not safe to directly write a ZipEntry obtained from 
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
>> and write it to the `ZipOutputStream` as
>> follows:
>>  ZipEntry entry;
>>  ZipInputStream zis = new ZipInputStream(...);
>>  ZipOutputStream zos = new ZipOutputStream(...);
>>  while((entry = zis.getNextEntry()) != null) {
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> The problem with this code is that the zip file format does not record the 
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio 
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ 
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a 
>> `ZipException` with the following message:
>>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
>> got 7 bytes)
>>  
>> The correct way of copying all entries from one zip file into another 
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>>  while((entry = zis.getNextEntry()) != null) {
>>  ZipEntry newEntry = new ZipEntry(entry.getName());
>>  zos.putNextEntry(newEntry);
>>  zis.transferTo(zos);
>>  }
>> or:
>>  while((entry = zis.getNextEntry()) != null) {
>>  entry.setCompressedSize(-1);
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> Unfortunately, there's a lot of user code out there which gets this wrong 
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
>> :).  ### Description  So while this has
>> clearly been a problem before, it apparently wasn't painful enough to 
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate 
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative 
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled 
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the 
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib 
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression 
>> levels but just a zip file created with
>> another zlib version.  I'd therefore like to propose the following 
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> -  ignore the compressed size if it was implicitly determined from the zip 
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>> 
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
>> `JarOutputStream.putNextEntry()` to explain the
>>   problem and why `putNextEntry()` will ignore the compressed size of a 
>> `ZipEntry` if that was set implicitely when
>>   reading that entry from a `ZipFile` or `ZipInputStream`.
>> 
>> 
>> ### Technical Details
>> 
>> A zip file consists of a stream of File Entries followed by a Central 
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other 
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are 
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger 

Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v4]

[Martin Doerr]

On Mon, 12 Oct 2020 22:00:24 GMT, CoreyAshford 
 wrote:

>> This latest push passes the intrinsic regression test. I had run the 
>> intrinsic TestBase64 regression test on the
>> previous push, but not the one in utils.  Interesting.  Somehow it didn't 
>> occur to me that there could be a problem
>> there if the intrinsic TestBase64 test passed.  I will check into the other 
>> regression test.  Don't review this latest
>> push just yet.
>
> Ok, all is clear.  I just ran `jdk/java/util/Base64/TestBase64.java` which 
> passes as well.  Please review again when
> convenient.

Hi Corey,

thanks for taking some stuff out of the “too short” path.
There may be a performance regression when decoding many short arrays because 
of the stub call overhead and the usage
of the slower part of the Java implementation.

We could do it a little better in many cases to compute the maximum possible 
iteration count i:
i = (sl - sp) / block_size
if (i * block_size > sl - 12) i--
if (i <= 0) return 0
What do you think?

I don’t think branch prediction hints are helpful for the “too short” check.

And we should better use CCR1 instead of CCR2 which is specified as 
non-volatile.

Did you already find a 2nd reviewer for the PPC64 part?

Best regards,
Martin



From: CoreyAshford 
Sent: Dienstag, 13. Oktober 2020 00:01
To: openjdk/jdk 
Cc: Doerr, Martin ; Mention 
Subject: Re: [openjdk/jdk] 8248188: Add IntrinsicCandidate and API for Base64 
decoding (#293)


Ok, all is clear. I just ran jdk/java/util/Base64/TestBase64.java which passes 
as well. Please review again when
convenient.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on 
GitHub, or
unsubscribe.

-

PR: https://git.openjdk.java.net/jdk/pull/293

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]

[Lance Andersen]

On Tue, 13 Oct 2020 19:07:33 GMT, Volker Simonis  wrote:

>> ### Summary
>> 
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>> 
>> In general it is not safe to directly write a ZipEntry obtained from 
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
>> and write it to the `ZipOutputStream` as
>> follows:
>>  ZipEntry entry;
>>  ZipInputStream zis = new ZipInputStream(...);
>>  ZipOutputStream zos = new ZipOutputStream(...);
>>  while((entry = zis.getNextEntry()) != null) {
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> The problem with this code is that the zip file format does not record the 
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio 
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ 
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a 
>> `ZipException` with the following message:
>>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
>> got 7 bytes)
>>  
>> The correct way of copying all entries from one zip file into another 
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>>  while((entry = zis.getNextEntry()) != null) {
>>  ZipEntry newEntry = new ZipEntry(entry.getName());
>>  zos.putNextEntry(newEntry);
>>  zis.transferTo(zos);
>>  }
>> or:
>>  while((entry = zis.getNextEntry()) != null) {
>>  entry.setCompressedSize(-1);
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> Unfortunately, there's a lot of user code out there which gets this wrong 
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
>> :).  ### Description  So while this has
>> clearly been a problem before, it apparently wasn't painful enough to 
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate 
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative 
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled 
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the 
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib 
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression 
>> levels but just a zip file created with
>> another zlib version.  I'd therefore like to propose the following 
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> -  ignore the compressed size if it was implicitly determined from the zip 
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>> 
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
>> `JarOutputStream.putNextEntry()` to explain the
>>   problem and why `putNextEntry()` will ignore the compressed size of a 
>> `ZipEntry` if that was set implicitely when
>>   reading that entry from a `ZipFile` or `ZipInputStream`.
>> 
>> 
>> ### Technical Details
>> 
>> A zip file consists of a stream of File Entries followed by a Central 
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other 
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are 
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger 

Re: RFR: 8251989: Hex formatting and parsing utility [v2]

[Roger Riggs]

> java.util.HexFormat utility:
> 
>  - Format and parse hexadecimal strings, with parameters for delimiter, 
> prefix, suffix and upper/lowercase
>  - Static factories and builder methods to create HexFormat copies with 
> modified parameters.
>  - Consistent naming of methods for conversion of byte arrays to formatted 
> strings and back: formatHex and parseHex
>  - Consistent naming of methods for conversion of primitive types: 
> toHexDigits... and fromHexDigits...
>  - Prefix and suffixes now apply to each formatted value, not the string as a 
> whole
>  - Using java.util.Appendable as a target for buffered conversions so output 
> to Writers and PrintStreams
>like System.out are supported in addition to StringBuilder. (IOExceptions 
> are converted to unchecked exceptions)
>  - Immutable and thread safe, a "value-based" class
> 
> See the [HexFormat
> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>  for details.
> Review comments and suggestions welcome.

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Various code review comments, rename UpperCase and LowerCase methods to match 
Character, remove unnecessary Class name
  qualifications, etc.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/482/files
  - new: https://git.openjdk.java.net/jdk/pull/482/files/c1bb4514..3170b498

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=482=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=482=00-01

  Stats: 61 lines in 9 files changed: 5 ins; 2 del; 54 mod
  Patch: https://git.openjdk.java.net/jdk/pull/482.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/482/head:pull/482

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]

[Volker Simonis]

On Tue, 13 Oct 2020 17:16:21 GMT, Lance Andersen  wrote:

>> Volker Simonis has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev
>> excludes the unrelated changes brought in by the merge/rebase.
>
> test/jdk/java/util/zip/CopyZipFile.java line 149:
> 
>> 147: os = new ByteArrayOutputStream();
>> 148: zos = new ZipOutputStream(os);
>> 149: ZipFile zf = new ZipFile(ZIP_FILE);
> 
> Any reason to not use try with resources here as well (and below)

Done

-

PR: https://git.openjdk.java.net/jdk/pull/520

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]

[Volker Simonis]

On Tue, 13 Oct 2020 18:50:00 GMT, Lance Andersen  wrote:

>> src/java.base/share/classes/java/util/jar/JarOutputStream.java line 87:
>> 
>>> 85:  * 
>>> 86:  * The current time will be used if the entry has no set 
>>> modification
>>> 87:  * time.
>> 
>> I'm happy with the wording. What you think about put a p tag before "The 
>> default compression method ..." so that it
>> goes into its own paragraph? I'm asking because it's inconsistent to have 
>> the first paragraph include the details on
>> the compression method and the details on the current time pushed into a 
>> second paragraph - if you generate the javadoc
>> then you'll see what I mean.
>
> I think that is a good idea to at the paragraph tag as you suggest

Done

-

PR: https://git.openjdk.java.net/jdk/pull/520

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]

[Volker Simonis]

> ### Summary
> 
> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
> which can lead to the `ZipException "invalid
> entry compressed size"`.
> ### Motivation
> 
> In general it is not safe to directly write a ZipEntry obtained from 
> `ZipInputStream.getNextEntry()`,
> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
> `ZipOutputStream.putNextEntry()` to a
> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
> and write it to the `ZipOutputStream` as
> follows:
>  ZipEntry entry;
>  ZipInputStream zis = new ZipInputStream(...);
>  ZipOutputStream zos = new ZipOutputStream(...);
>  while((entry = zis.getNextEntry()) != null) {
>  zos.putNextEntry(entry);
>  zis.transferTo(zos);
>  }
> The problem with this code is that the zip file format does not record the 
> compression level used for deflation in its
> entries. In general, it doesn't even mandate a predefined compression ratio 
> per compression level. Therefore the
> compressed size recorded in a `ZipEntry` read from a zip file might differ 
> from the new compressed size produced by the
> receiving `ZipOutputStream`. Such a difference will result in a 
> `ZipException` with the following message:
>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
> got 7 bytes)
>  
> The correct way of copying all entries from one zip file into another 
> requires the creation of a new `ZipEntry` or at
> least resetting of the compressed size field. E.g.:
>  while((entry = zis.getNextEntry()) != null) {
>  ZipEntry newEntry = new ZipEntry(entry.getName());
>  zos.putNextEntry(newEntry);
>  zis.transferTo(zos);
>  }
> or:
>  while((entry = zis.getNextEntry()) != null) {
>  entry.setCompressedSize(-1);
>  zos.putNextEntry(entry);
>  zis.transferTo(zos);
>  }
> Unfortunately, there's a lot of user code out there which gets this wrong and 
> uses the bad coding pattern described
> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
> size (expected 12 but got 7 bytes)"` gives
> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
> instances of this anti-pattern on GitHub when
> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
> wrapper task][1] is affected as well as the
> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
> occurrences of this pattern in OpenJDK (see
> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
> (e.g.
> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
> :).  ### Description  So while this has
> clearly been a problem before, it apparently wasn't painful enough to trigger 
> any action from the side of the JDK.
> However, recently quite some zlib forks with [superior deflate/inflate 
> performance have evolved][6]. Using them with
> OpenJDK is quite straight-forward: one just has to configure the alternative 
> implementations by setting
> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
> using these new zlib implementations for
> selected services in production and the only reason why we haven't enabled 
> them by default until now is the problem
> I've just described. The reason why these new libraries uncover the described 
> anti-pattern much more often is because
> their compression ratio is slightly different from that of the default zlib 
> library. This can easily trigger a
> `ZipException` even if an application is not using a different compression 
> levels but just a zip file created with
> another zlib version.  I'd therefore like to propose the following workaround 
> for the wrong
> `ZipOutputStream.putNextEntry()` usage in user code:
> -  ignore the compressed size if it was implicitly determined from the zip 
> file and not explicitly set by calling
>`ZipEntry.setCompressedSize()`.
> 
> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
> `JarOutputStream.putNextEntry()` to explain the
>   problem and why `putNextEntry()` will ignore the compressed size of a 
> `ZipEntry` if that was set implicitely when
>   reading that entry from a `ZipFile` or `ZipInputStream`.
> 
> 
> ### Technical Details
> 
> A zip file consists of a stream of File Entries followed by a Central 
> Directory (see [here for a more detailed
> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
> followed by the compressed Data and an
> optional Data Descriptor. The LFH contains the File Name and among other 
> attributes the Compressed and Uncompressed
> size and CRC of the Data. In the case where the latter three attributes are 
> not available at the time when the LFH is
> created, this fact will be recorded in a flag of the LFH and will trigger the 
> creation of a Data Descriptor with the
> corresponding information right after the Data section. Finally, the Central 
> Directory contains one Central 

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v5]

[Volker Simonis]

> ### Summary
> 
> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
> which can lead to the `ZipException "invalid
> entry compressed size"`.
> ### Motivation
> 
> In general it is not safe to directly write a ZipEntry obtained from 
> `ZipInputStream.getNextEntry()`,
> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
> `ZipOutputStream.putNextEntry()` to a
> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
> and write it to the `ZipOutputStream` as
> follows:
>  ZipEntry entry;
>  ZipInputStream zis = new ZipInputStream(...);
>  ZipOutputStream zos = new ZipOutputStream(...);
>  while((entry = zis.getNextEntry()) != null) {
>  zos.putNextEntry(entry);
>  zis.transferTo(zos);
>  }
> The problem with this code is that the zip file format does not record the 
> compression level used for deflation in its
> entries. In general, it doesn't even mandate a predefined compression ratio 
> per compression level. Therefore the
> compressed size recorded in a `ZipEntry` read from a zip file might differ 
> from the new compressed size produced by the
> receiving `ZipOutputStream`. Such a difference will result in a 
> `ZipException` with the following message:
>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
> got 7 bytes)
>  
> The correct way of copying all entries from one zip file into another 
> requires the creation of a new `ZipEntry` or at
> least resetting of the compressed size field. E.g.:
>  while((entry = zis.getNextEntry()) != null) {
>  ZipEntry newEntry = new ZipEntry(entry.getName());
>  zos.putNextEntry(newEntry);
>  zis.transferTo(zos);
>  }
> or:
>  while((entry = zis.getNextEntry()) != null) {
>  entry.setCompressedSize(-1);
>  zos.putNextEntry(entry);
>  zis.transferTo(zos);
>  }
> Unfortunately, there's a lot of user code out there which gets this wrong and 
> uses the bad coding pattern described
> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
> size (expected 12 but got 7 bytes)"` gives
> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
> instances of this anti-pattern on GitHub when
> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
> wrapper task][1] is affected as well as the
> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
> occurrences of this pattern in OpenJDK (see
> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
> (e.g.
> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
> :).  ### Description  So while this has
> clearly been a problem before, it apparently wasn't painful enough to trigger 
> any action from the side of the JDK.
> However, recently quite some zlib forks with [superior deflate/inflate 
> performance have evolved][6]. Using them with
> OpenJDK is quite straight-forward: one just has to configure the alternative 
> implementations by setting
> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
> using these new zlib implementations for
> selected services in production and the only reason why we haven't enabled 
> them by default until now is the problem
> I've just described. The reason why these new libraries uncover the described 
> anti-pattern much more often is because
> their compression ratio is slightly different from that of the default zlib 
> library. This can easily trigger a
> `ZipException` even if an application is not using a different compression 
> levels but just a zip file created with
> another zlib version.  I'd therefore like to propose the following workaround 
> for the wrong
> `ZipOutputStream.putNextEntry()` usage in user code:
> -  ignore the compressed size if it was implicitly determined from the zip 
> file and not explicitly set by calling
>`ZipEntry.setCompressedSize()`.
> 
> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
> `JarOutputStream.putNextEntry()` to explain the
>   problem and why `putNextEntry()` will ignore the compressed size of a 
> `ZipEntry` if that was set implicitely when
>   reading that entry from a `ZipFile` or `ZipInputStream`.
> 
> 
> ### Technical Details
> 
> A zip file consists of a stream of File Entries followed by a Central 
> Directory (see [here for a more detailed
> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
> followed by the compressed Data and an
> optional Data Descriptor. The LFH contains the File Name and among other 
> attributes the Compressed and Uncompressed
> size and CRC of the Data. In the case where the latter three attributes are 
> not available at the time when the LFH is
> created, this fact will be recorded in a flag of the LFH and will trigger the 
> creation of a Data Descriptor with the
> corresponding information right after the Data section. Finally, the Central 
> Directory contains one Central 

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]

[Lance Andersen]

On Tue, 13 Oct 2020 18:46:59 GMT, Alan Bateman  wrote:

>> Volker Simonis has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now
>> contains one commit:
>>   8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's 
>> compressed size
>
> src/java.base/share/classes/java/util/jar/JarOutputStream.java line 85:
> 
>> 83:  * ZipEntry#setCompressedSize(long)} method, then the compressed
>> 84:  * size will be set to the actual compressed size after deflation.
>> 85:  * 
> 
> I'm happy with the wording. What you think about put a p tag before "The 
> default compression method ..." so that it
> goes into its own paragraph? I'm asking because it's inconsistent to have the 
> first paragraph include the details on
> the compression method and the details on the current time pushed into a 
> second paragraph - if you generate the javadoc
> then you'll see what I mean.

I think that is a good idea to at the paragraph tag as you suggest

-

PR: https://git.openjdk.java.net/jdk/pull/520

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]

[Alan Bateman]

On Tue, 13 Oct 2020 11:40:36 GMT, Volker Simonis  wrote:

>> ### Summary
>> 
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>> 
>> In general it is not safe to directly write a ZipEntry obtained from 
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
>> and write it to the `ZipOutputStream` as
>> follows:
>>  ZipEntry entry;
>>  ZipInputStream zis = new ZipInputStream(...);
>>  ZipOutputStream zos = new ZipOutputStream(...);
>>  while((entry = zis.getNextEntry()) != null) {
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> The problem with this code is that the zip file format does not record the 
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio 
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ 
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a 
>> `ZipException` with the following message:
>>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
>> got 7 bytes)
>>  
>> The correct way of copying all entries from one zip file into another 
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>>  while((entry = zis.getNextEntry()) != null) {
>>  ZipEntry newEntry = new ZipEntry(entry.getName());
>>  zos.putNextEntry(newEntry);
>>  zis.transferTo(zos);
>>  }
>> or:
>>  while((entry = zis.getNextEntry()) != null) {
>>  entry.setCompressedSize(-1);
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> Unfortunately, there's a lot of user code out there which gets this wrong 
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
>> :).  ### Description  So while this has
>> clearly been a problem before, it apparently wasn't painful enough to 
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate 
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative 
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled 
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the 
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib 
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression 
>> levels but just a zip file created with
>> another zlib version.  I'd therefore like to propose the following 
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> -  ignore the compressed size if it was implicitly determined from the zip 
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>> 
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
>> `JarOutputStream.putNextEntry()` to explain the
>>   problem and why `putNextEntry()` will ignore the compressed size of a 
>> `ZipEntry` if that was set implicitely when
>>   reading that entry from a `ZipFile` or `ZipInputStream`.
>> 
>> 
>> ### Technical Details
>> 
>> A zip file consists of a stream of File Entries followed by a Central 
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other 
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are 
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger 

Re: RFR: 8254177: (tz) Upgrade time-zone data to tzdata2020b

[Andrew John Hughes]

On Tue, 13 Oct 2020 11:32:54 GMT, Kiran Sidhartha Ravikumar 
 wrote:

>> Looks good. I think we should release-note the removal of the 
>> "US/Pacific-New" Link on the off chance that some
>> production/testing system is looking for such a zone.
>
> Thanks for the review everyone, I have added a release note to the bug. I 
> will integrate the changes

Looks like we both reached the same patch for this one independently :)
systemv was removed from tzdata some time ago and wasn't even being read by the 
JDK as far as I could see. Its contents
are duplicated in jdk11_backward anyway. Good to see both it and pacificnew 
being removed. In the unlikely event the
removal of Pacific_New is problematic, it could be added to jdk11_backward as 
well, but it seems very unlikely.

-

PR: https://git.openjdk.java.net/jdk/pull/602

Re: RFR: 8223347: Integration of Vector API (Incubator) [v4]

[Paul Sandoz]

> This pull request is for integration of the Vector API. It was previously 
> reviewed under conditions when mercurial was
> used for the source code control system. Review threads can be found here 
> (searching for issue number 8223347 in the
> title):  
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-April/thread.html
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/thread.html
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-July/thread.html
> 
> If mercurial was still being used the code would be pushed directly, once the 
> CSR is approved. However, in this case a
> pull request is required and needs explicit reviewer approval. Between the 
> final review and this pull request no code
> has changed, except for that related to merging.

Paul Sandoz has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now
contains ten commits:

 - Merge master
 - Fix related to merge
 - HotspotIntrinsicCandidate to IntrinsicCandidate
 - Merge master
 - Fix permissions
 - Fix permissions
 - Merge master
 - Vector API new files
 - Integration of Vector API (Incubator)

-

Changes: https://git.openjdk.java.net/jdk/pull/367/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=367=03
  Stats: 295107 lines in 336 files changed: 292957 ins; 1062 del; 1088 mod
  Patch: https://git.openjdk.java.net/jdk/pull/367.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/367/head:pull/367

PR: https://git.openjdk.java.net/jdk/pull/367

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v2]

[Erik Joelsson]

On Tue, 13 Oct 2020 14:48:40 GMT, Andy Herrick  wrote:

>> JDK-8252870: Finalize (remove "incubator" from) jpackage
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8252870: Finalize (remove "incubator" from) jpackage
> - reverting two auto-generated files, and changing module-info to "@since 
> 16"

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/633

Re: RFR: 8251989: Hex formatting and parsing utility

[Roger Riggs]

On Mon, 12 Oct 2020 23:05:44 GMT, Marcono1234 
 wrote:

> Also is it common practice to use `System.out` in JDK tests? In my opinion it 
> often does not add much value once the
> unit test implementation has been completed because the output is not checked 
> during tests automatically anyways and
> might only clutter the console output.

It has been useful to see the output generated and if there are failures there 
is additional information available.

> The tests are using `String.toUpperCase`, `toLowerCase` and `format` without 
> `Locale` (therefore using the default
> one). Does the test setup guarantee a constant default locale or would be 
> better to include a `Locale` to make sure the
> tests don't break for any unusual locale?

Good point, the ROOT locale should be used. Though for the cases of the 
hexadecimal characters, they are consistently
treated across all locales.

> Has it been also considered to add support for parsing from a `Reader` to an 
> `OutputStream` and from `InputStream` to
> `Appendable` to support arbitrary length input and output?

That can be considered separately, the Jira issue 8254708 will track that 
enhancement.
A typical application reading input may need to handle a mix of input 
constructs with Hex being just one.
An application usually needs some kind of look ahead or push back to adjust to 
the contents of the stream
and that brings in a more complex grammar.

> Maybe it would also be good to mention for the method parsing and formatting 
> `int`, `long`, ... in which byte order the
> output is created.

The big/little -endian terminology usually applies to binary representations. 
The natural reading order for numbers is
left to right but it can be more explicit.

> And would it be worth supporting a delimiter _period_ or _frequency_ to only 
> apply the delimiter every nth byte? This
> would be useful when the user want to write hex chars in groups of 4 or 8.

It is a tradeoff in complexity.  The api focuses on the conversion of byte 
arrays to strings and back.
That behavior can be achieved naturally using the `toHexDigits` methods with 
the application handling the insertion of
delimiters.

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]

[Lance Andersen]

On Tue, 13 Oct 2020 11:40:36 GMT, Volker Simonis  wrote:

>> ### Summary
>> 
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code 
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>> 
>> In general it is not safe to directly write a ZipEntry obtained from 
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with 
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream` 
>> and write it to the `ZipOutputStream` as
>> follows:
>>  ZipEntry entry;
>>  ZipInputStream zis = new ZipInputStream(...);
>>  ZipOutputStream zos = new ZipOutputStream(...);
>>  while((entry = zis.getNextEntry()) != null) {
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> The problem with this code is that the zip file format does not record the 
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio 
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ 
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a 
>> `ZipException` with the following message:
>>  java.util.zip.ZipException: invalid entry compressed size (expected 12 but 
>> got 7 bytes)
>>  
>> The correct way of copying all entries from one zip file into another 
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>>  while((entry = zis.getNextEntry()) != null) {
>>  ZipEntry newEntry = new ZipEntry(entry.getName());
>>  zos.putNextEntry(newEntry);
>>  zis.transferTo(zos);
>>  }
>> or:
>>  while((entry = zis.getNextEntry()) != null) {
>>  entry.setCompressedSize(-1);
>>  zos.putNextEntry(entry);
>>  zis.transferTo(zos);
>>  }
>> Unfortunately, there's a lot of user code out there which gets this wrong 
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed 
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of 
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x 
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two 
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them 
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999 
>> :).  ### Description  So while this has
>> clearly been a problem before, it apparently wasn't painful enough to 
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate 
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative 
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by 
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled 
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the 
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib 
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression 
>> levels but just a zip file created with
>> another zlib version.  I'd therefore like to propose the following 
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> -  ignore the compressed size if it was implicitly determined from the zip 
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>> 
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and 
>> `JarOutputStream.putNextEntry()` to explain the
>>   problem and why `putNextEntry()` will ignore the compressed size of a 
>> `ZipEntry` if that was set implicitely when
>>   reading that entry from a `ZipFile` or `ZipInputStream`.
>> 
>> 
>> ### Technical Details
>> 
>> A zip file consists of a stream of File Entries followed by a Central 
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH) 
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other 
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are 
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger 

Integrated: 8251861: Remove unused jdk.internal.ref.SoftCleanable and WeakCleanable

[Kiran Sidhartha Ravikumar]

On Mon, 12 Oct 2020 19:36:52 GMT, Kiran Sidhartha Ravikumar 
 wrote:

> Hi Guys,
> 
> Please review the fix to remove jdk.internal.ref.SoftCleanable and 
> WeakCleanable classes as there is no visible usage
> of them in JDK. Currently, only PhantomCleanable is used.
> More information can be viewed at 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/067997.html
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8251861
> 
> Thanks,
> Kiran

This pull request has now been integrated.

Changeset: ba24f963
Author:Kiran Sidhartha Ravikumar 
Committer: Sean Coffey 
URL:   https://git.openjdk.java.net/jdk/commit/ba24f963
Stats: 635 lines in 4 files changed: 0 ins; 633 del; 2 mod

8251861: Remove unused jdk.internal.ref.SoftCleanable and WeakCleanable

Reviewed-by: mchung, rriggs

-

PR: https://git.openjdk.java.net/jdk/pull/616

Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v9]

[Richard Reingruber]

On Sun, 11 Oct 2020 07:20:07 GMT, Richard Reingruber  wrote:

>>> 
>>> 
>>> I tried to run testing with latest changes and latest JDK and build failed:
>>> src/hotspot/share/runtime/escapeBarrier.cpp:310:35: error: no matching 
>>> function for call to
>>> 'StackFrameStream::StackFrameStream(JavaThread*&)' 310 | StackFrameStream 
>>> fst(deoptee);
>> 
>> I noticed this too. I wanted to test with ZGC before pushing the small
>> fix. Unfortunately I get
>> 
>> #  Internal Error 
>> (/priv/d038402/git/reinrich/jdk_ea_new/src/hotspot/share/runtime/stackWatermark.inline.hpp:67),
>> pid=90890, tid=90912 #  assert(processing_started()) failed: Processing 
>> should already have started
>> 
>> [...]
>> 
>> Current thread (0x7f749c25b1c0):  JavaThread "JDWP Transport Listener: 
>> dt_socket" daemon [_thread_in_vm, id=90912,
>> stack(0x7f7474c9f000,0x7f7474da)] 
>> _threads_hazard_ptr=0x7f749c2b00c0, _nested_threads_hazard_ptr_cnt=0
>> Stack: [0x7f7474c9f000,0x7f7474da],  sp=0x7f7474d9c240,  
>> free space=1012k
>> Native frames: (J=compiled Java code, A=aot compiled Java code, 
>> j=interpreted, Vv=VM code, C=native code)
>> V  [libjvm.so+0x15b3255]  StackWatermarkSet::on_iteration(JavaThread*, frame 
>> const&)+0xa5
>> V  [libjvm.so+0xa1024f]  frame::sender(RegisterMap*) const+0x13f
>> V  [libjvm.so+0xa048f8]  frame::real_sender(RegisterMap*) const+0x18
>> V  [libjvm.so+0x176261b]  vframe::sender() const+0xeb
>> V  [libjvm.so+0x16cd56b]  JavaThread::last_java_vframe(RegisterMap*)+0x5b
>> V  [libjvm.so+0xfa7a56]  JvmtiEnvBase::vframeFor(JavaThread*, int)+0x46
>> V  [libjvm.so+0xfab8e5]  JvmtiEnvBase::check_top_frame(JavaThread*, 
>> JavaThread*, jvalue, TosState, Handle*)+0x1f5
>> V  [libjvm.so+0xfac13e]  JvmtiEnvBase::force_early_return(JavaThread*, 
>> jvalue, TosState)+0x15e
>> V  [libjvm.so+0xf36fa8]  jvmti_ForceEarlyReturnLong+0x258
>> C  [libjdwp.so+0xa8b3]  forceEarlyReturn+0x293
>> C  [libjdwp.so+0x12945]  debugLoop_run+0x1f5
>> C  [libjdwp.so+0x25bb3]  attachThread+0x33
>> V  [libjvm.so+0xfcf524]  JvmtiAgentThread::call_start_function()+0x1d4
>> V  [libjvm.so+0x16cc8f7]  JavaThread::thread_main_inner()+0x247
>> V  [libjvm.so+0x16d1ce8]  Thread::call_run()+0xf8
>> V  [libjvm.so+0x12dd75e]  thread_native_entry(Thread*)+0x10e
>> 
>> In the test case
>> `EAForceEarlyReturnOfInlinedMethodWithScalarReplacedObjectsReallocFailure` 
>> of the
>> new test `jdk/com/sun/jdi/EATests.java`
>> 
>> So far I do not have an indication that the failure is caused by this change 
>> but
>> when I run the test with -XX:-DoEscapeAnalysis then the test succeeds.
>> 
>> I need to look more into it. Wish I was a ZGC expert :)
>> 
>> Anyway I pushed the build fix. Tests succeed with default GC.
>
> The crash described above happens after JDK-8253180
> (https://github.com/openjdk/jdk/commit/b9873e18330b7e43ca47bc1c0655e7ab20828f7a)
>  when executing `EATests.java` with
> ZGC:  make run-test TEST=test/jdk/com/sun/jdi/EATests.java 
> JTREG=VM_OPTIONS=-XX:+UseZGC
> 
> My understanding of JDK-8253180 (and ZGC) is rather vague. To me it looks as 
> if stackwalks outside of a
> safepoint/handshake on suspended threads are currently not supported. It 
> would be my understanding that
> `StackWatermarkSet::start_processing()` needs to be called before walking the 
> stack of a thread. Currently this is only
> done in preparation of a safepoint or handshake.  
> `JvmtiEnvBase::check_top_frame()` walks the stack of a suspended
> thread without safepoint/handshake. This triggers the crash in my opinion. 
> When `StackWatermarkSet::start_processing()`
> is called before the test succeeds.  I will ask Erik Österlund about this.

The issues with ZGC concurrent thread stack processing will be resolved with 
#627

-

PR: https://git.openjdk.java.net/jdk/pull/119

Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]

[Lance Andersen]

On Wed, 7 Oct 2020 17:04:02 GMT, Lance Andersen  wrote:

>> I totally agree. What about using just the last sentence (as you've 
>> proposed) in the spec section and add the other to
>> as @implNote? O you think the last sentence will be enough?
>
> I think we can just go with the last sentence/paragraph.  Perhaps  we can 
> further simplify the  paragraph/sentence with
> something like:
> The compressed entry size will be recalculated  for compressed (DEFLATED) 
> entries when  ZipEntry::setCompressedSize has
> not been explicitly called on the ZipEntry.
> or
> 
> The compressed (DEFLATED) entry size will be recalculated when  
> ZipEntry::setCompressedSize has not been explicitly
> called on the ZipEntry.

I think the wording looks much better now

-

PR: https://git.openjdk.java.net/jdk/pull/520

Re: RFR: 8251861: Remove unused jdk.internal.ref.SoftCleanable and WeakCleanable

[Kiran Sidhartha Ravikumar]

On Tue, 13 Oct 2020 16:39:51 GMT, Roger Riggs  wrote:

>> Hi Guys,
>> 
>> Please review the fix to remove jdk.internal.ref.SoftCleanable and 
>> WeakCleanable classes as there is no visible usage
>> of them in JDK. Currently, only PhantomCleanable is used.
>> More information can be viewed at 
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/067997.html
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8251861
>> 
>> Thanks,
>> Kiran
>
> Marked as reviewed by rriggs (Reviewer).

Thanks guys

-

PR: https://git.openjdk.java.net/jdk/pull/616

Re: RFR: 8251861: Remove unused jdk.internal.ref.SoftCleanable and WeakCleanable

[Roger Riggs]

On Mon, 12 Oct 2020 19:36:52 GMT, Kiran Sidhartha Ravikumar 
 wrote:

> Hi Guys,
> 
> Please review the fix to remove jdk.internal.ref.SoftCleanable and 
> WeakCleanable classes as there is no visible usage
> of them in JDK. Currently, only PhantomCleanable is used.
> More information can be viewed at 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/067997.html
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8251861
> 
> Thanks,
> Kiran

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/616

Re: RFR: 8251989: Hex formatting and parsing utility

[Roger Riggs]

On Mon, 12 Oct 2020 20:48:52 GMT, Marcono1234 
 wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> Review comments and suggestions welcome.
>
> src/java.base/share/classes/java/util/HexFormat.java line 45:
> 
>> 43:  * the {@code withXXX} methods return copies of {@code HexFormat}ters 
>> modified
>> 44:  * {@link #withPrefix(String)}, {@link #withSuffix(String)}, {@link 
>> #withDelimiter(String)}
>> 45:  * or choice of {@link #withUppercase()} or {@link #withLowercase()} 
>> parameters using
> 
> Would withUpperCase and withLowerCase 
> (captial "C") be better to be consistent
> with `String.toUpperCase` and `Character.toUpperCase`?

Good point, the uppercase() method changes too;  ->  isUpperCase().

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: 8251989: Hex formatting and parsing utility

[Roger Riggs]

On Mon, 12 Oct 2020 22:36:34 GMT, Marcono1234 
 wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> Review comments and suggestions welcome.
>
> src/java.base/share/classes/java/util/HexFormat.java line 1015:
> 
>> 1013:  */
>> 1014: @Override
>> 1015: public String toString() {
> 
> Might be useful to also include the class name?

The caller may need to provide their own context for the output.

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: 8247402: rewrite the implementation requirements for Map::compute()

[Stuart Marks]

On Tue, 13 Oct 2020 13:35:37 GMT, Pavel Rappo  wrote:

>> The proposed implementation behaves differently from the existing 
>> implementation in the case of `null` value. That is,
>> when `map.containsKey(key) == true && map.get(key) == null`. The difference 
>> is that the proposed implementation will
>> remove the `(key, null)` mapping if `newValue == null`, whereas the existing 
>> implementation will not.
>
> Perhaps I should clarify my previous comment. When I said "implementation" 
> what I meant was that pseudo-code inside the
> `@implSpec` tag, not the actual body of `default V compute(K key, 
> BiFunction
> remappingFunction)`.

This change is to a normative portion of the specification, and as such will 
require a CSR.

-

PR: https://git.openjdk.java.net/jdk/pull/451

Re: RFR: 8251989: Hex formatting and parsing utility

[Roger Riggs]

On Mon, 12 Oct 2020 20:41:20 GMT, Marcono1234 
 wrote:

>> java.util.HexFormat utility:
>> 
>>  - Format and parse hexadecimal strings, with parameters for delimiter, 
>> prefix, suffix and upper/lowercase
>>  - Static factories and builder methods to create HexFormat copies with 
>> modified parameters.
>>  - Consistent naming of methods for conversion of byte arrays to formatted 
>> strings and back: formatHex and parseHex
>>  - Consistent naming of methods for conversion of primitive types: 
>> toHexDigits... and fromHexDigits...
>>  - Prefix and suffixes now apply to each formatted value, not the string as 
>> a whole
>>  - Using java.util.Appendable as a target for buffered conversions so output 
>> to Writers and PrintStreams
>>like System.out are supported in addition to StringBuilder. (IOExceptions 
>> are converted to unchecked exceptions)
>>  - Immutable and thread safe, a "value-based" class
>> 
>> See the [HexFormat
>> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html)
>>  for details.
>> Review comments and suggestions welcome.
>
> test/jdk/java/util/HexFormat/HexFormatTest.java line 406:
> 
>> 404:
>> 405: int byteVal = hex.fromHexDigits(byteStr);
>> 406: assert(byteStr.equals("7f"));
> 
> Why use regular `assert` statements in this test method? TestNG's methods 
> likely yield more useful results.

These code fragments are the examples used in the javadoc; making a simple 
copy/paste from working code to the javadoc.
In the example code `assert`, a Java language construct is cleaner than showing 
testng assertions).

-

PR: https://git.openjdk.java.net/jdk/pull/482

Re: RFR: 8251861: Remove unused jdk.internal.ref.SoftCleanable and WeakCleanable

[Mandy Chung]

On Mon, 12 Oct 2020 19:36:52 GMT, Kiran Sidhartha Ravikumar 
 wrote:

> Hi Guys,
> 
> Please review the fix to remove jdk.internal.ref.SoftCleanable and 
> WeakCleanable classes as there is no visible usage
> of them in JDK. Currently, only PhantomCleanable is used.
> More information can be viewed at 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/067997.html
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8251861
> 
> Thanks,
> Kiran

Thanks for taking this one.  Looks okay.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/616

Re: RFR: 8223347: Integration of Vector API (Incubator) [v3]

[Paul Sandoz]

> This pull request is for integration of the Vector API. It was previously 
> reviewed under conditions when mercurial was
> used for the source code control system. Review threads can be found here 
> (searching for issue number 8223347 in the
> title):  
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-April/thread.html
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/thread.html
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-July/thread.html
> 
> If mercurial was still being used the code would be pushed directly, once the 
> CSR is approved. However, in this case a
> pull request is required and needs explicit reviewer approval. Between the 
> final review and this pull request no code
> has changed, except for that related to merging.

Paul Sandoz has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix related to merge

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/367/files
  - new: https://git.openjdk.java.net/jdk/pull/367/files/9cca17b8..d5acb4ff

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=367=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=367=01-02

  Stats: 76 lines in 1 file changed: 0 ins; 0 del; 76 mod
  Patch: https://git.openjdk.java.net/jdk/pull/367.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/367/head:pull/367

PR: https://git.openjdk.java.net/jdk/pull/367