Hi, On Sep 12, 2014, at 12:45 PM, Igor Ignatyev <igor.ignat...@oracle.com> wrote:
> Paul, > > thanks for your review. I'll take care about this change, since Konstantin is > on vacation. > Thanks. > updated webrev: > http://cr.openjdk.java.net/~iignatyev/kshefov/8057719/webrev.00/ > > please see inline answers. > > Igor > > On 09/11/2014 05:12 PM, Paul Sandoz wrote: >> >> On Sep 5, 2014, at 7:52 PM, Konstantin Shefov <konstantin.she...@oracle.com> >> wrote: >> >>> Hello, >>> >>> Please review the new tests for the feature "Lambda Form Reduction and >>> Caching" https://bugs.openjdk.java.net/browse/JDK-8046703 >>> >>> JBS task: https://bugs.openjdk.java.net/browse/JDK-8057719 >>> >>> Webrev: http://cr.openjdk.java.net/~kshefov/8057719/webrev.00/ >>> >>> These tests also depend on testlibrary change: >>> https://bugs.openjdk.java.net/browse/JDK-8057707 >>> Webrev of the testlib change: >>> http://cr.openjdk.java.net/~kshefov/8057707/webrev.00/ >>> >> >> Generally looks good. It would be interesting see code-coverage results. >> >> Are you sure the LFMultiThreadCachingTest is sufficiently testing the >> thread-safety of j.l.invoke classes? It might be that more threads need to >> be generated (== #cores), and the test repeatedly performed in a loop to >> increase the chance of stuff stomping on each other. (I see you have >> iterations in the outer loop of runTests, that might be sufficient, but you >> might need a tighter loop specific to each test in LFMultiThreadCachingTest). >> > good point, I updated LFMultiThreadCachingTest to use more threads, however I > doubt that we need to add a loop. Writing additional MT-stress tests, which > can provide more confidence on thread-safety, is out of the scope of > JDK-8057719. > It does not have to be of jcstress-test-like quality, but if one is trying to test some degree concurrent execution I think it worthwhile having a simple iteration for loop in LFMultiThreadCachingTest: for (int iter = 0; i < ITERS; i++) { ... } balanced by default to not unduly make the test run too long. Plus often it is useful to define a sys prop for ITER so one can manually run it for longer periods. Up to you, perhaps consider it has a possible enhancement, esp. if you wanna get this in quickly. >> >> A general comment, feel free to ignore. It's easy to use EnumSet to obtain a >> collection of enum values: >> >> Set<TestMethods> tms = EnumSet.allOf(TestMethods.class) >> >> and filtered: >> >> Set<TestMethods> tms = >> EnumSet.complementOf(EnumSet.of(TestMethods.IDENTITY, TestMethods.CONSTANT)) >> >> (Quite concise with a static import.) >> >> Change LambdaFormTestCase.runTests to accept a Collection<TestMethods> and >> you are good to go :-) > done +1, ok to push. Paul.
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
