> On May 18, 2015, at 2:07 PM, Paul Sandoz <paul.san...@oracle.com> wrote:
>
> Hi Jim,
>
> I reviewed the JDK code. There is quite a lot and i don't understand all the
> semantics but it generally looks ok.
>
> Stuff below is mostly suggested API tweaks.
>
> Paul.
>
> jdk.internal.jimage.BasicImageReader
> --
>
> 124 public String[] getEntryNames() {
> 125 int[] offsets = substrate.attributeOffsets();
> 126 List<String> list = new ArrayList<>();
> 127
> 128 for (int offset : offsets) {
> 129 if (offset != 0) {
> 130 ImageLocation location =
> 131 ImageLocation.readFrom(this, offset);
> 132 list.add(location.getFullNameString());
> 133 }
> 134 }
> 135
> 136 String[] array = list.toArray(new String[0]);
> 137 Arrays.sort(array);
> 138
> 139 return array;
> 140 }
>
> You can do this instead:
>
> list.sort(null); // more idiomatic for Java 8
> return list.toArray(new String[list.size()]);
>
> It should be marginally more efficient just because the size information is
> passed to the array.
>
> Alternatively you use a stream too, depending on your perf requirements:
>
> return IntStream.of(substrate.attributeOffsets())
> .filter(o -> o != 0)
> .mapToObj(o -> ImageLocation.readFrom(this, o).getFullNameString())
> .sorted()
> .toArray(String[]::new);
Done
>
> Same applies to getAllLocations:
>
> return IntStream.of(substrate.attributeOffsets())
> .filter(o -> o != 0)
> .mapToObj(o -> ImageLocation.readFrom(this, o))
> .sorted(Comparator.comparing(ImageLocation::getFullNameString)) // Use
> Comparator.comparing in any case for key extraction
> .toArray(ImageLocation[]::new);
>
>
Done
> jdk.internal.jrtfs.JrtFileSystem.java
> --
>
> 494 private Iterator<Path> nodesToIterator(Path path, String childPrefix,
> List<Node> childNodes) {
> 495 List<Path> childPaths;
> 496 if (childPrefix == null) {
> 497 childPaths = childNodes.stream()
> 498 .map(child -> toJrtPath(child.getNameString()))
> 499 .collect(Collectors.toCollection(ArrayList::new));
> 500 } else {
> 501 childPaths = childNodes.stream()
> 502 .map(child -> toJrtPath(childPrefix +
> child.getNameString().substring(1)))
> 503 .collect(Collectors.toCollection(ArrayList::new));
> 504 }
> 505 return childPaths.iterator();
> 506 }
>
> Use Collectors.toList(). Could be marginally simplified as:
>
> Function<String, JrtPath> f = childPrefix == null
> ? child -> toJrtPath(child.getNameString())
> : child -> toJrtPath(childPrefix + child.getNameString().substring(1));
> return childNodes.stream().map(f).collect(toList()).iterator();
>
>
Required:
Function<Node, Path> f = childPrefix == null
? child -> toJrtPath(child.getNameString())
: child -> toJrtPath(childPrefix +
child.getNameString().substring(1));
return childNodes.stream().map(f).collect(toList()).iterator();
> jdk.internal.jimage.ExternalFilesWriter
>
> 93 private static String nativeDir(String filename) {
> 94 if (System.getProperty("os.name").startsWith("Windows")) {
> 95 if (filename.endsWith(".dll") || filename.endsWith(".diz")
> 96 || filename.endsWith(".pdb") ||
> filename.endsWith(".map")) {
> 97 return "bin";
> 98 } else {
> 99 return "lib";
> 100 }
> 101 } else {
> 102 return "lib";
> 103 }
> 104 }
>
> Does that need to be performed in a doPriv block?
>
>
System.getProperty("os.name”) does not appear to be privileged.
permission java.util.PropertyPermission "os.name", "read”; in grant { } section
> jdk.internal.jimage.ImageFileCreator
> --
>
> 108 private void readAllEntries(Map<String, Set<String>>
> modulePackagesMap,
> 109 Set<Archive> archives) {
> 110 archives.stream().forEach((archive) -> {
> 111 List<Entry> archiveResources = new ArrayList<>();
> 112 archive.visitEntries(x-> archiveResources.add(x));
> 113 String mn = archive.moduleName();
> 114 entriesForModule.put(mn, archiveResources);
> 115 // Extract package names
> 116 List<Entry> classes = archiveResources.stream()
> 117 .filter(n -> n.type() == EntryType.CLASS_OR_RESOURCE)
> 118 .collect(Collectors.toList());
> 119 Set<String> pkgs = classes.stream().map(Entry::name)
> 120 .filter(n -> isResourcePackage(n))
> 121 .map(ImageFileCreator::toPackage)
> 122 .collect(Collectors.toSet());
> 123 modulePackagesMap.put(mn, pkgs);
> 124 });
> 125 }
>
> Do not need to create the intermediate List<Entry>.
>
> If archive.vistEntries was changed instead to:
>
> Stream<Entry> entries();
>
> The code above might flow better and, off the top of my head, you could do
> something like:
>
> archives.stream().forEach((archive) -> {
> Map<Boolean, List<Entry>> es = archive.entries()
> .collect(Collectors.partitioningBy(n -> n.type() ==
> EntryType.CLASS_OR_RESOURCE));
>
> String mn = archive.moduleName();
> entriesForModule.put(mn, es.get(false));
>
> // Extract package names
> Set<String> pkgs = es.get(true).stream().map(Entry::name)
> .filter(n -> isResourcePackage(n))
> .map(ImageFileCreator::toPackage)
> .collect(Collectors.toSet());
> modulePackagesMap.put(mn, pkgs);
> });
>
> For recreateJimage i think you can then do:
>
> Map<String, List<Entry>> entriesForModule =
> archives.stream().collect(Collectors.toMap(
> Archive::moduleName,
> a -> a.entries().collect(Collectors.toList())
> ));
>
> Or derive from Map<String, Archive>
>
> entriesForModule = nameToArchive.entrySet()
> .stream()
> .collect(Collectors.toMap(e -> e.getKey(),
> e ->
> e.getValue().entries().collect(Collectors.toList())));
Done, modified slightly.
>
>
> jdk.internal.jimage.ImageModuleDataWriter
> --
>
> 69 Map<String, List<String>> modulePackages = new LinkedHashMap<>();
> 70 modules.stream().sorted().forEach((moduleName) -> {
> 71 List<String> localPackages =
> modulePackagesMap.get(moduleName).stream()
> 72 .map(pn -> pn.replace('.', '/'))
> 73 .sorted()
> 74 .collect(Collectors.toList());
> 75 modulePackages.put(moduleName, localPackages);
> 76 });
>
> can replace forEach with collect(Collectors.toMap(Functions.identity(), n ->
> modulePackagesMap.get(n)...)); assuming no duplicate keys?
>
>
Done, Functions -> Function
> jdk.internal.jimage.ResourcePoolImpl
>
> 196 Set<String> pkgs = moduleToPackage.get(res.getModule());
> 197 if (pkgs == null) {
> 198 pkgs = new HashSet<>();
> 199 moduleToPackage.put(res.getModule(), pkgs);
> 200 }
>
> use
>
> Set<String> pkgs = moduleToPackage.computeIfAbsent(res.getModule(),k -> new
> HashSet<>());
>
Done.
> Paul.
>
> On May 15, 2015, at 5:45 PM, Jim Laskey (Oracle) <james.las...@oracle.com>
> wrote:
>
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/top/
>> <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/top/>
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/hotspot/
>> <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/hotspot/>
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/jdk/
>> <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/jdk/>
>> http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/langtools/
>> <http://cr.openjdk.java.net/~jlaskey/jdk9-jimage/langtools/>
>>
>> There are some issues with regard to the hotspot changes, but I’m working
>> with Coleen and John R. to resolve those.
>>
>> Cheers,
>>
>> — Jim
>>
>
