8006965: remove test_gamma and add dedicated test_* targets instead [Was: Re: RFR (S): 8006965: test_gamma should run with import JDK]

Christian Thalinger christian.thalinger at oracle.com
Wed Mar 20 16:56:19 PDT 2013 

On Mar 20, 2013, at 3:42 PM, John Coomes <John.Coomes at oracle.com> wrote:

> Christian Thalinger (christian.thalinger at oracle.com) wrote:
>> 
>> On Mar 19, 2013, at 7:16 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>> 
>>> Add new test_* targets to default targets to execute a test when you do 'make all_fastdebug' for example.
>> 
>> Yesterday we thought that's a good idea but it's a problem for cross-compilations (e.g. JPRT uses all_* targets).  So I'd rather not add test_* targets to all_* targets.
> 
> I agree, better to avoid it.
> 
>>> May be it is time to let Queens go and use -help as you suggested
>> 
>> Some data on using -help instead of Queens:
>> 
>> - C2 compiles one method when running Queens and two methods for -help
>> - C1 compiles around 44 for Queens and around 20 for -help
>> - tiered compiles 73 for Queens and 33 for -help
>> 
>> So it's about the same smoke test for C2 but half for C1.  Which is think is enough.
>> 
>> None of both do any GC testing.
> 
> They're doing so little testing that it should be easy to just let
> them go.  But the targets will be useful when we later define a better
> set of warm-and-breathing tests that are invoked using test/Makefile
> (even if those require installing the jvm into a jdk, as long as that
> is automated).
> 
>>> or 'cd test; make VM_FLAVOR=$@ servertest' (as JPRT does).
>> 
>> No, that doesn't work because you need to install the libjvm into a JDK for running that test.
>> 
>> Here is a new webrev:
>> 
>> http://cr.openjdk.java.net/~twisti/8006965/
> 
> This looks good to me.  Thanks for persisting.

Thank you.  By your request I changed the synopsis of the bug:

8006965: remove test_gamma and add dedicated test_* targets instead

-- Chris

