RFR(M): 8165896: Use "open" flag from JVM_DefineModule to export all module packages

[Rachel Protacio]

Thanks, Lois!

On 3/28/2017 10:31 AM, Lois Foltan wrote:
> Hi Rachel,
>
> I think this change looks good.  A couple of comments:
>
> - The additional check in PackageEntry::set_exported() at line #99 is 
> not necessary because the call at line #94, is_unqual_exported(), now 
> has an new check for an open module.
Thanks for catching this.
>
> - PackageEntry::set_is_exported_allUnnamed() should not include a 
> check for open unless the corresponding 
> PackageEntry::is_exported_allUnnamed() contains a similar check. The 
> concept that a package is exported to all unnamed modules was to 
> optimize the case where there would have been explicit qualified 
> exports to each unnamed module on the defining package's exports 
> list.  Also note that PackageEntry::is_qexported_to(ModuleEntry* m) 
> and PackageEntry::is_qual_exported() considers a package that has been 
> exported to allUnnamed() to be a qualifiedly exported.  So again if 
> there is a check for open it has to been done consistently on the 
> setter and getter methods of allUnnamed().
Ok, I added the check to is_exported_allUnnamed() and also 
set_unqual_exported().
>
> - Consider adding an assert before line #302 in classfile/modules.cpp, 
> that for java.base, is_open should not be true.
Added.

I'll just hold on to my changes for now and send them out again once all 
the jigsaw stuff has been integrated, since I'll likely have to update a 
few things.

Thanks,
Rachel
>
> Thanks,
> Lois
>
> On 3/23/2017 2:23 PM, Rachel Protacio wrote:
>> Hello!
>>
>> Please review this enhancement, which implements the VM side of open 
>> modules. Open modules export all their packages unqualifiedly, which 
>> was previously done by exporting the packages one by one on the java 
>> side. This change saves that step by adding an internal "open" 
>> representation for the module structure, which is checked before 
>> checking the exportedness of the package itself.
>>
>> JDK link: https://bugs.openjdk.java.net/browse/JDK-8165896
>>
>> hotspot webrev: 
>> http://cr.openjdk.java.net/~rprotacio/8165896.hotspot.00/
>> jdk webrev: http://cr.openjdk.java.net/~rprotacio/8165896.jdk.00/
>> root repo patch:
>>
>> --- old/test/lib/sun/hotspot/WhiteBox.java    2017-03-23 
>> 12:50:36.869963040 -0400
>> +++ new/test/lib/sun/hotspot/WhiteBox.java    2017-03-23 
>> 12:50:36.421963021 -0400
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2012, 2016, Oracle and/or its affiliates. All 
>> rights reserved.
>> + * Copyright (c) 2012, 2017, Oracle and/or its affiliates. All 
>> rights reserved.
>>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>   *
>>   * This code is free software; you can redistribute it and/or modify it
>> @@ -449,8 +449,8 @@
>>    }
>>
>>    // Jigsaw
>> -  public native void DefineModule(Object module, String version, 
>> String location,
>> -                                  Object[] packages);
>> +  public native void DefineModule(Object module, boolean is_open, 
>> String version,
>> +                                  String location, Object[] packages);
>>    public native void AddModuleExports(Object from_module, String 
>> pkg, Object to_module);
>>    public native void AddReadsModule(Object from_module, Object 
>> source_module);
>>    public native void AddModulePackage(Object module, String pkg);
>>
>>
>> Testing:
>>
>>  * includes new regression test
>>  * JPRT and RBT hotspot tiers 2-5
>>
>> Notes:
>>
>>  * This changeset re-conceptualizes the unnamed module as an open
>>    module (as far as the internal representation is concerned). This
>>    saves the VM from specifying every package in the unnamed module as
>>    unqualifiedly exported.
>>  * Additionally, automatic modules are defined as open modules in terms
>>    of package exports. As such, in the jdk repo's Module.java code,
>>    defineModule0() is called with isOpen==true for automatic modules.
>>  * In my search through module startup code, I found a for-loop in
>>    modules.cpp that was looping through java.base packages just for
>>    some logging. The entire loop should have been bounded by a
>>    log_is_enabled() conditional. Since this patch is generally
>>    improving the efficiency of modules startup, I corrected that here.
>>  * Fixed a few comment typos I found along the way.
>>
>> Thank you!
>> Rachel
>