RFR: NoBugIDYet: Refactoring os classes
Rickard Bäckman
rickard.backman at oracle.com
Thu Mar 14 04:05:53 PDT 2013
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