On 11/17/17, 3:35 AM, Alan Bateman wrote:
On 17/11/2017 02:18, Xueming Shen wrote:
Hi,
Please help review the change for JDK-8189611.
issue: https://bugs.openjdk.java.net/browse/JDK-8189611
webrev: http://cr.openjdk.java.net/~sherman/8189611/webrev
Just a few initial comments/questions on the API additions:
1. getRealName is very welcome but I think it should be a no-arg
method on JarEntry, not JarFile. That would make it easier to use and
also avoids the temptation to call JarFile.getRealName with an entry
in a different JAR file.
I was considering to put it into JarEntry. The only concern is that all
mr jarfile related stuff
are in JarFile, it's kinda easy/convenient to refer to the concept "if
multi-release is..." in
JarFile than JarEntry, especially considering mr jarfile normally should
only be interested/
used by limited group of developer, it might be better to simply put it
near those mr related
methods. But I don't have a strong opinion about it. I can move it into
JarEntry, if it's desired.
2. JarFile.versionedStream() is very welcome too but the javadoc says
"latest versioned entry" when it really means the highest version that
is less than or equal to the runtime version that the JarFile was
opened with. I suspect this one might take a few iterations to get the
wording smooth.
sure. will see if I can find better wording.
3. Is ZipFile.entryNameStream really needed? Just asking because
zf.stream().map(ZipEntry::getName) is possible today.
It's not a "must" for sure. The motivation behind this is that my
observation of most normal use scenario
inside JDK is that only the "name" info is interested/collect/used. The
use pattern usually is
zf.stream().map(ZipEntry::getName)
same for the old Enumeration case, only the "name" is used and the
"entry" object is thrown away mostly.
Other than the memory consumption (showed in those two snapshots), it's
also relatively costly to create
the ZipEntry especially its "esxdostime" calculation. The jmh numbers
from a simple benchmark test
suggests the entryNameStream is about 15-20% faster. So, the only reason
it's there is for better
performance. And hope it might be useful/convenient for the end user as
well. If it's a concern, I can
hide it into the SharedSecrets, or simply use the
zf.stream().map(ZipEntry::getName) instead.
--------------------------------------
JarFileBenchmark.jf_entries avgt 20 0.871 ± 0.079 ms/op
JarFileBenchmark.jf_entryNameStream avgt 20 0.786 ± 0.165 ms/op
JarFileBenchmark.jf_getEntry avgt 20 6.805 ± 0.615 ms/op
JarFileBenchmark.jf_stream avgt 20 0.915 ± 0.096 ms/op
JarFileBenchmark.jf_versionedStream avgt 20 9.412 ± 0.642 ms/op
JarFileBenchmark.zf_entries avgt 20 0.877 ± 0.092 ms/op
JarFileBenchmark.zf_entryNameStream avgt 20 0.730 ± 0.123 ms/op
JarFileBenchmark.zf_getEntry avgt 20 1.805 ± 0.177 ms/op
JarFileBenchmark.zf_stream avgt 20 0.926 ± 0.167 ms/op
---------------------------------------
Thanks!
-Sherman