> 
> -John
> 
>>> On 3/19/13 6:16 PM, Christian Thalinger wrote:
>>>> 
>>>> On Mar 13, 2013, at 4:44 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>> 
>>>>> 
>>>>> On Feb 26, 2013, at 6:09 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On Feb 25, 2013, at 11:24 PM, David Holmes <David.Holmes at oracle.com> wrote:
>>>>>> 
>>>>>>> On 26/02/2013 4:42 AM, Christian Thalinger wrote:
>>>>>>>> On Feb 24, 2013, at 2:54 PM, David Holmes <David.Holmes at oracle.com> wrote:
>>>>>>>>> On 23/02/2013 1:55 PM, Christian Thalinger wrote:
>>>>>>>>>> I talked to a lot of people about this today.  What we really want is to not run tests when we build.  Mikael and I were looking into how we could do that without gamma and there is a way:
>>>>>>>>>> 
>>>>>>>>>> http://cr.openjdk.java.net/~twisti/8006965/
>>>>>>>>>> 
>>>>>>>>>> This would be the first of three fixes:
>>>>>>>>>> 
>>>>>>>>>> Fix 1)  The patch above removes test_gamma and uses some weirdness in the VM (-Dsun.java.launcher=gamma) to run it with an existing JDK; add test_{product,fastdebug,debug} targets
>>>>>>>>> 
>>>>>>>>> This logic is not suitable:
>>>>>>>>> 
>>>>>>>>> 541 # Testing the built JVM
>>>>>>>>> 542 ifeq ($(JAVA_HOME),)
>>>>>>>>> 543   RUN_JVM=JAVA_HOME=$(JDK_IMPORT_PATH) $(JDK_IMPORT_PATH)/bin/java -d$(ARCH_DATA_MODEL) -server -XXaltjvm=$(MISC_DIR)/$(VM_FLAVOR) -Dsun.java.launcher=gamma
>>>>>>>>> 544 else
>>>>>>>>> 545   RUN_JVM=$(JAVA_HOME)/bin/java -d$(ARCH_DATA_MODEL) -server -XXaltjvm=$(MISC_DIR)/$(VM_FLAVOR) -Dsun.java.launcher=gamma
>>>>>>>>> 546 endif
>>>>>>>>> 
>>>>>>>>> I have JAVA_HOME set in my environment for use by other tools/scripts and it points at JDK7. The existing logic does not use my environments JAVA_HOME setting so neither should the revised logic!
>>>>>>>> 
>>>>>>>> That's not entirely correct.  test_gamma uses your JAVA_HOME setting:
>>>>>>> 
>>>>>>> This is so confusing. Our makefiles are an abomination!
>>>>>> 
>>>>>> I couldn't agree more.
>>>>>> 
>>>>>>> 
>>>>>>> So this all started because the makefile has:
>>>>>>> 
>>>>>>> JAVA_HOME=$(ABS_BOOTDIR)
>>>>>>> 
>>>>>>> which was flagged as wrong because gamma would run in the boot JDK. But now it seems the make variable JAVA_HOME is irrelevant anyway because the test_gamma script will use the JAVA_HOME environment variable.
>>>>>>> 
>>>>>>> So how did the boot JDK come back into this???
>>>>>>> 
>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ export JAVA_HOME=/java/re/jdk/8/latest/binaries/linux-x64
>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ ./test_gamma
>>>>>>>> Using java runtime at: /java/re/jdk/8/latest/binaries/linux-x64/jre
>>>>>>>> <snip>
>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ export JAVA_HOME=/foo
>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ ./test_gamma
>>>>>>>> JAVA_HOME must point to a 64-bit OpenJDK.
>>>>>>>> 
>>>>>>>> And here comes this little snippet into play:
>>>>>>>> 
>>>>>>>> -MAKE_ARGS += JAVA_HOME=$(ABS_BOOTDIR)
>>>>>>>> +MAKE_ARGS += BOOTDIR=$(ABS_BOOTDIR)
>>>>>>>> 
>>>>>>>> Only setting JAVA_HOME to ABS_BOOTDIR make test_gamma work even if you have a JAVA_HOME set.
>>>>>>> 
>>>>>>> I still don't get this. What has BOOTDIR got to do with JAVA_HOME? Where is this BOOTDIR value being used?
>>>>> 
>>>>> Ha!  I can remember.  It's used by jmvti.make (implicitly).  The logic in rules.make is:
>>>>> 
>>>>> ifdef ALT_BOOTDIR
>>>>> 
>>>>> COMPILE.JAVAC = $(ALT_BOOTDIR)/bin/javac
>>>>> 
>>>>> else
>>>>> 
>>>>> ifdef BOOTDIR
>>>>> 
>>>>> COMPILE.JAVAC = $(BOOTDIR)/bin/javac
>>>>> 
>>>>> else
>>>>> 
>>>>> ifdef JAVA_HOME
>>>>> else
>>>>> 
>>>>> endif
>>>>> endif
>>>>> endif
>>>>> 
>>>>> BOOTDIR is not defined in rules.make and JAVA_HOME is picked up (which is set to ABS_BOOTDIR).
>>>> 
>>>> Back to my favorite review these days :-)  I put the BOOTDIR setting back because we need it.
>>>> 
>>>> Any reviewers who approve or veto pushing this change?
>>>> 
>>>> -- Chris
>>>> 
>>>>> 
>>>>> This all sucks and needs to be replaced by something completely different.  Soon.
>>>>> 
>>>>> -- Chris
>>>>> 
>>>>>>> There is no use of it in http://cr.openjdk.java.net/~twisti/8006965/8006965.patch and I don't see it pre-existing ??
>>>>>> 
>>>>>> I talked to John Coomes about that yesterday and we can remove that line.  ABS_BOOTDIR is only used by Windows.
>>>>>> 
>>>>>> -- Chris
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> 
>>>>>>>> I have added this logic so that users can control what JDK is used when running the test.  In fact they should use ALT_JDK_IMPORT_PATH if they want to control that.
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> I also don't see why the above sets JAVA_HOME at #543 - what will read that environment variable?
>>>>>>>> 
>>>>>>>> It's the odd logic in os::jvm_path guarded by Arguments::created_by_gamma_launcher().  A clean-up of that logic would be part of Fix 3.
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> I still have concerns over what JDK_IMPORT_PATH will point to for different JDK builders.
>>>>>>>> 
>>>>>>>> It's either JDK_IMPORT_PATH or JDK_IMAGE_DIR.  Since most people don't want to export their libjvm into a JDK we have to use JDK_IMPORT_PATH.  We could add some logic that checks if JDK_IMAGE_DIR exists and use that one.
>>>>>>>> 
>>>>>>>> -- Chris
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> And this addition still makes no sense to me:
>>>>>>>>> 
>>>>>>>>> MAKE_ARGS += BOOTDIR=$(ABS_BOOTDIR)
>>>>>>>>> 
>>>>>>>>> Why do you need to add BOOTDIR to the MAKE_ARGS?
>>>>>>>>> 
>>>>>>>>> David
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> Fix 2)  Remove gamma and all the ugly code that comes with it (copies of the jdk launcher in hotspot and other pieces); make the hotspot script work like the test targets in Fix 1
>>>>>>>>>> 
>>>>>>>>>> Fix 3)  Remove the -Dsun.java.launcher=gamma and possibly replace the existing -Dsun.boot.library.path weirdness by explicit command line options like -Xbootlibrarypath:{/p,/a}
>>>>>>>>>> 
>>>>>>>>>> -- Chris
>>>>>>>>>> 
>>>>>>>>>> On Feb 22, 2013, at 3:21 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> On Feb 22, 2013, at 12:58 AM, Staffan Larsen <staffan.larsen at oracle.com> wrote:
>>>>>>>>>>> 
>>>>>>>>>>>> I'm not sure what the correct solution is, but when you do find out, the jdkpath.sh target should also be updated.
>>>>>>>>>>> 
>>>>>>>>>>> How many are actually using the hotspot script?  Would people be very sentimental if we would remove the gamma launcher altogether?
>>>>>>>>>>> 
>>>>>>>>>>> Taking to people here it seems that most are copying their libjvm into a JDK and use java anyway.
>>>>>>>>>>> 
>>>>>>>>>>> -- Chris
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> /Staffan
>>>>>>>>>>>> 
>>>>>>>>>>>> On 22 feb 2013, at 03:40, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>>> http://cr.openjdk.java.net/~twisti/8006965
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 8006965: test_gamma should run with import JDK
>>>>>>>>>>>>> Reviewed-by:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Right now test_gamma runs with the boot JDK which is JDK n-1 (where
>>>>>>>>>>>>> JDK n is the version we are actually compiling for).  This setup is
>>>>>>>>>>>>> unsupported and thus should not be done during HotSpot builds.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> The fix is to always use JDK_IMPORT_PATH instead of JAVA_HOME when
>>>>>>>>>>>>> running test_gamma.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> make/bsd/makefiles/buildtree.make
>>>>>>>>>>>>> make/defs.make
>>>>>>>>>>>>> make/linux/makefiles/buildtree.make
>>>>>>>>>>>>> make/solaris/makefiles/buildtree.make
>>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>

More information about the hotspot-dev mailing list