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);
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);
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();
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?
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())));
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?
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<>());
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
>
