RFR: 8232864: Classes generated by GenerateJLIClassesPlugin.java are not reproducable

[Mandy Chung]

Hi Jie,

This looks good but I ran into test/jdk/tools/jlink/BasicTest failure 
with OOME in some system configuration.   I made a minor fix to the 
cleanup of the configuration collections after the plugin executes.

          // Let it go
-        speciesTypes = null;
-        invokerTypes = null;
-        dmhMethods = null;
+        speciesTypes.clear();
+        invokerTypes.clear();
+        callSiteTypes.clear();
+        dmhMethods.clear();

I have pushed the patch with the above change.

Mandy

On 10/25/19 7:34 PM, Jie Fu wrote:
>
> Hi Mandy,
>
> Thanks for your review and help.
> I think your refactoring is a nice improvement.
>
> Updated: http://cr.openjdk.java.net/~jiefu/8232864/webrev.03/
> It seems much better now.
>
> Testing:
>  - make test TEST="tier1 tier2 tier3" CONF=release
>
> Thanks a lot.
> Best regards,
> Jie
>
> On 2019/10/26 上午3:29, Mandy Chung wrote:
>>
>>
>> On 10/23/19 11:21 PM, Jie Fu wrote:
>>> Hi Mandy,
>>>
>>> Updated: http://cr.openjdk.java.net/~jiefu/8232864/webrev.02/
>>> It seems better now.
>>>
>> It's better while I think this can be further simplified.
>>
>> line 101 to 217 involves iterating on the default sets/map and then
>> create a new TreeSet and TreeMap and then process the config file.
>>
>> Currently addDMHMethodType validates the methodType and then adds
>> to dmhMethods which ensures that the map entry value is a sorted set.
>> So we can add one new method to add an entry to speciesTypes,
>> invokeTypes and callSiteTypes which performs the validation and then
>> add to the corresponding collection.
>>
>> I think that way the code will be cleaner.  Below is something for
>> your reference.
>>
>> Mandy
>>
>> diff --git a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/GenerateJLIClassesPlugin.java b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/GenerateJLIClassesPlugin.java
>> --- a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/GenerateJLIClassesPlugin.java
>> +++ b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/GenerateJLIClassesPlugin.java
>> @@ -75,13 +75,13 @@
>>       private static final JavaLangInvokeAccess JLIA
>>               = SharedSecrets.getJavaLangInvokeAccess();
>>   
>> -    Set<String> speciesTypes = Set.of();
>> +    private final TreeSet<String> speciesTypes = new TreeSet<>();
>>   
>> -    Set<String> invokerTypes = Set.of();
>> +    private final TreeSet<String> invokerTypes = new TreeSet<>();
>>   
>> -    Set<String> callSiteTypes = Set.of();
>> +    private final TreeSet<String> callSiteTypes = new TreeSet<>();
>>   
>> -    Map<String, Set<String>> dmhMethods = Map.of();
>> +    private final Map<String, Set<String>> dmhMethods = new TreeMap<>();
>>   
>>       String mainArgument;
>>   
>> @@ -187,21 +187,31 @@
>>           mainArgument = config.get(NAME);
>>       }
>>
>> +    private void addSpeciesType(String type) {
>> +        speciesTypes.add(expandSignature(type));
>> +    }
>> +
>> +    private void addInvokerType(String methodType) {
>> +        validateMethodType(methodType);
>> +        invokerTypes.add(methodType);
>> +    }
>> +
>> +    private void addCallSisterType(String csType) {
>> +        validateMethodType(csType);
>> +        callSiteTypes.add(csType);
>> +    }
>> +
>>       public void initialize(ResourcePool in) {
>>           // Start with the default configuration
>> -        speciesTypes = defaultSpecies().stream()
>> -                .map(type -> expandSignature(type))
>> -                .collect(Collectors.toSet());
>> +        defaultSpecies().stream().forEach(this::addSpeciesType);
>>   
>> -        invokerTypes = defaultInvokers();
>> -        validateMethodTypes(invokerTypes);
>> +        defaultInvokers().stream().forEach(this::validateMethodType);
>>   
>> -        callSiteTypes = defaultCallSiteTypes();
>> +        defaultCallSiteTypes().stream().forEach(this::addCallSisterType);
>>   
>> -        dmhMethods = defaultDMHMethods();
>> -        for (Set<String> dmhMethodTypes : dmhMethods.values()) {
>> -            validateMethodTypes(dmhMethodTypes);
>> -        }
>> +        defaultDMHMethods().entrySet().stream().forEach(e -> {
>> +            e.getValue().stream().forEach(type -> addDMHMethodType(e.getKey(), type))
>> +        });
>>   
>>           // Extend the default configuration with the contents in the supplied
>>           // input file - if none was supplied we look for the default file
>> @@ -225,18 +235,6 @@
>>       }
>>
>>   
>>       private void readTraceConfig(Stream<String> lines) {
>> -        // Use TreeSet/TreeMap to keep things sorted in a deterministic
>> -        // order to avoid scrambling the layout on small changes and to
>> -        // ease finding methods in the generated code
>> -        speciesTypes = new TreeSet<>(speciesTypes);
>> -        invokerTypes = new TreeSet<>(invokerTypes);
>> -        callSiteTypes = new TreeSet<>(callSiteTypes);
>> -
>> -        TreeMap<String, Set<String>> newDMHMethods = new TreeMap<>();
>> -        for (Map.Entry<String, Set<String>> entry : dmhMethods.entrySet()) {
>> -            newDMHMethods.put(entry.getKey(), new TreeSet<>(entry.getValue()));
>> -        }
>> -        dmhMethods = newDMHMethods;
>>           lines.map(line -> line.split(" "))
>>                .forEach(parts -> {
>>                   switch (parts[0]) {
>> @@ -245,19 +243,18 @@
>>                           if (parts.length == 3 && parts[1].startsWith("java.lang.invoke.BoundMethodHandle$Species_")) {
>>                               String species = parts[1].substring("java.lang.invoke.BoundMethodHandle$Species_".length());
>>                               if (!"L".equals(species)) {
>> -                                speciesTypes.add(expandSignature(species));
>> +                                addSpeciesType(species);
>>                               }
>>                           }
>>                           break;
>>                       case "[LF_RESOLVE]":
>>                           String methodType = parts[3];
>> -                        validateMethodType(methodType);
>>                           if (parts[1].equals(INVOKERS_HOLDER_NAME)) {
>>                               if ("linkToTargetMethod".equals(parts[2]) ||
>>                                       "linkToCallSite".equals(parts[2])) {
>> -                                callSiteTypes.add(methodType);
>> +                                addCallSisterType(methodType);
>>                               } else {
>> -                                invokerTypes.add(methodType);
>> +                                addInvokerType(methodType);
>>                               }
>>                           } else if (parts[1].contains("DirectMethodHandle")) {
>>                               String dmh = parts[2];
>> @@ -291,12 +288,6 @@
>>           }
>>       }
>>   
>> -    private void validateMethodTypes(Set<String> dmhMethodTypes) {
>> -        for (String type : dmhMethodTypes) {
>> -            validateMethodType(type);
>> -        }
>> -    }
>>
>>       private void validateMethodType(String type) {
>>           String[] typeParts = type.split("_");
>>           // check return type (second part)
>> @@ -338,12 +329,6 @@
>>   
>>           // Generate LambdaForm Holder classes
>>           generateHolderClasses(out);
>> -
>> -        // Let it go
>> -        speciesTypes = null;
>> -        invokerTypes = null;
>> -        dmhMethods = null;
>> -
>>           return out.build();
>>       }
>>