Hi, Let’s try again. I considered the issues brought up in the last review and was able to not only remove the versionedEntry altogether, I was able to greatly simplify the entire changeset. I removed all changes to JarEntry and JarVerifier, and added a name field and some simple methods to JarFileEntry. This solved a whole bunch of potential issues. I’m also creating the jar files used in the tests so there are no binaries in the changeset. One test, MultiReleaseJarURLConnection, has an error on windows because it can’t delete one of the created jar files. I don’t think I can do anything about it — JDK-4823678 is a 12 year old bug that describes the problem. If anyone has an idea on what I can do to make this a clean test, please let me know.
Issue: https://bugs.openjdk.java.net/browse/JDK-8132734 <https://bugs.openjdk.java.net/browse/JDK-8132734> JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305 <https://bugs.openjdk.java.net/browse/JDK-8047305> Webrev: http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/ <http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/> Thanks, Steve > On Oct 15, 2015, at 8:47 PM, Xueming Shen <xueming.s...@oracle.com> wrote: > > On 10/15/15 1:53 AM, Paul Sandoz wrote: >>> On 15 Oct 2015, at 05:00, Xueming Shen <xueming.s...@oracle.com> wrote: >>> >>> I'm not sure if it is a good idea, from performance perspective, to add a >>> "versionEntry" field into the JarEntry >>> to support this feature, given most of the jar files might not be >>> multi-release-jar aware, and the Jar input& >>> output streams dont work with a multi-release jar directly. Why should they >>> all pay a runtime price for it. If >>> we really have to add an extra field, the JarFileEntry might be a better >>> place, and it might be desired to >>> define a new subclass JarFileEntryMR to use when the MR is enabled, instead >>> of adding directly into the existing >>> JarFileEntry. >>> >> According to jol there is currently space available due to alignment. If >> there was not it would add about 4% in direct instance size. But the actual >> footprint is likely to be chunkier because of the string character storage >> for the name so the % increase in size would be smaller e.g. perhaps on >> average < 2% which might be ok given that i presume such entries are >> unlikely to be cached. >> >> So i am not concerned about the size. If there was a way to design it to >> avoid modification of existing classes all the better, but i dunno if it is >> possible. Steve will surely know more. >> >> Paul. >> > > Let's try it from another angle:-) Based on the webrev, no one need to and > actually does create a > JarEntry with a versionedEntry, except JarFile, and JarFile only creates its > own version of JarEntry, > the JarFileEntry. > > The only non-JarFile consumer of "versioned" JarEntry is the package private > JarVerifier.getCoderSigners, > and obviously it takes a JarFile together with the source JarEntry (again, if > this jarEntry is not from A > JarFile, it should never have a "versionedEntry") > > So why do you want to put this field into the super class JarEntry, not the > JarFileEntry, don't see any > benefit of doing that. > > While I'm writing this email, it appears to me that we might have a more > "severe" issue with the > general/base JarEntry class to hold the link to the "versionedEntry". The > "general" JarEntry object is > not associated with a specific JarFile (the JarFileEntry does). So there is > no way for > JarFile.getInputStream(ZipFile ze) to verify that the JarEntry passed in and > its "versionedEntry" is > actually belong to "this" JarFile in the following implementation, if the > "ze" is just a JarEntry but > NOT instanceof of a JarFileEntry > > 759 public synchronized InputStream getInputStream(ZipEntry ze) > 760 throws IOException > 761 { > 762 maybeInstantiateVerifier(); > 763 > 764 if (ze instanceof JarEntry) { > 765 ZipEntry vze = ((JarEntry)ze).versionedEntry; > 766 if (vze != null) { > 767 return getInputStream(vze); > 768 } > 769 } > 770 > > I think it's a bug of the implementation if we don't check, as the "versioned > entry" is really > associated to the jar file that creates it. Sure, as I said above, there is > actually no way you can > create a general JarEntry or a JarFileEntry with a "versionedEntry" from > "outside", but it appears > to be possible (have not tried, just in theory) to mess up the current > mechanism by passing a > "jar entry" from one JarFile instance to another one, if two JarFile > instances are open on the same > multi-release-jar, but with different "version setting" policy... > > But again, I still believe it might be a wrong approach to add such a > "versionedEntry" into any of the > JarEntry, including the JarFileEntry. As specified by the specification, the > returned entry should be the > jar entry pointing to the versioned entity in the Jar File, not the root one. > The question I would like to > ask is why do you even need the "root entry" at all, if there is a matched > versioned one. It might be > desired to have a mechanism to return the "base/root name" for such an entry, > but it probably does > not deserve a "dedicate" entry object. > > -Sherman > >> >> >
