RFR: NoBugIDYet: Refactoring os classes
Coleen Phillmore
coleen.phillimore at oracle.com
Wed Mar 13 06:22:12 PDT 2013
On 3/13/2013 8:57 AM, David Holmes wrote:
> On 13/03/2013 9:41 PM, Rickard Bäckman wrote:
>> David,
>>
>> On Mar 13, 2013, at 11:33 AM, David Holmes wrote:
>>
>>> Hi Rickard,
>>>
>>> An initial comment on your description here:
>>>
>>>> There are three classes: osSolaris, osSolarisSparc and os.
>>>> These are tried together through a couple of typedefs. osSolaris is
>>>> typedefed to osCurrent and osSolarisSparc is typedefed to osCpu.
>>>> Inside the os class we now typedef osCurrent to pd and osCpu to cpu.
>>>> This way we can now access things that was previously accessed
>>>> through os::Solaris::some_method like: os::pd::some_method. That
>>>> helps if we want to write some code that would be common for a
>>>> couple of platforms, using an api through pd like:
>>>>
>>>> doSomething() {
>>>> os::pd::setup();
>>>> do_something();
>>>> os::pd::close();
>>>> }
>>>
>>> I don't think the "pd" makes sense because you have to know what the
>>> "pd" actually is. In old terms an os::Solaris method can call
>>> another os::Solaris method because it knows it is there. If "pd" is
>>> common somehow (say pthreads functionality) then that commonality
>>> has to be named eg os::pthreads, because there might be another
>>> commonality related to some other aspect of functionality (perhaps
>>> Dtrace), so they can't both be called "pd".
>>
>> So maybe the example here was a bit bad. Maybe this makes more sense:
>>
>> osXnix::suspend_thread(thread) {
>> ...
>> os::pd::signal_notify(thread);
>> …
>> }
>>
>> You still have to know that pd is an interface for one os
>> platform-dependent API. signal_notify() in this example would be
>> implemented by pthread_kill on Linux and Bsd, thr_kill on Solaris.
>> Windows wouldn't need one.
>
> But that seems more awkward than what we have now. Now we have
> os::signal_notify() which is implemented differently for
> solaris/linux/bsd. That specialization is done via the compile-time
> include mechanism. With what you suggest the methods that call things
> like signal_notify now have to know that they are implemented
> differently on different platforms whereas currently they do not need
> to know that.
So this is a good argument for simply promoting most/many/all of these
pd functions to os.hpp, have the implementation be in os_<os>.cpp and
lose the fact that they are osBsd/osLinux/osWindows class concept. Is
this possible? Would it make the os:: class/namespace too large?
Could we still get rid of the #includes in the middle of the class
declaration?
Thanks,
Coleen
>
> David
>
>> Another example taken from os::Bsd
>>
>> void os::Bsd::hotspot_sigmask(Thread* thread) {
>>
>> //Save caller's signal mask before setting VM signal mask
>> sigset_t caller_sigmask;
>> pthread_sigmask(SIG_BLOCK, NULL, &caller_sigmask);
>>
>> OSThread* osthread = thread->osthread();
>> osthread->set_caller_sigmask(caller_sigmask);
>>
>> pthread_sigmask(SIG_UNBLOCK, os::Bsd::unblocked_signals(), NULL);
>>
>> if (!ReduceSignalUsage) {
>> if (thread->is_VM_thread()) {
>> // Only the VM thread handles BREAK_SIGNAL ...
>> pthread_sigmask(SIG_UNBLOCK, vm_signals(), NULL);
>> } else {
>> // ... all other threads block BREAK_SIGNAL
>> pthread_sigmask(SIG_BLOCK, vm_signals(), NULL);
>> }
>> }
>> }
>>
>> could be replaced by an:
>>
>> void osXnix::hotspot_sigmask(Thread *thread) {
>> sigset_t caller_sigmask;
>> os::pd::thread_sigmask(SIG_BLOCK, NULL, &caller_sigmask);
>> …
>> os::pd::thread_sigmask(…);
>> …
>> }
>>
>> thread_sigmask would be pthread_sigmask for Bsd, Linux and
>> thr_setsigmask on Solaris.
>>
>> I agree that having yet another level of grouping like
>> os::signals::sigmask() or os::threads::create_thread() might be a
>> good idea.
>> I just think that this change is large enough as it is. Further
>> improvements are always possible.
>>
>> Thanks
>> /R
>>
>>
>>>
>>> Cheers,
>>> David
>>>
>>> On 13/03/2013 7:50 PM, Rickard Bäckman wrote:
>>>> Hi all,
>>>>
>>>> I've spent some time on trying to do some changes to (in my
>>>> opinion) clean up the os_ files.
>>>>
>>>> The change is broken up into 4 web revs to make the changes easier
>>>> to spot since it involves new files, renaming of files, etc.
>>>>
>>>> Step 1)
>>>> * removes the os::Bsd, os::Linux, etc classes. They are
>>>> replaced by classes osBsd, osLinux, osPosix (renamed in a later
>>>> step), etc.
>>>> * The #include os_name.hpp inside the class definition is moved
>>>> to before the definition of class os.
>>>> * Renaming users to the new osBsd, osSolaris, osWindows,
>>>> osLinux, etc.
>>>> * Adds a typedef osCurrent pd; inside the os class.
>>>>
>>>> Step 2)
>>>> * Some methods existed in all or almost all
>>>> os_<os>_<platform>.hpp, they were moved to os.hpp.
>>>>
>>>> Step 3)
>>>> * Removes os_<os>_<platform>.hpp and replaces it with a
>>>> os_cpu.hpp. A new os_cpu.cpp contains the implementations of those
>>>> methods.
>>>> * Changes #includes of os_<os>_<platform> into #include "os_cpu"
>>>> * Adds a typedef osCpu cpu; inside the os class.
>>>>
>>>> Step 4)
>>>> * Renames os_posix to os_xnix (xnix is *nix).
>>>> * Renames os_<os>.ext to os_family.ext
>>>> * Includes are changed from:
>>>>
>>>> #ifdef TARGET_OS_FAMILY_linux
>>>> # include "os_linux.hpp"
>>>> #endif
>>>> #ifdef TARGET_OS_FAMILY_solaris
>>>> # include "os_solaris.hpp"
>>>> #endif
>>>> #ifdef TARGET_OS_FAMILY_windows
>>>> # include "os_windows.hpp"
>>>> #endif
>>>> #ifdef TARGET_OS_FAMILY_bsd
>>>> # include "os_bsd.hpp"
>>>> #endif
>>>>
>>>> to:
>>>>
>>>> #include "os_family.hpp"
>>>>
>>>>
>>>> I'll try to summarize the final result the way it would look for
>>>> solaris_sparc:
>>>>
>>>> There are three classes: osSolaris, osSolarisSparc and os.
>>>> These are tried together through a couple of typedefs. osSolaris is
>>>> typedefed to osCurrent and osSolarisSparc is typedefed to osCpu.
>>>> Inside the os class we now typedef osCurrent to pd and osCpu to cpu.
>>>> This way we can now access things that was previously accessed
>>>> through os::Solaris::some_method like: os::pd::some_method. That
>>>> helps if we want to write some code that would be common for a
>>>> couple of platforms, using an api through pd like:
>>>>
>>>> doSomething() {
>>>> os::pd::setup();
>>>> do_something();
>>>> os::pd::close();
>>>> }
>>>>
>>>> This should enable us to move code from platform specific code
>>>> (thread creation, signal handling, etc) that look very similar
>>>> (Linux, Bsd, Solaris) into something else that utilizes APIs
>>>> through os::pd or os::cpu.
>>>>
>>>> it enables us to have platform specific includes in what
>>>> os_family.hpp (used to be os_linux.hpp) for platform specific
>>>> things like semaphores.
>>>>
>>>> To show the layout of the files:
>>>>
>>>> file: os_family.hpp
>>>> class osSolaris {
>>>> };
>>>>
>>>> typedef osSolaris osCurrent;
>>>>
>>>> file: os_cpu.hpp
>>>> class osSolarisSparc {
>>>> };
>>>>
>>>> typedef osSolarisSparc osCpu;
>>>>
>>>> file: os.hpp
>>>> class os {
>>>> typedef osCpu cpu;
>>>> typedef osCurrent pd;
>>>> };
>>>>
>>>> Webrevs:
>>>> http://cr.openjdk.java.net/~rbackman/refactor/step1/webrev/
>>>> http://cr.openjdk.java.net/~rbackman/refactor/step2/webrev/
>>>> http://cr.openjdk.java.net/~rbackman/refactor/step3/webrev/
>>>> http://cr.openjdk.java.net/~rbackman/refactor/step4/webrev/
>>>>
>>>> Thanks
>>>> /R
>>>>
>>
More information about the hotspot-dev
mailing list