RFR: NoBugIDYet: Refactoring os classes

Coleen Phillmore coleen.phillimore at oracle.com
Wed Mar 13 07:29:29 PDT 2013 

On 3/13/2013 10:04 AM, Rickard Bäckman wrote:
> Coleen,
>
>
> On Mar 13, 2013, at 2:48 PM, Coleen Phillmore wrote:
>
>> Oh, you did pick xnix!  I have to confess to not reading the webrevs yet.  I only glanced at webrev 4.   I don't think we can name different files the same thing for some historical reason that I can't remember.   Does this design rely on renaming os_linux.cpp os_family.cpp?  I strongly prefer the original which is more descriptive.
> Yes, I picked xnix. We can name different files the same thing as long as only one is part of the compilation (which it will be since the files with the same names are in different os/ directories). No the design doesn't rely on renaming os_linux.cpp to os_family.cpp. One thing I would like is to at least rename the include files (if we really don't want to do that at least create an intermediate file) to avoid all the
>
> #if IS_SOLARIS
> #include os_solaris.hpp
> #elseif IS_LINUX
> #include os_linux.hpp
> #…
> #endif

Yes, I was thinking it would be good to have the intermediate .hpp file 
to avoid the conditional includes for this case instead.

Coleen
>
> Thanks
> /R
>
>> This is good though, it moves along the logjam that was preventing us from sharing more code.   I'll look at this more later.
> To be able to reduce the amount of duplicated code is one of the main things I'm trying to address with this change.
>
> Thanks
> /R
>
>> Coleen
>>
>> On 3/13/2013 9:38 AM, Rickard Bäckman wrote:
>>> That is done in the step4 part of my webrev. :)
>>>
>>> /R
>>>
>>> On Mar 13, 2013, at 2:32 PM, Coleen Phillmore wrote:
>>>
>>>> Your reply below made think maybe we should rename posix directories and files xnix.
>>>> Coleen
>>>>
>>>> On 3/13/2013 9:11 AM, Rickard Bäckman wrote:
>>>>> David,
>>>>>
>>>>> On Mar 13, 2013, at 1:57 PM, 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.
>>>>> Ah, no. At the moment we have a bunch of things that rely on knowing details about os::Bsd, os::Linux (for example jvm_bsd, jvm_linux). We also have a lot of code that doesn't care about the details. They use the os:: interface. This does however require that all platforms have an implementation (empty or not) of whatever is in os:: (unless it is only called from some piece of code that is platform specific. In that case I do think the code is misplaced though).
>>>>>
>>>>> That is still the way things should work. However things that previously relied on os::Bsd or os::Linux should instead rely on os::pd. That would make that code easier to port to new platforms as well as easier to at some point unify into code that works for multiple platforms.
>>>>>
>>>>> os:: is still the main interface to os things. In my example above we would have had an
>>>>> os::suspend_thread() - that would have had an implementation on all platforms. The *nix platforms could use the above implementation and Windows another.
>>>>> os::suspend_thread() would look like:
>>>>> {
>>>>>    os::pd::suspend_thread();
>>>>> }
>>>>>
>>>>> instead of having 4 different implementations of suspend_thread, we would now have 2.
>>>>>
>>>>> Hope that makes things clearer.
>>>>>
>>>>> Thanks
>>>>> /R
>>>>>
>>>>>> 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