[9] Review request : JDK-8057719: Develop new tests for LambdaForm Reduction and Caching feature

[Paul Sandoz]

Hi,

On Sep 12, 2014, at 12:45 PM, Igor Ignatyev <igor.ignatyev at 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.shefov at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://mail.openjdk.java.net/pipermail/mlvm-dev/attachments/20140912/1f1defa0/signature.asc>