8156871: Possible concurrency issue with JVM_AddModuleExports

[Lois Foltan]

On 6/16/2016 12:06 PM, Christian Tornqvist wrote:
> Hi Lois,
>
> In /test/runtime/modules/ModuleStress/src/jdk.test/test/Main.java:
>
>    73         // Repeatedly create the layer above stressing the exportation of
>    74         // package jdk.test/test to several different modules.
>    75         ExecutorService pool = Executors.newFixedThreadPool(100);
>
> I'm worried about the fixed number 100, especially for devices with a low number of cores (and potentially on Windows with 32bit address space). Could you put a limit on this based on min(100, Runtime.availableProcessors() * something (10?)) ? I don't need to see a new webrev for this.
Done.
>
> Otherwise the test looks good, thanks for taking care of the testng issue.
Thank you Christian for the review!
Lois
>
> Thanks,
> Christian
>
> -----Original Message-----
> From: Lois Foltan [mailto:lois.foltan at oracle.com]
> Sent: Thursday, June 16, 2016 10:48 AM
> To: Christian Tornqvist <christian.tornqvist at oracle.com>
> Cc: 'hotspot-runtime-dev' <hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: 8156871: Possible concurrency issue with JVM_AddModuleExports
>
>
> On 6/16/2016 9:37 AM, Christian Tornqvist wrote:
>> Hi Lois,
>>
>> Ok, I see why @compile won't work for you. We should probably look at moving the CompilerUtils code into one of the shared testlibrary classes at some point, I don't think your test is the last that will need this functionality.
>>
>> As for testng, you can avoid using testng by simply changing the testExportModuleStress() method to be the main method and calling compileAll() from that.
> Hi Christian,
>
> I have removed the use of testng as you suggest and moved CompilerUtils.java up one level to the hotspot/test/runtime/modules directory so it can be used to develop other module tests until we have a chance to move it into the shared testlibrary classes.
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8156871.4/webrev/
>
> I believe I have the required reviews for the functional piece of this, so just the test needs to be reviewed.
>
> Thanks!
> Lois
>
>> Thanks,
>> Christian
>>
>> -----Original Message-----
>> From: Lois Foltan [mailto:lois.foltan at oracle.com]
>> Sent: Wednesday, June 15, 2016 11:39 AM
>> To: Christian Tornqvist <christian.tornqvist at oracle.com>
>> Cc: 'hotspot-runtime-dev' <hotspot-runtime-dev at openjdk.java.net>
>> Subject: Re: 8156871: Possible concurrency issue with
>> JVM_AddModuleExports
>>
>>
>> On 6/15/2016 9:32 AM, Christian Tornqvist wrote:
>>> Hi Lois,
>>>
>>> I briefly looked at the test code and I have some concerns. I see the
>>> CompilerUtils class, what prevents you from using @compile in jtreg?
>>>
>>> Also, it's a testng test, in Runtime we've decided to not use testng
>>> to write our tests, is there a reason for using testng here?
>> Hi Christian,
>>
>> Thanks for reviewing the test.  CompilerUtils provides a very
>> convenient way prior to the test being run, to compile a module
>> declaration from an exploded build.  All that is needed is to provide
>> the module name and the directory location of the top of the exploded
>> directory that contains that module declaration to the CompilerUtils::compile method.
>> And it allows for providing the directory where the module definition
>> once compiled, should be located.  As we develop more and more
>> extensive module tests, I think it would be unfortunate to have to
>> manually go through and need to specify for every java file that makes up that module declaration:
>>
>>        @compile src/jdk.test/module-info.java
>>        @compile src/jdk.test/test/Main.java
>>        @compile src/jdk.translet/module-info.java
>>        @compile src/jdk.translet/translet/Main.java
>>
>> And these are just simple module declarations!  If there is a better
>> way to do this without the CompilerUtils functionality please let me
>> know, I would be happy to change it.  testng provided a way to run
>> CompilerUtils::compile prior to the actual test run.  There is
>> precedence for this in other areas of the hotspot/test directory.
>>
>> Thanks,
>> Lois
>>
>>> Thanks,
>>> Christian
>>>
>>> -----Original Message-----
>>> From: hotspot-runtime-dev
>>> [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of
>>> Lois Foltan
>>> Sent: Friday, June 10, 2016 10:25 AM
>>> To: hotspot-runtime-dev <hotspot-runtime-dev at openjdk.java.net>
>>> Subject: RFR: 8156871: Possible concurrency issue with
>>> JVM_AddModuleExports
>>>
>>> Hello,
>>>
>>> Please review the work to finalize a concurrency issue when setting
>>> the exported state of a PackageEntry.  The work completed in bug
>>> https://bugs.openjdk.java.net/browse/JDK-8152404 actually had the
>>> side effect of fixing this bug as well.  Prior to JDK-8152404, each
>>> PackageEntry determined its exported state via two flags, one of
>>> which was a general _is_exported flag followed by a another more
>>> specific state of exportability flag that determined if the package
>>> was exported qualifiedly or not to a given module.  Checking these
>>> two flags, as the
>>> PackageEntry::is_* methods used to do without the Module_lock, was
>>> problematic and yielded a situation where a call to add a module to a
>>> PackageEntry's qualified exported entry list failed because it was
>>> determined that the package was unqualifiedly exported when it really
>>> was not.
>>>
>>> To complete this fix, I have removed the is_unqual_exported call
>>> prior to setting a PackageEntry's exported list.  The method
>>> PackageEntry::set_exported determines what the current package export
>>> state is and acts correctly.  Also, the test case has been added in
>>> this webrev since it is a good stress test case for JVM module support.
>>>
>>> Passes JPRT, java/lang, java/util RBT hotspot nightly.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8156871
>>> Open webrev:
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8156871/webrev/
>>>
>>> Thanks,
>>> Lois
>>>
>