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

[Jie Fu]

Nice catch!

Thank you so much, Mandy.

On 2019/10/30 上午1:19, Mandy Chung wrote:
> 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();
>>>       }
>>>
>