RFR(S) : 8242314 : use reproducible random in vmTestbase shared code

Tue Apr 28 04:15:49 UTC 2020


> On Apr 25, 2020, at 9:22 PM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
> 
> 
> 
>> On Apr 25, 2020, at 8:45 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> 
>>> On Apr 25, 2020, at 9:01 PM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
>>> 
>>> 
>>>> On Apr 25, 2020, at 4:57 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>> 
>>>>> On Apr 22, 2020, at 2:03 PM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
>>>>> 
>>>>> http://cr.openjdk.java.net/~iignatyev/8242314/webrev.00/ 
>>>>>> 122 lines changed: 71 ins; 19 del; 32 mod;
>>>>> 
>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> could you please review this small patch which updates shared vmTestbase  test code to use j.t.l.Utils.getRandomInstance() as a random number generator or as a source of seed values? this patch lays the ground work for updates of :vmTestbase_* test groups which are to be done by remaining subtasks of 8241623[1], such split make it possible to work on/review/test testgroup-specific changes in concurrently.  
>>>>> 
>>>>> unfortunately, such extra dependency on /test/lib classes broke the shaky balance and caused some tests to fail due to "split testlibrary" problem (see 8188828[2] for the definition), the solution to that is to remove all dependencies on /test/lib classes from ExecDriver class, so the patch also includes "inlining" of several jdk.test.lib.Utils and Platform methods/fields into ExecDriver.
>>>>> 
>>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8242314/webrev.00/ 
>>>>> testing: all :vmTestbase_* test groups on {linux,windows,macosx}-x64 and solaris-sparcv9
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8242314
>>>>> 
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8241623
>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8188828
>>>>> 
>>>>> Thanks,
>>>>> -- Igor
>>>> 
>>>> The test changes look fine.
>>>> 
>>>> But I don't feel like I understand the "split library" problem well
>>>> enough to review the ExecDriver changes.  I will say that I don't like
>>>> the solution of copying code, and hope there is a better way.  Is
>>>> there a bug for the problem?  It seems like it could crop up again,
>>>> perhaps repeatedly, in which case the code copying becomes increasingly
>>>> bad.  And it might be a good idea to leave some breadcrumbs associated
>>>> with this code copying workaround, so an eventual better fix could also
>>>> cleanup the duplication.
>>>> 
>>> 
>>> Hi Kim,
>>> 
>>> thanks for look at this!
>>> 
>>> "split library" problem occur when a test "have these properties: 
>>> - has more than one @library 
>>> - depends on class Foo from library A which depends on class Bar from another library B 
>>> - has an implicit/explicit build for class Baz from libraries which depends on class Bar"[1]
>>> in this case A=/vmTestbase, B=/test/lib, Foo=ExecDriver, Bar=jdk.test.lib.Utils, and Baz can be pretty much any of class from jdk.test.lib, e.g. jdk.test.lib.process.ProcessTools.
>> 
>> Yes, I read that already, from JDK-8188828.  But I don't understand
>> the problem; remember, I'm not really a Java programmer, I just fake
>> it sometimes :)  In particular, I don't have a good understanding of
>> Java packaging and modularization and such.  So I'd rather not be
>> counted as having reviewed that part of the change.
> 
> well, a usual Java programmer will have a little understanding of the problem either since it's kinda unique to jtreg, or rather to how we use it. let me try to elaborate more on that. @library tag tells jtreg where to look for source code during compilation, later when jtreg invokes javac to compile java source code to java bytecode it puts the result depending on the location of the class which provoked compilation, that's to say if you have '@build Bar' (or `@run main Bar`), jtreg will search for Bar in current test source directory and in all @libraries, if Bar is found in test source, jtreg will use test-specific directory as destination directory; if Bar is found in a @library, jtreg will use the directory specific for this library, and this directory is shared b/w different tests. to find which particular dependency of Bar to build, jtreg relies on javac's ability to implicitly compile dependencies, and the bytecode of all dependencies will be placed in the same directory regardless of the origin of dependencies source code. it all works great, unless you have a situation described above, as in such cases, you might end up w/ some part of @library be compiled into one directory, and some part in a directory of another @library -- a so-called "split library" problem, so the next test which uses 1st @library but doesn't use the 2nd will fail to find class files of some classes from 1st @library. I hope it makes this at least a little bit clearer. there is also a very good analyze of this problem from Ioi in CODETOOLS-7901986 -- https://bugs.openjdk.java.net/browse/CODETOOLS-7901986?focusedCommentId=14108320&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14108320 <https://bugs.openjdk.java.net/browse/CODETOOLS-7901986?focusedCommentId=14108320&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14108320> <https://bugs.openjdk.java.net/browse/CODETOOLS-7901986?focusedCommentId=14108320&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14108320 <https://bugs.openjdk.java.net/browse/CODETOOLS-7901986?focusedCommentId=14108320&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14108320>>
> 
>> 
>>> there is a better way to fix that, which we discussed have discussed in the aftermath of our encounter of this problem (when 8188828 was used as a stopgap solution), and this way is to redesign/simplify and potentially modularize test libraries so it will be easier/saner to build them. unfortunately, we are not there yet, yet we are making progress; this activity is tracked by 8211289[2], 8211290[3], and 8211358[4].
>>> 
>>> as of breadcrumbs, I think I left mentions of the copied methods in all the methods, e.g. L#144; and the only copied pieces of code which don't have such breadcrumbs are TEST_CLASS_PATH and TEST_JDK static fields. as they are quite straightforward, I didn't feel that it's need to refer back to the original code. but if you like, I can add comments there as well.
>> 
>> Regarding the breadcrumb comments, I didn't recognize them as such.
>> Had they been something more than just a fully qualified method name,
>> i.e. something like "Copied from <fqname>", it would have been clear.
>> Is there a cleanup CR for later?
> sure I'll update comments and file an RFE to clean this up. 

filed https://bugs.openjdk.java.net/browse/JDK-8243927 <https://bugs.openjdk.java.net/browse/JDK-8243927>; and revamped the comments:
 - full webrev: http://cr.openjdk.java.net/~iignatyev//8242314/webrev.01
 - diff b/w 00 and 01: http://cr.openjdk.java.net/~iignatyev//8242314/webrev.0-1 <http://cr.openjdk.java.net/~iignatyev//8242314/webrev.0-1>

-- Igor
>> 
>>> Thanks,
>>> -- Igor 
>>> 
>>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8188828?focusedCommentId=14121228&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14121228 <https://bugs.openjdk.java.net/browse/JDK-8188828?focusedCommentId=14121228&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14121228> <https://bugs.openjdk.java.net/browse/JDK-8188828?focusedCommentId=14121228&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14121228 <https://bugs.openjdk.java.net/browse/JDK-8188828?focusedCommentId=14121228&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14121228>>
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8211289 <https://bugs.openjdk.java.net/browse/JDK-8211289><https://bugs.openjdk.java.net/browse/JDK-8211289 <https://bugs.openjdk.java.net/browse/JDK-8211289>>
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8211290 <https://bugs.openjdk.java.net/browse/JDK-8211290><https://bugs.openjdk.java.net/browse/JDK-8211290 <https://bugs.openjdk.java.net/browse/JDK-8211290>>
>>> [4] https://bugs.openjdk.java.net/browse/JDK-8211358 <https://bugs.openjdk.java.net/browse/JDK-8211358><https://bugs.openjdk.java.net/browse/JDK-8211358 <https://bugs.openjdk.java.net/browse/JDK-8211358>>