RFR(L): 8143125: [aix] Further Developments for AIX
Volker Simonis
volker.simonis at gmail.com
Wed Dec 2 10:19:25 UTC 2015
Hi Thomas,
thanks a lot for this nice cleanup. Please find my remaining comments below:
libo4.hpp
=======
- please update the copyright year
- remove the following comments which reference SAP path or bugs
45 // See libo4.h (at //bas2/sapjvm/dev/common/libs/libo4/include)
46 // for details on this API.
59 // See libo4.h (at //bas2/sapjvm/dev/common/libs/libo4/include)
60 // for details on this API.
72 // See also CSN Internal Message 0001182318 2008
73 //
74 // See libo4.h (at //bas2/sapjvm/dev/common/libs/libo4/include)
75 // for details on this API.
libo4.cpp
=======
- please update the copyright year
vmStructs_aix_ppc.hpp
=================
- please update the copyright year
misc_aix.hpp
==========
- "trc" is defined to the empty string and doesn't seemed to be used anywhere:
45 #define trc(fmt, ...)
do we still need it?
os_aix.cpp
========
- why do we need the following define right in-between the includes:
76 #include "utilities/defaultStream.hpp"
77 #define PV_8_Compat 0x308000 /* Power PC 8 */
78 #include "utilities/events.hpp"
it doesn't seem to be used in os_aix.cpp
- you replaced most of the logging with trcVerbose but there are still
tow or three places using fprintf and jio_fprintf. Could you please
replace them by calls to trcVerbose as well.
- I can't see where the kernel thread id is initialized. Shouldn't you
call set_kernel_thread_id() from os::create_thread and
os::create_attached_thread() ?
- in os::commit_memory() are you sure you always have 4K pages:
2342 if (UseExplicitCommit) {
2343 // AIX commits memory on touch. So, touch all pages to be committed.
2344 for (char* p = addr; p < (addr + size); p += SIZE_4K) {
2345 *p = '\0';
2346 }
2347 }
wouldn’t it be better (i.e. potentially faster) to use
os::vm_page_size() instead of SIZE_4K ?
- it doesn't seem we use '_large_page_size'. We statically initialize
it to '0' and later on in os::init() to '-1'. Better remove it and
directly return '0' (or '-1') from os::large_page_size().
libperfstat_aix.{cpp, hpp}
==================
- please update the copyright year
- I want to second Goetz - we really don't need
perfstat_cpu_total_t_52. Please remove it from both
libperfstat_aix.{cpp, hpp}
loadlib_aix.cpp
==================
- please update the copyright year
misc_aix.{hpp,cpp}
==============
- I don't see why we need MiscUtils::describe_errno(). Can you please
instead use strerror() to get the error message (as we already do in a
lot of other places, e.g. in os_aix.cpp). That said, I realized that
strerror() is not thread-safe. So we should probably change it to the
POSIX function strerror_r(). But this is not only an AIX problem, so
better do that in a follow up change.
- the same holds true for MiscUtils::sleep_ms(). Why do we need yet
another AIX-specific helper function? Moreover, it is used only once.
Can you please instead use usleep() or sleep() directly, depending on
how long you want to wait. (Just as a side note: your current
implementation of sleep_ms() would sleep for three seconds if called
with 1000ms as argument, and it would call usleep() with an argument
of 1.000.000 which should be avoided according to your comment (which
I couldn't verify in the AIX man-page (see
https://www-01.ibm.com/support/knowledgecenter/ssw_aix_53/com.ibm.aix.basetechref/doc/basetrf2/sleep.htm))).
So please remove MiscUtils::sleep_ms().
There's no need to provide a new webrev, if you check that the your
latest changes will build and run.
Thank you and best regards,
Volker
On Thu, Nov 26, 2015 at 9:29 AM, Lindenmaier, Goetz
<goetz.lindenmaier at sap.com> wrote:
> Hi Thomas,
>
> thanks a lot for doing all these changes.
> Looks good now.
>
> Could you add the change comment to the patch before you
> Make a webrev next time? Simplifies sponsoring ;)
>
> Best regards,
> Goetz.
>
> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> Sent: Mittwoch, 25. November 2015 18:01
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: ppc-aix-port-dev at openjdk.java.net; HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR(L): 8143125: [aix] Further Developments for AIX
>
> Hi Goetz,
>
> new webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8143125-Further/webrev.01/webrev/
>
> thank you for reviewing! See remarks inline...
>
> On Tue, Nov 24, 2015 at 12:38 PM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>> wrote:
> Hi Thomas,
>
> I looked at your change. It’s good we get these improvements into
> openJDK. But I think we should skip some stuff that's not (yet?)
> used there. I'll sponsor the change. Details:
>
> libperfstat_aix.cpp:
>
> What do you need
> static fun_perfstat_partition_total_t g_fun_perfstat_partition_total = NULL;
> static fun_perfstat_wpar_total_t g_fun_perfstat_wpar_total = NULL;
> static fun_wpar_getcid_t g_fun_wpar_getcid = NULL;
> for? I think they are never used.
>
> They are now used (for the respective wrapper functions which in turn are used in os_aix.cpp. Sorry for omitting this code.
>
> libperfstat_aix.cpp:161
> We support AIX 5.3 with xlC12 only. I think we need not add the older
> #defines to openJDK. Also, the comment should be adapted and not mention xlc8 etc.
> Also, there are datastructures for 5.2 which can be removed.
>
> Removed the older defines.
> I would for now prefer to leave the AIX 5.2 data structures in this file. I am not sure if we could encounter older libperfstat versions on AIX 5.3 too. I plan to rework this coding to get rid of the duplicate structure definitions, but would prefer to do this in a separate patch.
>
>
> Where is this used?
> bool libperfstat::get_wparinfo(wparinfo_t* pwi)
> Do we need it if it's not used?
>
> It is now used :) in os_aix.cpp.
>
>
> libperfstat.hpp:835
> I would remove these prototypes commented out.
>
> I removed them.
>
> loadlib_aix.cpp
> Why do you do these changes? Please revert them.
>
> I reverted them.
>
> os_aix.hpp:219
> What's this good for?
> get_brk_at_startup()
> get_lowest_allocation_above_brk()
>
> I removed those prototypes.
>
> os_aix.hpp:259
> What's this good for?
> parse_qsyslib_path()
>
> I removed this prototype.
>
> os_aix.cpp:410
> Why do you move query_multipage_support()?
>
> I moved this back to its original position.
>
> os_aix.inline.hpp:164
> Why do you reverse the order of the functions here?
> The old order is the same as in the related files os_linux.inline.hpp etc.
>
> I reversed the order back to original order.
>
> Best regards,
> Goetz.
>
>
> Kind Regards, Thomas
>
>
>> -----Original Message-----
>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net<mailto:hotspot-dev-bounces at openjdk.java.net>] On
>> Behalf Of Thomas Stüfe
>> Sent: Freitag, 20. November 2015 11:49
>> To: ppc-aix-port-dev at openjdk.java.net<mailto:ppc-aix-port-dev at openjdk.java.net>; HotSpot Open Source Developers
>> Subject: RFR(L): 8143125: [aix] Further Developments for AIX
>>
>> Hi all,
>>
>> please review and sponsor these AIX only changes. Basically, with this
>> change we bring the OpenJDK AIX hotspot port up to speed with the
>> developments done at SAP in the recent months.
>>
>> For a more detailled number of changes and fixes, please refer to the bug
>> description.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8143125
>> webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8143125-
>> Further/webrev.00/webrev/index.html
>>
>> Kind Regards, Thomas
>
More information about the ppc-aix-port-dev
mailing list