RFR: Proposed jimage refresh for JDK9

[Paul Sandoz]

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.laskey at 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
>