Quantcast

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems

David Holmes
Thanks Erik!

David

On 19/05/2017 6:07 PM, Erik Joelsson wrote:

> Build changes look good to me.
>
> /Erik
>
>
> On 2017-05-19 09:15, David Holmes wrote:
>> Hi Magnus,
>>
>> On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:
>>>
>>>
>>> On 2017-05-18 09:35, David Holmes wrote:
>>>> On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:
>>>>> On 2017-05-18 08:25, David Holmes wrote:
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8174231
>>>>>>
>>>>>> webrevs:
>>>>>>
>>>>>> Build-related:
>>>>>> http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
>>>>>
>>>>> Build changes look  good.
>>>>
>>>> Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging
>>>> prints outs - do you want me to remove them? I suppose they may be
>>>> useful if something goes wrong on some platform.
>>>
>>> I didn't even notice them. :-/
>>>
>>> It's a bit unfortunate we don't have a debug level on the logging
>>> from configure. :-( Otherwise they would have clearly belonged there.
>>>
>>> The AC_MSG_NOTICE messages stands out much from the rest of the
>>> configure log, so maybe it's better that you remove them. The logic
>>> itself is very simple, if the -D flags are missing then we can surely
>>> tell what happened. So yes, please remove them.
>>
>> Webrev updated in place.
>>
>> I have removed them to avoid noise - particularly as they get executed
>> twice.
>>
>> I also made an adjustment to AC_SEARCH_LIBS as I don't want to pass
>> the saved_LIBS value in explicitly, but want it to use the LIBS value
>> - which is no longer cleared before the call. I've verified all
>> platforms are okay - except AIX which I'll need Thomas to recheck when
>> he can.
>>
>> I also discovered an oddity in that our ARM64 builds seem to use
>> different system libraries in that librt.so is not needed for
>> clock_gettime. This still seems to work ok. Of more of a concern if we
>> were expand this kind of function-existence check is that libc seems
>> to contain "dummy" (or at least dysfunctional) versions of a number of
>> the core pthread APIs!
>>
>> Thanks,
>> David
>>
>>> Alternatively, rewrite them as CHECKING/RESULT, if you want to keep
>>> the logging. That way they match better the rest of the configure log
>>> (and also describes what you're doing). Just check if AC_SEARCH_LIBS
>>> prints some output (likely so, I think), then you can't do it in the
>>> middle of a CHECKING/RESULT pair, but have to do the CHECKING part
>>> after AC_SEARCH_LIBS.
>>>
>>> /Magnus
>>>
>>>>
>>>> David
>>>>
>>>>> /Magnus
>>>>>
>>>>>>       hotspot:
>>>>>> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/
>>>>>>
>>>>>> First a big thank you to Thomas Stuefe for testing various
>>>>>> versions of
>>>>>> this on AIX.
>>>>>>
>>>>>> This is primarily a refactoring and cleanup exercise (ie lots of
>>>>>> deleted duplicated code!).
>>>>>>
>>>>>> I have taken the PlatformEvent, PlatformParker and Parker::* code,
>>>>>> out
>>>>>> of os_linux and moved it into os_posix for use by Linux, OSX, BSD,
>>>>>> AIX
>>>>>> and perhaps one day Solaris (more on that later).
>>>>>>
>>>>>> The Linux code was the most functionally complete, dealing with
>>>>>> correct use of CLOCK_MONOTONIC for relative timed waits, and the
>>>>>> default wall-clock for absolute timed waits. That functionality is
>>>>>> not, unfortunately, supported by all our POSIX platforms so there are
>>>>>> some configure time build checks to set some #defines, and then some
>>>>>> dynamic lookup at runtime**. We allow for the runtime environment to
>>>>>> be less capable than the build environment, but not the other way
>>>>>> around (without build time support we don't know the runtime types
>>>>>> needed to make library calls).
>>>>>>
>>>>>> ** There is some duplication of dynamic lookup code on Linux but this
>>>>>> can be cleaned up in future work if we refactor the time/clock code
>>>>>> into os_posix as well.
>>>>>>
>>>>>> The cleanup covers a number of things:
>>>>>> - removal of linux anachronisms that got "ported" into the other
>>>>>> platforms
>>>>>>   - eg EINTR can not be returned from the wait methods
>>>>>> - removal of solaris anachronisms that got ported into the linux code
>>>>>> and then on to other platforms
>>>>>>   - eg ETIMEDOUT is what we expect never ETIME
>>>>>> - removal of the ancient/obsolete os::*::allowdebug_blocked_signals()
>>>>>> from the Parker methods
>>>>>> - consolidation of unpackTime and compute_abstime into one utility
>>>>>> function
>>>>>> - use statics for things completely private to the implementation
>>>>>> rather than making them part of the os* API (eg access to condAttr
>>>>>> objects)
>>>>>> - cleanup up commentary and style within methods of the same class
>>>>>> - clean up coding style in places eg not using Names that start with
>>>>>> capitals.
>>>>>>
>>>>>> I have not tried to cleanup every single oddity, nor tried to
>>>>>> reconcile differences between the very similar in places
>>>>>> PlatformEvent
>>>>>> and Park methods. For example PlatformEvent still examines the
>>>>>> FilterSpuriousWakeups** flag, and Parker still ignores it.
>>>>>>
>>>>>> ** Perhaps a candidate for deprecation and future removal.
>>>>>>
>>>>>> There is one mini "enhancement" slipped in this. I now explicitly
>>>>>> initialize mutexes with a mutexAttr object with its type set to
>>>>>> PTHREAD_MUTEX_NORMAL, instead of relying on the definition of
>>>>>> PTHREAD_MUTEX_DEFAULT. On FreesBSD the default is not "normal" but
>>>>>> "error checking" and so is slow. On all other current platforms there
>>>>>> is no effective change.
>>>>>>
>>>>>> Finally, Solaris is excluded from all this (other than the debug
>>>>>> signal blocking cleanup) because it potentially supports three
>>>>>> different low-level sync subsystems: UI thr*, Pthread, and direct LWP
>>>>>> sync. Solaris cleanup would be a separate RFE.
>>>>>>
>>>>>> No doubt I've overlooked mentioning something that someone will
>>>>>> spot. :)
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems

David Holmes
<dropping build-dev>

Hi Robbin,

On 19/05/2017 10:33 PM, Robbin Ehn wrote:

> Hi David
>
> On 05/19/2017 01:36 PM, David Holmes wrote:
>>>
>>> There are three different forms of the calculation. The two relative
>>> time versions use a different time function and so a different time
>>> structure (timeval vs timespec) and a different calculation.
>
> Yes that's why I included unit in my example signature.
>
>>
>> It's more complicated than that because we may have build time
>> SUPPORTS_CLOCK_MONOTONIC but we still need the runtime check as well.
>>
>> to_abstime is the old linux unpackTime with the addition of the build
>> time conditionals.
>
> I'm not changing that, I'm not sure how to explain better,
> so here is the patch (only compiled and silly naming convention):
> http://cr.openjdk.java.net/~rehn/8174231/webrev/

Thanks for the taking the time to do this.

> This makes the calculation independent of source/unit.

Okay I see. Took me a few read throughs to get the gist of it - and it
helps to read from the bottom functions up :)

Not sure why you are returning a value from the functions though ??

Let's see what others think. It's somewhat harder to compare against the
existing code.

Thanks again.
David

> Thanks!
>
> /Robbin
>
>
>
>>
>> David
>>
>>> David
>>> -----
>>>
>>>> I do not see a problem with this, only better readability?
>>>>
>>>> /Robbin
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>       struct timespec now;
>>>>>>       int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>>>       assert_status(status == 0, status, "clock_gettime");
>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec,
>>>>>> now.tv_nsec, NANOUNITS);
>>>>>>    } else {
>>>>>> #else
>>>>>>    {
>>>>>> #endif
>>>>>>       struct timeval now;
>>>>>>       int status = gettimeofday(&now, NULL);
>>>>>>       assert(status == 0, "gettimeofday");
>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec,
>>>>>> now.tv_usec, MICROUNITS);
>>>>>>    }
>>>>>> #endif
>>>>>>
>>>>>> Thanks for fixing this!
>>>>>>
>>>>>> /Robbin
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems

Robbin Ehn
Hi David,

On 05/20/2017 03:07 PM, David Holmes wrote:
> Okay I see. Took me a few read throughs to get the gist of it - and it helps to read from the bottom functions up :)

Great! Yes, C-style with static functions tends to end up that way, since you don't want a lot of forward declarations.

> Not sure why you are returning a value from the functions though ??

I skipped (re-)moving an assert on the max_secs value,
1660   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
just to make the code the same.
So there are some minors/nits in the patch.

>
> Let's see what others think. It's somewhat harder to compare against the existing code.

Yes agreed.

/Robbin

>
> Thanks again.
> David
>
>> Thanks!
>>
>> /Robbin
>>
>>
>>
>>>
>>> David
>>>
>>>> David
>>>> -----
>>>>
>>>>> I do not see a problem with this, only better readability?
>>>>>
>>>>> /Robbin
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>>       struct timespec now;
>>>>>>>       int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>>>>       assert_status(status == 0, status, "clock_gettime");
>>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_nsec, NANOUNITS);
>>>>>>>    } else {
>>>>>>> #else
>>>>>>>    {
>>>>>>> #endif
>>>>>>>       struct timeval now;
>>>>>>>       int status = gettimeofday(&now, NULL);
>>>>>>>       assert(status == 0, "gettimeofday");
>>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, MICROUNITS);
>>>>>>>    }
>>>>>>> #endif
>>>>>>>
>>>>>>> Thanks for fixing this!
>>>>>>>
>>>>>>> /Robbin
Loading...