[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: 8156871: Possible concurrency issue with JVM_AddModuleExports
From: Lois Foltan <lois.foltan () oracle ! com>
Date: 2016-06-16 17:44:16
Message-ID: 5762E570.5000805 () oracle ! com
[Download RAW message or body]
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@oracle.com]
> Sent: Thursday, June 16, 2016 10:48 AM
> To: Christian Tornqvist <christian.tornqvist@oracle.com>
> Cc: 'hotspot-runtime-dev' <hotspot-runtime-dev@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@oracle.com]
> > Sent: Wednesday, June 15, 2016 11:39 AM
> > To: Christian Tornqvist <christian.tornqvist@oracle.com>
> > Cc: 'hotspot-runtime-dev' <hotspot-runtime-dev@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@openjdk.java.net] On Behalf Of
> > > Lois Foltan
> > > Sent: Friday, June 10, 2016 10:25 AM
> > > To: hotspot-runtime-dev <hotspot-runtime-dev@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
> > >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic