RFR: NoBugIDYet: Refactoring os classes

[Rickard Bäckman]

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.

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
>>