Merging the MacOS X Port into HotSpot Express 23 (7098194)

Wed Oct 12 18:59:48 PDT 2011

Okay, I won't respond to all the past emails concerning my first round 
comments, but will start afresh here utilising the additional 
information that has been provided.

But first a new comment:

src/os_cpu/bsd_zero/vm/os_bsd_zero.cpp (and probably elsewhere)

  fatal(err_msg("pthread_attr_init failed with err = " INT32_FORMAT, rslt));

It is far more informative to provide the error string "strerror(rslt)" 
than the decimal value of the error code. (We're probably guilty of this 
in many places but lets not propagate bad habits.)

---

src/share/vm/prims/jni.cpp + thread.*

!   thread->set_done_attaching_via_jni();

I think the naming has gotten overly verbose here. The only way to 
attach is by definition via JNI. I don't think there was anything wrong 
with the existing _is_attaching logic (yes I added it! ;-) ) and my only 
complaint was that the new _is_attached should really be _was_attached. 
Perhaps a better/simpler/cleaer approach here would have been to have a 
field: can_set_native_thread_name, and only set it to true in the 
non-attaching case. Another cleanup RFE perhaps.

---

src/share/vm/utilities/debug.cpp

Normally if we propose to move something into a product build there is a 
discussion about what we are moving, why, and what the impact is on 
build size and runtime performance. I don't see any of that here. :(

---

- set_native_thread_name:

Ok so I have to go back to earlier emails here. Makes sense that 
Thread.setName is augmented to call JVM_SetNativeThreadName. However I 
don't like the semantics of this particular addition. setName has no 
usage restrictions on it, but set_native_thread_name is a no-op unless 
called by the current thread. By restricting it to the current thread it 
simplifies synchronization logic, but it seems a somewhat arbitrary 
restriction in terms of the functionality, given the more general nature 
of the setName API. I guess this is Yet Another RFE: 
set_native_thread_name should not be restricted to only the current thread.

BTW: are the JDK changes associated with this also being pushed at some 
stage? If so are the hotspot changes being pushed first?

Cheers,
David

On 13/10/2011 10:56 AM, Daniel D. Daugherty wrote:
> Greetings,
>
> I've closed out Code Review Round 0 and I'm posting new webrevs
> for Code Review Round 1:
>
> Here's the webrev for just the deltas between Code Review Round 0
> and Code Review Round 1:
>
> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-deltas-only
>
>
> Here's the webrev for all the changes in one shot:
>
> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-all/
>
> And here are the same subset webrevs:
>
> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-bsd-resync/
>
> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-macosx-other-code/
>
> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-macosx-dtrace-code/
>
> http://cr.openjdk.java.net/~dcubed/mac_os_x_port/7098194-webrev/1-macosx-infra/
>
>
>
> Here's my current commit message:
>
>> 7098194: integrate macosx-port changes
>> Summary: Integrate macosx-port/hotspot changes as of 2011.09.29.
>> Reviewed-by: kvn, dholmes, never, phh
>> Contributed-by: Christos Zoulas <christos at zoulas.com>, Greg Lewis
>> <glewis at eyesbeyond.com>, Kurt Miller <kurt at intricatesoftware.com>,
>> Alexander Strange <astrange at apple.com>, Mike Swingler
>> <swingler at apple.com>, Roger Hoover <rhoover at apple.com>
>
> Thanks, in advance, for any final feedback for this changeset.
>
> Dan
>
>
>> 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
>>
>>