RFR: NoBugIDYet: Refactoring os classes

Volker Simonis volker.simonis at gmail.com
Fri Mar 15 08:52:18 PDT 2013 

Hi Rickard,

the one thing I forgot to mention in my previous mail is that in
general I really aprpeciate the refactoring of the os-classes.
Although this will cause us a fair amount of work (see Thomas mail) I
really think it is worth doing it. Especially the inclusion of the
whole os_<xxx>_<yyy>.hpp files right into the middle of the os class
definition is a real PITA (and it breaks modern IDEs as well!).

Regards,
Volker


On Thu, Mar 14, 2013 at 12:05 PM, Rickard Bäckman
<rickard.backman at oracle.com> wrote:
> Volker,
>
> no I haven't tried a lot of IDEs. It seems as if intermediate files are preferred.
>
> Thanks
> /R
>
> On Mar 14, 2013, at 11:35 AM, Volker Simonis wrote:
>
>> Have you tested how IDEs (Eclipse, NetBeans) behave if we have
>> multiple files with the same name in different locations. At least
>> navigation between such files may become unintuitive. In Emacs for
>> example it's hard to keep them apart because they get these additional
>> numbers in their name ('<n>') which is only based on the order in
>> which they were loaded.
>>
>> So I'd personally also prefer to stay with the old name scheme and use
>> intermediate .hpp files. But that's just my personal opinion:)
>>
>> Volker
>>
>> On Wed, Mar 13, 2013 at 3:29 PM, Coleen Phillmore
>> <coleen.phillimore at oracle.com> wrote:
>>>
>>> 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