Re: RFR: 8163798: Create a JarFile versionedStream method

[Mandy Chung]

> On Sep 12, 2016, at 5:14 PM, Steve Drach  wrote:
> 
> http://cr.openjdk.java.net/~sdrach/8163798/webrev.06/ 
> 
+1

Mandy

Re: RFR: 8163798: Create a JarFile versionedStream method

[Paul Sandoz]

> On 12 Sep 2016, at 17:14, Steve Drach  wrote:
> 
> I guess I’m going to keep doing this until I get it right ;-)  Here’s another 
> webrev that doesn’t use an exception for a common case, and addresses most of 
> Mandy concerns.
> 
> http://cr.openjdk.java.net/~sdrach/8163798/webrev.06/ 
> 
> 

Looks good,
Paul.

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

I guess I’m going to keep doing this until I get it right ;-)  Here’s another 
webrev that doesn’t use an exception for a common case, and addresses most of 
Mandy concerns.  

http://cr.openjdk.java.net/~sdrach/8163798/webrev.06/ 


Comments in-line

> This version looks good.
> 
> Can you add the javadoc to describe what the stream method returns (a union 
> of the base entries + all versioned entries <= JarFile::getVersion.

Done

> 
> Nit:  you may want to call jf.getVersion().major() once rather than the 
> number of entries.

Done

> 
> Now that you have jdk.internal.util.jar internal API for MRJAR.  It may be 
> useful to add a static getRealName(JarFile jar, JarEntry entry) method for 
> convenience such that jdeps and jlink can use this MRJAR-specific internal 
> API and no need to use the shared secret. Maybe rename VersionedStream to 
> VersionedJarFileHelper?
> 
> TestVersionedStream::close
>   You may want to use jdk.testlibrary.FileUtils and deleteFileTreeWithRetry 
> may be what you want.

I can’t use it because it removes the top-level directory (i.e. JTwork/scratch).

> 
> Mandy

Re: RFR: 8163798: Create a JarFile versionedStream method

[Mandy Chung]

> On Sep 12, 2016, at 2:17 PM, Steve Drach  wrote:
> 
> Here’s a new webrev addressing Paul’s additional concerns
> 
> http://cr.openjdk.java.net/~sdrach/8163798/webrev.04/ 
> 

This version looks good.

Can you add the javadoc to describe what the stream method returns (a union of 
the base entries + all versioned entries <= JarFile::getVersion.

Nit:  you may want to call jf.getVersion().major() once rather than the number 
of entries.

Now that you have jdk.internal.util.jar internal API for MRJAR.  It may be 
useful to add a static getRealName(JarFile jar, JarEntry entry) method for 
convenience such that jdeps and jlink can use this MRJAR-specific internal API 
and no need to use the shared secret. Maybe rename VersionedStream to 
VersionedJarFileHelper?

TestVersionedStream::close
   You may want to use jdk.testlibrary.FileUtils and deleteFileTreeWithRetry 
may be what you want.

Mandy

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

Here’s a new webrev addressing Paul’s additional concerns

http://cr.openjdk.java.net/~sdrach/8163798/webrev.04/ 



> Ok, better. Now there is no need to create an instance of VersionedStream, 
> all methods can be static.

Done

> 
> I wonder if you can combine the checks for the index:
> 
>   64 int index = name.indexOf('/', META_INF_VERSIONS_LEN);
>   65 if (index == -1) {

Removed that test, let Integer.parseint handle it

> 
>   77 if (++index < name.length()) {
> 
> e.g. no slash, or found at end (not just malformed since we want to skip 
> version directory entries if present?)
> 
>   if (index == -1 || index == name.length() - 1)
>
> 
>>> 
>>> Also can getJarEntry ever return null?
>> 
>> yes, that’s why it’s filtered out
>> 
> 
> Under what circumstances in this case can it return null? since you have 
> iterated over the existing entries and reduced to a distinct subset.

Right, that was before I put the “fix” in JarFile at line 561.  I removed the 
null check

> 
> Paul.
> 
>>> 
>>> Claes makes a good point regarding performance. I would suggest getting 
>>> this functional and tested then tweaking for performance.
>>> 
>>> Paul.
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Paul Sandoz]

> On 12 Sep 2016, at 12:36, Steve Drach  wrote:
> 
>>> I made a simple change, the new webrev is 
>>> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
>>> 
>>> 
>> 
>> I don’t like the state interplay between allowedVersions and getBaseSuffix, 
>> and the filtering for null. Consider merging filter.map.filter into a single 
>> flatMap.
> 
> Moved it into a map as Claes suggested
> 

Ok, better. Now there is no need to create an instance of VersionedStream, all 
methods can be static.

I wonder if you can combine the checks for the index:

  64 int index = name.indexOf('/', META_INF_VERSIONS_LEN);
  65 if (index == -1) {

  77 if (++index < name.length()) {

e.g. no slash, or found at end (not just malformed since we want to skip 
version directory entries if present?)

  if (index == -1 || index == name.length() - 1)


>> 
>> Also can getJarEntry ever return null?
> 
> yes, that’s why it’s filtered out
> 

Under what circumstances in this case can it return null? since you have 
iterated over the existing entries and reduced to a distinct subset.

Paul.

>> 
>> Claes makes a good point regarding performance. I would suggest getting this 
>> functional and tested then tweaking for performance.
>> 
>> Paul.

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

Here’s a new webrev that addresses Claes’ and Paul’s concerns

http://cr.openjdk.java.net/~sdrach/8163798/webrev.03/ 


> On Sep 11, 2016, at 1:12 PM, Steve Drach  wrote:
> 
> I made a simple change, the new webrev is 
> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
> 
> 
>> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
>> 
>> Hi,
>> 
>> Please review this changeset that adds a VersionedStream class to the 
>> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
>> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
>> make a public JarFile::versionedStream method at this time.  Once we get 
>> sufficient experience with this and find a few more use cases, we will 
>> revisit the idea of making this a public method in JarFile.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
>> 
>> 
>> Thanks,
>> Steve
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

>> I made a simple change, the new webrev is 
>> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
>> 
>> 
> 
> I don’t like the state interplay between allowedVersions and getBaseSuffix, 
> and the filtering for null. Consider merging filter.map.filter into a single 
> flatMap.

Moved it into a map as Claes suggested

> 
> Also can getJarEntry ever return null?

yes, that’s why it’s filtered out

> 
> Claes makes a good point regarding performance. I would suggest getting this 
> functional and tested then tweaking for performance.
> 
> Paul.
> 
> 
> 
>>> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review this changeset that adds a VersionedStream class to the 
>>> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
>>> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
>>> make a public JarFile::versionedStream method at this time.  Once we get 
>>> sufficient experience with this and find a few more use cases, we will 
>>> revisit the idea of making this a public method in JarFile.
>>> 
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
>>> 
>>> 
>>> Thanks,
>>> Steve
>> 
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

> jdk/internal/util/jar/VersionedStream.java:
> 
> - use Integer.parseInt(name, vptr, ptr, 10) to avoid creating vstr on
>  every entry

done

> 
> - folding allowedVersions into getBaseSuffix seems easier than to keep
>  global state (sptr) and splitting the logic over a filter and a map
>  operation

done

> 
> - distinct() could be used instead of explicitly collecting to a

I did use distinct in webrev.02.  I left it in webrev.03 (coming soon).

>  LinkedHashSet, but this does make me wonder if we could do even
>  better by creating a map from base name to the appropriate versioned
>  JarEntry and return a stream over that map to avoid looking things up
>  via jf::getJarEntry.

I look things up with getJarEntry to get the aliased entry.  Otherwise we get 
the real name and we want to refer to entries by the base name.

> Could be quite a bit more efficient, and as we
>  have to create a set or do distinct the overheads should be roughly
>  the same either way
> 
> - declaring META_INF_VERSIONS_LEN seems like a premature optimization

Left it in, it’s harmless and doesn’t need to be computed every time

> 
> - nit: static final is preferred over final static

done

> 
> - nit: rename *ptr to *Index

done

> 
> Test and JarFile changes seem fine to me.
> 
> Thanks!
> 
> /Claes
> 
> On 2016-09-11 22:12, Steve Drach wrote:
>> I made a simple change, the new webrev is 
>> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
>> 
>> 
>>> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review this changeset that adds a VersionedStream class to the 
>>> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
>>> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
>>> make a public JarFile::versionedStream method at this time.  Once we get 
>>> sufficient experience with this and find a few more use cases, we will 
>>> revisit the idea of making this a public method in JarFile.
>>> 
>>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
>>> 
>>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
>>> 
>>> 
>>> Thanks,
>>> Steve
>> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Paul Sandoz]

> On 11 Sep 2016, at 13:12, Steve Drach  wrote:
> 
> I made a simple change, the new webrev is 
> http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 
> 
> 

I don’t like the state interplay between allowedVersions and getBaseSuffix, and 
the filtering for null. Consider merging filter.map.filter into a single 
flatMap.

Also can getJarEntry ever return null?

Claes makes a good point regarding performance. I would suggest getting this 
functional and tested then tweaking for performance.

Paul.



>> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
>> 
>> Hi,
>> 
>> Please review this changeset that adds a VersionedStream class to the 
>> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
>> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
>> make a public JarFile::versionedStream method at this time.  Once we get 
>> sufficient experience with this and find a few more use cases, we will 
>> revisit the idea of making this a public method in JarFile.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
>> 
>> 
>> Thanks,
>> Steve
> 

Re: RFR: 8163798: Create a JarFile versionedStream method

[Claes Redestad]

Hi,

jdk/internal/util/jar/VersionedStream.java:

- use Integer.parseInt(name, vptr, ptr, 10) to avoid creating vstr on
  every entry

- folding allowedVersions into getBaseSuffix seems easier than to keep
  global state (sptr) and splitting the logic over a filter and a map
  operation

- distinct() could be used instead of explicitly collecting to a
  LinkedHashSet, but this does make me wonder if we could do even
  better by creating a map from base name to the appropriate versioned
  JarEntry and return a stream over that map to avoid looking things up
  via jf::getJarEntry. Could be quite a bit more efficient, and as we
  have to create a set or do distinct the overheads should be roughly
  the same either way

- declaring META_INF_VERSIONS_LEN seems like a premature optimization

- nit: static final is preferred over final static

- nit: rename *ptr to *Index

Test and JarFile changes seem fine to me.

Thanks!

/Claes

On 2016-09-11 22:12, Steve Drach wrote:

I made a simple change, the new webrev is 
http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 



On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:

Hi,

Please review this changeset that adds a VersionedStream class to the 
jdk.internal.util.jar package.  Some may recall that I submitted a similar RFR 
a few weeks ago; this is a redesign from that one.  We decided not to make a 
public JarFile::versionedStream method at this time.  Once we get sufficient 
experience with this and find a few more use cases, we will revisit the idea of 
making this a public method in JarFile.

issue: https://bugs.openjdk.java.net/browse/JDK-8163798 

webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 


Thanks,
Steve

Re: RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

I made a simple change, the new webrev is 
http://cr.openjdk.java.net/~sdrach/8163798/webrev.02/ 


> On Sep 9, 2016, at 4:02 PM, Steve Drach  wrote:
> 
> Hi,
> 
> Please review this changeset that adds a VersionedStream class to the 
> jdk.internal.util.jar package.  Some may recall that I submitted a similar 
> RFR a few weeks ago; this is a redesign from that one.  We decided not to 
> make a public JarFile::versionedStream method at this time.  Once we get 
> sufficient experience with this and find a few more use cases, we will 
> revisit the idea of making this a public method in JarFile.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 
> 
> 
> Thanks,
> Steve

RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

Hi,

Please review this changeset that adds a VersionedStream class to the 
jdk.internal.util.jar package.  Some may recall that I submitted a similar RFR 
a few weeks ago; this is a redesign from that one.  We decided not to make a 
public JarFile::versionedStream method at this time.  Once we get sufficient 
experience with this and find a few more use cases, we will revisit the idea of 
making this a public method in JarFile.

issue: https://bugs.openjdk.java.net/browse/JDK-8163798 

webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 


Thanks,
Steve