Merging the MacOS X Port into HotSpot Express 23 (7098194)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Oct 12 15:53:38 PDT 2011
> On 10/12/11 6:33 PM, Daniel D. Daugherty wrote:
>> Paul,
>>
>> Thanks for the thorough review. Replies embedded below.
>>
>> On 10/12/11 2:56 PM, Paul Hohensee wrote:
>>> The printf format thing is handled in globalDefinitions.hpp via
>>> system-dependent
>>> macros named INTX_FORMAT, PTR_FORMAT, etc. There should be no
>>> direct uses of
>>> printf format specifiers in Hotspot code, so the change to jni_x86.h
>>> should go
>>> away and any uses of %lld and %ld should be replaced by the
>>> appropriate FORMAT
>>> macro. If needed, you can use the platform-specific extensions to
>>> globalDefinitions.hpp
>>> in the cpu and utilities directories.
>>
>> Based on what Roger said, I suspect that if I remove the change to
>> jni_x86.h (typedef long long jlong on __APPLE__), then I'll have
>> some non-trivial amount of warnings fallout that I'll need to
>> deal with. I'm willing to do that, but not right now and not with
>> this changeset. The clock is ticking on getting this changeset
>> pushed and I don't consider this a showstopper.
>
> Ok. Another RFE.
Yes.
>
>>
>>
>>> If set_native_thread_name() is an implementation of an extension to
>>> Thread,
>>> then I'd recommend leaving it and the supporting code out (and
>>> there's a lot of
>>> it, vis jvm.cpp as you point out) until the core libs people figure
>>> out how
>>> they want to handle it. Maybe file an RFE for the work.
>>
>> If I remove that support, then I won't be able to use the resulting
>> VM with the rest of the macosx-port forest, right? Part of my
>> bring up strategy relies on my being able to use the HSX-23 port
>> bits with the rest of the macosx-port built bits...
>
> True. But it may not be used in it's current form.
I think it is:
jdk/src/share/javavm/export/jvm.h:JVM_SetNativeThreadName(JNIEnv *env,
jobject jthread, jstring name);
jdk/src/share/native/java/lang/Thread.c: {"setNativeName", "(" STR
")V", (void *)&JVM_SetNativeThreadName},
jdk/src/share/classes/java/lang/Thread.java: setNativeName(name);
jdk/src/share/classes/java/lang/Thread.java: private native void
setNativeName(String name);
Dan
>
> Thanks,
>
> Paul
>
>>
>>
>>> In debug.cpp, I'd make Debugging a product flag if you're going to
>>> make Command
>>> a product class. That way you don't need the #ifndef PRODUCT around
>>> its uses.
>>
>> Done.
>>
>>
>>> There's a use of "%#p" at line 481 in the Old file (487 in the New
>>> one) that should
>>> be replaced by PTR_FORMAT.
>>
>> Fixed.
>>
>>
>>> The comment on line 496 of the New version should
>>> be "!PRODUCT" rather than "PRODUCT".
>>
>> Fixed there and in other places in the file.
>>
>>
>>> You might change pss() to read
>>>
>>> 577 extern "C" void pss() { // print all stacks
>>> 578 if (!Thread::current()) return;
>>> 579 Command c("pss");
>>> 581 Threads::print(true, PRODUCT_ONLY(false) NOT_PRODUCT(true));
>>> 585 }
>>>
>>>
>>> It's perhaps a bit neater.
>>
>> Fixed the print() call on line 581.
>>
>>
>>> Re "!Thread::current()", the rule in hotspot is to make
>>> comparisons to null explicit, so you could use instead
>>> "Thread::current() != NULL",
>>> because what NULL is varies by platform.
>>
>> Fixed both !current() uses in the file.
>>
>> Dan
>>
>>
>>>
>>> Paul
>>>
>>> On 10/12/11 2:34 PM, roger hoover wrote:
>>>> As I promised Dan earlier this morning....replies embedded:
>>>>
>>>> On Oct 12, 2011, at 9:48 AM, Daniel D. Daugherty wrote:
>>>>
>>>>> David,
>>>>>
>>>>> Thanks for the thorough review (as always). Replies embedded below.
>>>>>
>>>>>
>>>>> On 10/11/11 10:48 PM, David Holmes wrote:
>>>>>> Hi Dan,
>>>>>>
>>>>>> src/cpu/x86/vm/jni_x86.h
>>>>>>
>>>>>> I don't understand why this change would be needed:
>>>>>>
>>>>>> ! #if defined(_LP64)&& !defined(__APPLE__)
>>>>>> typedef long jlong;
>>>>>> #else
>>>>>> typedef long long jlong;
>>>>>> #endif
>>>>>>
>>>>>> this is saying that on "apple" under _LP64 a long is not 64-bits.
>>>>>> But the very definition of LP64 is that longs and pointers are
>>>>>> 64-bits. ???
>>>>> $ cat sizes.c
>>>>> #include<stdio.h>
>>>>>
>>>>> int main() {
>>>>> printf("Hello world! _LP64=");
>>>>> #ifdef _LP64
>>>>> printf("%d\n", _LP64);
>>>>> #else
>>>>> printf("undefined\n");
>>>>> #endif
>>>>> printf("sizeof(long) = %d\n", (int) sizeof(long));
>>>>> printf("sizeof(long long) = %d\n", (int) sizeof(long long));
>>>>> }
>>>>>
>>>>> Output on my MacOS 10.6.8 machine:
>>>>>
>>>>> Hello world! _LP64=1
>>>>> sizeof(long) = 8
>>>>> sizeof(long long) = 8
>>>>>
>>>>> Output on my Solaris X86 machine:
>>>>>
>>>>> Hello world! _LP64=undefined
>>>>> sizeof(long) = 4
>>>>> sizeof(long long) = 8
>>>>>
>>>>> Output on my Solaris X86 machine when built with '-m64':
>>>>>
>>>>> Hello world! _LP64=1
>>>>> sizeof(long) = 8
>>>>> sizeof(long long) = 8
>>>>>
>>>>> Since __APPLE__ machines have a 8 byte long, I don't understand
>>>>> why the person thought that "long long jlong" was necessary...
>>>>>
>>>>> We need an Apple person to jump in here...
>>>>>
>>>> (answered before, but here it is again):
>>>> This is likely due to the fprintf format usage mess. The hotspot
>>>> code had assumed that any 64-bit fprintf format could be used with
>>>> any 64-bit value. Apple compilers give warnings if you print a
>>>> long with "%lld", and insists upon "%ld" for a clean compile even
>>>> though both are 64-bit values. Changing the type to long long
>>>> allows the format to remain unchanged.
>>>>
>>>> The correct way to fix this mess is to insure that the formats
>>>> exactly match the types used. We couldn't pull this off at apple
>>>> since we didn't have the capabilities to build under all of the
>>>> other os platforms to test the changes.
>>>>
>>>>>> src/os/<os>/vm/os_<os>.cpp
>>>>>>
>>>>>> + void os::set_native_thread_name(const char *name) {
>>>>>> + // Not yet implemented.
>>>>>> + return;
>>>>>> + }
>>>>>>
>>>>>> I hate seeing "shared" code that is not implemented on 3 out of 4
>>>>>> supported platforms. Though it seems Linux will also support
>>>>>> pthread_setname_np as of March 2010 (not sure which kernel or
>>>>>> glibc versions needed). If not for the fact this also adds an
>>>>>> exported JVM method to set the native thread name, we could
>>>>>> otherwise bury this in the platform specific native thread
>>>>>> creation code (which might also obviate the need for the new
>>>>>> _is_attached logic - see next point). I also wonder what Java
>>>>>> code is using this new JVM API?
>>>>> I suspect that this new API is used by other pieces of the MacOS X
>>>>> port project, but I don't know that for sure. Again, we need an
>>>>> Apple person to jump in here.
>>>> The native thread name addition was a huge win, both for us
>>>> debugging and for our developer customers. The only complaint we
>>>> had was from other projects whose code attached to Java and (in
>>>> initial revisions) had their threads renamed. It is exported so
>>>> that Thread.java can set the native thread name.
>>>>
>>>> I have no explanation for the lack of a return code. I can only
>>>> surmise that it is an oversight that was not caught by the compiler.
>>>>
>>>> The other pieces are:
>>>> macosx-port/jdk/src/share/native/java/lang/Thread.c:
>>>> {"getThreads", "()[" THD, (void *)&JVM_GetAllThreads},
>>>> {"dumpThreads", "([" THD ")[[" STE, (void
>>>> *)&JVM_DumpThreads},
>>>> --> {"setNativeName", "(" STR ")V", (void
>>>> *)&JVM_SetNativeThreadName},
>>>> };
>>>> -and-
>>>> macosx-port/jdk/src/share/classes/java/lang/Thread.java
>>>> /**
>>>> * Changes the name of this thread to be equal to the argument
>>>> *<code>name</code>.
>>>> *<p>
>>>> * First the<code>checkAccess</code> method of this thread is
>>>> called
>>>> * with no arguments. This may result in throwing a
>>>> *<code>SecurityException</code>.
>>>> *
>>>> * @param name the new name for this thread.
>>>> * @exception SecurityException if the current thread cannot
>>>> modify this
>>>> * thread.
>>>> * @see #getName
>>>> * @see #checkAccess()
>>>> */
>>>> public final void setName(String name) {
>>>> checkAccess();
>>>> this.name = name.toCharArray();
>>>> --> if (threadStatus != 0) {
>>>> --> setNativeName(name);
>>>> --> }
>>>> }
>>>>>
>>>>>> Also wondering where the "current thread only" restriction came
>>>>>> about?
>>>>> If the API is restricted to the "current thread", then we don't have
>>>>> to worry about races and locking. Also, I'm wondering why there is no
>>>>> return code.
>>>>>
>>>>>
>>>>>> src/share/vm/runtime/thread.hpp
>>>>>>
>>>>>> + // Remember whether or not we were attached
>>>>>> + bool _is_attached;
>>>>>>
>>>>>> It took me a while to realize that _is_attached really means "was
>>>>>> attached" ie that the Java thread came about due to a native
>>>>>> thread attaching to the VM via JNI attachCurrentThread. Could I
>>>>>> suggest this is renamed to was_attached?
>>>>> I was taking another look at this. The key user of the new flag is:
>>>>>
>>>>> src/share/vm/prims/jvm.cpp
>>>>>
>>>>> JVM_ENTRY(void, JVM_SetNativeThreadName(JNIEnv* env, jobject
>>>>> jthread, jstring n
>>>>> ame))
>>>>> JVMWrapper("JVM_SetNativeThreadName");
>>>>> ResourceMark rm(THREAD);
>>>>> oop java_thread = JNIHandles::resolve_non_null(jthread);
>>>>> JavaThread* thr = java_lang_Thread::thread(java_thread);
>>>>> // Thread naming only supported for the current thread, doesn't
>>>>> work for
>>>>> // target threads.
>>>>> if (Thread::current() == thr&& !thr->is_attached()) {
>>>>> // we don't set the name of an attached thread to avoid stepping
>>>>> // on other programs
>>>>> const char *thread_name =
>>>>> java_lang_String::as_utf8_string(JNIHandles::reso
>>>>> lve_non_null(name));
>>>>> os::set_native_thread_name(thread_name);
>>>>> }
>>>>> JVM_END
>>>>>
>>>>> which is coded to not set the name of a thread that has been
>>>>> attached. In the JavaThread(bool) constructor:
>>>>>
>>>>> _is_attached = _is_attaching = is_attaching;
>>>>>
>>>>> we set both flags to the "is_attaching" parameter (default false)
>>>>> so if we're coming down the JNI attach path, that parameter
>>>>> should be true so we set both flags to true. After the thread
>>>>> has been properly attached, we call set_attached() which sets
>>>>> the _is_attaching flag to false. Whew!
>>>>>
>>>>> In the JavaThread(ThreadFunction, size_t) constructor we set
>>>>> both flags to false because we're not attaching... and we
>>>>> didn't come down the JNI attach path.
>>>>>
>>>>> I don't like the "was attached" attribute and I no longer like
>>>>> the "is attaching" and "is attached" attributes either. Sigh.
>>>>> I think both fields need to be cleaned up and renamed. Perhaps
>>>>> something like:
>>>>>
>>>>> is_attaching_via_jni
>>>>> was_attached_via_jni
>>>>>
>>>>> would be more clear?
>>>>>
>>>>>
>>>>>> src/share/vm/utilities/debug.cpp
>>>>>>
>>>>>> Why are we exposing this debug code in a product build?
>>>>> We'll need an Apple person to jump in here...
>>>>>
>>>>>
>>>> It has been an Apple tradition (from long before I joined the Java
>>>> team in 2004) to include a basic set of debugging routines in
>>>> product builds. We call them from gdb while debugging problems
>>>> that don't show up in debug builds, and we've told our developer
>>>> customers about them. The rest of the Apple port does not depend
>>>> on these and whether or not to continue Apple's tradition is
>>>> totally up to Oracle.
>>>>
>>>>>> src/share/vm/prims/jni.cpp
>>>>>>
>>>>>> The USDT2 ifdef changes are truly ugly especially in this file.
>>>>>> Is there no way to hide USDT1 vs USDT2 at a lower level so that
>>>>>> the top-level probe points are not affected? I know this is
>>>>>> flagged for "future work" but history shows such cleans ups
>>>>>> rarely if ever happen - and this current code really is pretty
>>>>>> awful to look at.
>>>>> Yes, the #ifdef'ing makes the current code awful. I don't think
>>>>> anyone will dispute that observation.
>>>>>
>>>>> The better solution would be for Solaris to migrate to USDT2.
>>>>> I'm discussing that with Keith McGuigan. However, I don't want
>>>>> to take much of his time right now because he is hip deep in
>>>>> another alligator swamp at the moment.
>>>>>
>>>>> The best I can offer at the moment is filing a new bug to track
>>>>> the future work. Yes, I know that clean up type work rarely gets
>>>>> done, but this merge with the MacOS X port project needs to get
>>>>> into HSX-23 as soon as possible. Broken functionality in the
>>>>> other platforms is a stopper, but ugly is not a stopper.
>>>>>
>>>>> Thanks again for the thorough review.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>> On 12/10/2011 3:34 AM, Daniel D. Daugherty wrote:
>>>>>>> Greetings,
>>>>>>>
>>>>>>> Tom R shepherded the the BSD Port into HotSpot Express 23 using:
>>>>>>>
>>>>>>> 7089790 integrate bsd-port changes
>>>>>>>
>>>>>>> and I'm following up on that work using the following bug:
>>>>>>>
>>>>>>> 7098194 integrate macosx-port changes
>>>>>>>
>>>>>>> The synopsis is a bit off. In reality, the 7098194changeset will
>>>>>>> include additional changes from the BSD Port in addition the deltas
>>>>>>> made by the MacOS X Port. The bsd-port/hotspot tip changeset for
>>>>>>> this resync is:
>>>>>>>
>>>>>>> changeset: 2729:f1a18ada5853
>>>>>>> tag: tip
>>>>>>> user: Greg Lewis<glewis at eyesbeyond.com>
>>>>>>> date: Wed Sep 21 22:29:10 2011 -0700
>>>>>>> summary: . Finish removing hsearch_r.
>>>>>>>
>>>>>>> The macosx-port/hotspot changeset for this merge is:
>>>>>>>
>>>>>>> changeset: 2756:69de8d34a370
>>>>>>> tag: tip
>>>>>>> user: swingler at apple.com
>>>>>>> date: Thu Sep 29 18:10:16 2011 -0700
>>>>>>> summary: Adding JAVA_LIBRARY_PATH for bundled app launching
>>>>>>> (avoids stomping DYLD_LIBRARY_PATH)
>>>>>>>
>>>>>>> The focus of 7098194 is to get the MacOS X port into HSX-23 without
>>>>>>> regressing the non-MacOS X platforms. In other words, we're trying
>>>>>>> to get HSX-23 caught up with the BSD Port and MacOS X Port
>>>>>>> projects.
>>>>>>> Shaking out the MacOS X Port itself will be done with other
>>>>>>> changesets
>>>>>>> on an as needed basis.
>>>>>>>
>>>>>>> Just to be clear, the push target for this changeset is the
>>>>>>> RT_Baseline
>>>>>>> sub-repo of HotSpot Express (currently HSX-23):
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot
>>>>>>>
>>>>>>> Like what Tom did for 7089790, I'm posting multiple webrevs so
>>>>>>> folks
>>>>>>> can review these changes in different ways. My primary focus
>>>>>>> here is
>>>>>>> the common or shared code so I'm less worried about the BSD or
>>>>>>> MacOS X
>>>>>>> specific changes. Obviously, if you see something I messed up in
>>>>>>> those
>>>>>>> files, I'd like to know.
>>>>>>>
>>>>>>> Here's the webrev for all the changes in one shot:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-all/
>>>>>>>
>>>>>>>
>>>>>>> The order of the files in the above webrev is the same as for
>>>>>>> for the subset webrevs below.
>>>>>>>
>>>>>>> Here's the webrevs for the changes in subsets:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-bsd-resync/
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The above webrev has the changes to the bsd-port/hotspot repo made
>>>>>>> after Tom R's work on 7089790 (the 2011.08.02 wiki snapshot).
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-macosx-other-code/
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The above webrev has the non-Dtrace and non-infrastructure changes
>>>>>>> made for the MacOS X port. There's a couple of files that also
>>>>>>> contain
>>>>>>> Dtrace related changes, but I decided it was better to include
>>>>>>> those
>>>>>>> files in this subset.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-macosx-dtrace-code/
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The above webrev has almost all of the changes to enable Dtrace on
>>>>>>> MacOS X (a couple of files are in the macosx-other-code subset).
>>>>>>> On MacOS X, a newer version of Dtrace is implemented than on
>>>>>>> Solaris
>>>>>>> which is why the code is #ifdef'ed. I had to change the original
>>>>>>> #ifdef'ing because the original implementation had put some
>>>>>>> non-Dtrace
>>>>>>> code into #ifdef USDT1 ... #endif blocks so the code didn't
>>>>>>> build on
>>>>>>> non-Solaris platforms. In order to be more clean with #ifdef'ing, I
>>>>>>> redid all MacOS X Dtrace #ifdef'ing in the following forms:
>>>>>>>
>>>>>>> #ifndef USDT2
>>>>>>> <older Dtrace implementation is in this block>
>>>>>>> <some non-Dtrace code (macros) that call the older Dtrace>
>>>>>>> <implementation are also in this block>
>>>>>>> #else /* USDT2 */
>>>>>>> <new Dtrace implementation for MacOS X in this block>
>>>>>>> #endif /* USDT2 */
>>>>>>>
>>>>>>> #ifndef USDT2
>>>>>>> <older Dtrace implementation is in this block>
>>>>>>> <no #else because the newer Dtrace doesn't need/have the
>>>>>>> equivalent>
>>>>>>> #endif /* ! USDT2 */
>>>>>>>
>>>>>>> Yes, I realize that the MacOS X Dtrace implementation does not
>>>>>>> follow
>>>>>>> HotSpot style guidelines very consistently, but I decided not to
>>>>>>> fix
>>>>>>> that so I could diff against the macosx-port/hotspot more reliably.
>>>>>>>
>>>>>>> I have to consult with Keith McGuigan about how to migrate the
>>>>>>> Solaris
>>>>>>> Dtrace implementation to the newer version. However, that work
>>>>>>> will be
>>>>>>> done independently of this bug (7098194).
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/0-macosx-infra/
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The above webrev has all the infrastructure (e.g., Makefile)
>>>>>>> related
>>>>>>> changes. Many/most folks aren't interested in Makefile stuff so
>>>>>>> I've
>>>>>>> put these changes in their own subset. Of course, I need Kelly
>>>>>>> O'Hair
>>>>>>> to bless these changes...
>>>>>>>
>>>>>>> Builds done so far:
>>>>>>>
>>>>>>> - Solaris X86 builds of {Client, Server} VM * {product,
>>>>>>> fastdebug, jvmg}
>>>>>>> - WinXP builds of {Client, Server} VM * {product, fastdebug, debug}
>>>>>>> - MacOS 10.6.8/Xcode 3.2.6 build of the macosx-port forest with
>>>>>>> new HSX
>>>>>>> repo
>>>>>>> - MacOS 10.7/Xcode 4.1.1 build of the macosx-port forest with
>>>>>>> new HSX repo
>>>>>>>
>>>>>>> Testing done so far:
>>>>>>>
>>>>>>> - Serviceability stack (25 subsuites):
>>>>>>> VM/NSK: jvmti, jvmti_unit, jdwp, jdi, sa-jdi, heapdump, hprof, jdb,
>>>>>>> logging, mm
>>>>>>> SDK/JDK: jdi, jdi_closed, jli, logging, mm_csjmx, mm_csm, mm_jlm,
>>>>>>> mm_jlm_closed, mm_jmx, mm_jmx_closed, mm_sm, mm_sm_closed,
>>>>>>> misc_attach, misc_jvmstat, misc_tools
>>>>>>> - Tested configs (8 configs)
>>>>>>> {Solaris X86, WinXP} * {Client, Server} VM * {-Xmixed, -Xcomp}
>>>>>>>
>>>>>>> - MacOS X builds have only had minimal 'java -version' type
>>>>>>> testing,
>>>>>>> i.e., did the build "work"?
>>>>>>>
>>>>>>> No regressions have been seen in the Solaris X86 testing and WinXP
>>>>>>> testing should be finished later today.
>>>>>>>
>>>>>>> Things still to do (in no particular order):
>>>>>>>
>>>>>>> - gather the list of contributors for the changeset comment
>>>>>>> - JPRT test jobs when the JPRT-hotspotwest queue settles down
>>>>>>> - dtrace testing on Solaris X86
>>>>>>> - code review
>>>>>>> - start bring up of more formal dev testing on MacOS X
>>>>>>>
>>>>>>> Thanks, in advance, for any review comments.
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>
More information about the hotspot-dev
mailing list