RFR: NoBugIDYet: Refactoring os classes (original) (raw)

Rickard Bäckman rickard.backman at oracle.com
Wed Mar 13 04:41:09 PDT 2013

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::somemethod like: os::pd::somemethod. 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(); dosomething(); 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.

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 osname.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.hpp, they were moved to os.hpp. Step 3) * Removes os.hpp and replaces it with a oscpu.hpp. A new oscpu.cpp contains the implementations of those methods. * Changes #includes of os into #include "oscpu" * Adds a typedef osCpu cpu; inside the os class. Step 4) * Renames osposix to osxnix (xnix is *nix). * Renames os.ext to osfamily.ext * Includes are changed from: #ifdef TARGETOSFAMILYlinux # include "oslinux.hpp" #endif #ifdef TARGETOSFAMILYsolaris # include "ossolaris.hpp" #endif #ifdef TARGETOSFAMILYwindows # include "oswindows.hpp" #endif #ifdef TARGETOSFAMILYbsd # include "osbsd.hpp" #endif to: #include "osfamily.hpp"

I'll try to summarize the final result the way it would look for solarissparc: 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::somemethod like: os::pd::somemethod. 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(); dosomething(); 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 osfamily.hpp (used to be oslinux.hpp) for platform specific things like semaphores. To show the layout of the files: file: osfamily.hpp class osSolaris { }; typedef osSolaris osCurrent; file: oscpu.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