Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parallel_rng_types: refactoring and "objectification" #482

Merged
merged 4 commits into from
Jan 8, 2020

Conversation

dev-zero
Copy link
Contributor

This rng_stream_type is one of the most low-level/basic objects I found. To see how a complete OO approach could look like, I converted it to Modern Fortran.

Points to discuss:

  • object initialization: while it looks like a constructor, there are differences to what people usually call a constructor. With that in mind, it might be clearer to create a type-bound procedure init(...) instead.
  • finalization: this object does not have any pointer components which would require special care, which is why there is no final routine
  • since I didn't want to adapt the whole code yet, I've added a separate ..._TEST program to test this part without most of the other infrastructure

To build and test:

make ARCH=... VERSION=... parallel_rng_types_TEST
./exe/$ARCH/parallel_rng_types_TEST.popt

There is not much here yet, except for the syntactic sugar. More to come when adapting the code to the changes and when going up the "foodchain" (tackling the globenv).

@dev-zero dev-zero force-pushed the feature/objectification branch 2 times, most recently from f27f2c4 to c63a99c Compare August 8, 2019 13:00
@dev-zero
Copy link
Contributor Author

dev-zero commented Aug 8, 2019

some observations which may be important to reduce the usage of pointers in CP2K:

  • Fortran passes values to procedures (subroutines and functions) by-reference, meaning that unless one actually has to retain (shared ownership), allocate or deallocate a passed object inside a procedure one should not take an argument as pointer (or allocatable for that matter)
  • While you can pass a pointer of a type to a function which does not carry the pointer attribute specification for that attribute type, vice-versa is not possible, leading to the situation where you have to use pointers everywhere (same goes for an allocatable)
  • The only thing one loses when not taking it as a pointer is that you can't check whether the pointer is actually associated (meaning that this shifts the responsibility to call the function with valid data to the caller again, as it should be)
  • There are multiple reasons to use pointers inside user-defined types and to decide whether or not a pointer can be replaced by either the bare object or an allocatable depends on how it is used:
    • if it is just a pointer because the functions called on it require a pointer then one must first check whether the signatures of the called functions can be changed (see point above)
    • if it is a pointer because of optional/deferred allocation it could be changed to an allocatable, given the functions called on it support it (in case it is non-optional, simply replace it by a bare object)
    • if it is used as a pointer because of shared ownership (or as a weak pointer) it must remain a pointer
  • The reasons for using pointers (not dummy arguments, for those, see above) inside functions also vary:
    • if they are used to allocate objects and then move those objects into structures, one could replace them by allocatables and use move_alloc instead, unfortunately this doesn't work for arrays (Fortran doesn't support arrays of allocatables or pointers)
    • when used as shortcuts one can instead use the ASSOCIATE construct (which becomes much more important as soon as members of user-defined types lose the pointer specifier).

Example for the problem of passing non-pointer objects to functions expecting pointers or allocatables:

module mymod
    type old_type
        integer :: val
    end type old_type

    type new_type
        integer :: val
    contains
        procedure, pass(self) :: print_me => print_new_type
    end type new_type

contains
    subroutine print_new_type(self)
        class(new_type) :: self
        print *, self%val
    end subroutine

    subroutine print_old_type(type_instance)
        type(old_type) :: type_instance
        print *, type_instance%val
    end subroutine

    subroutine print_old_type_by_ptr(type_instance)
        type(old_type), pointer :: type_instance
        print *, type_instance%val
    end subroutine

    subroutine print_old_type_by_alloc(type_instance)
        type(old_type), allocatable :: type_instance
        print *, type_instance%val
    end subroutine
end module mymod

program main
    use mymod

    type(old_type) :: ot
    type(old_type), pointer :: ot_ptr
    type(old_type), allocatable :: ot_alloc

    ot%val = 10
    allocate (ot_ptr, ot_alloc)
    ot_ptr%val = 11
    ot_alloc%val = 12

    call print_old_type(ot)
    call print_old_type(ot_ptr) ! can pass a pointer to a procedure not expecting a pointer
    call print_old_type(ot_alloc) ! can pass an allocatable to a procedure not expecting a allocatable

    call print_old_type_by_ptr(ot_ptr)
    !call print_old_type_by_ptr(ot) ! DOES NOT WORK, MUST BE A POINTER
    !call print_old_type_by_ptr(ot_alloc) ! DOES NOT WORK, MUST BE A POINTER

    call print_old_type_by_alloc(ot_alloc)
    !call print_old_type_by_alloc(ot) ! DOES NOT WORK, MUST BE AN ALLOCATABLE
    !call print_old_type_by_alloc(ot_ptr) ! DOES NOT WORK, MUST BE AN ALLOCATABLE

    deallocate (ot_ptr)
end program main

Some observations wrt our prettifier:

  • it should be possible to disable it for blocks of data, aligning them by decimal point increases readability
  • it should not touch spaces in math formulas, spacing increases readability
  • it can't distinguish between the statement WRITE and a function write, which might become more common as a type-bound procedure

... wrt conventions checker:

  • -Werror=realloc-lhs-all may be useful to detect undesired reallocations, but for things like strings it becomes really annoying having to workaround and not being able to use correct code as intended

@dev-zero
Copy link
Contributor Author

dev-zero commented Aug 8, 2019

@juerghutter this PR and my remarks above are essentially a follow-up on our discussion about our usage of pointers and possibly moving to allocatables (or plain objects). The conversion of rng_stream_type to a type with type-bound procedures is secondary, although it simplifies the USE statements and makes for a nicer interface.

@oschuett
Copy link
Member

oschuett commented Aug 8, 2019

Yes, the pointer attribute should be removed from pretty much all dummy arguments. The good news is that we can work our way through the "foodchain" one type at a time. Obviously, we have to remove a type's reference counting first, but that shouldn't be too difficulty. Our objects hardly ever change ownership, ie. all those pointers are just weak references.

I also agree that the types that actually own an object should have it as plain object or allocatable. In the past we always ran into compiler issues (e.g. 1284d6c and f1f51e8), but we just have to keep trying.

@dev-zero
Copy link
Contributor Author

dev-zero commented Aug 9, 2019

Yes, the pointer attribute should be removed from pretty much all dummy arguments. The good news is that we can work our way through the "foodchain" one type at a time. Obviously, we have to remove a type's reference counting first, but that shouldn't be too difficulty. Our objects hardly ever change ownership, ie. all those pointers are just weak references.

From what I've seen one must be careful about the the reference counting removal. And I think it is not even necessary to start there: first remove the pointer attribute on the non-allocation-related methods of a type, then you can remove the pointer on many of the functions which get a pointer to that type just because they're using its methods.

The only thing I haven't thought about yet is how to handle the case where you're doing a get on a type to get a reference to a type it has:

type :: foo
end type

type :: bar
    type(foo), pointer :: my_bar
end type

type(bar), pointer, intent(in) :: b
type(foo), pointer :: f
bar_get(b, foo=f)
foo_do_something(f, 1, 2, 3)

This pattern will be difficult with non-pointers.

I also agree that the types that actually own an object should have it as plain object or allocatable. In the past we always ran into compiler issues (e.g. 1284d6c and f1f51e8), but we just have to keep trying.

Thanks for the references. 2016 isn't too long ago :-(

@dev-zero dev-zero changed the title RFC: OO adaption of existing types parallel_rng_types: refactoring and "objectification" Aug 26, 2019
@dev-zero dev-zero force-pushed the feature/objectification branch 4 times, most recently from 64141ec to bf0079b Compare September 4, 2019 13:43
@dev-zero
Copy link
Contributor Author

dev-zero commented Sep 4, 2019

this PR now only depends on #541 and fortran-lang/fprettify#55, respectively the update of fprettify in #565

@dev-zero dev-zero force-pushed the feature/objectification branch 4 times, most recently from 00c9594 to 40d408d Compare October 1, 2019 08:47
@dev-zero dev-zero force-pushed the feature/objectification branch 2 times, most recently from d823162 to b30f570 Compare November 20, 2019 17:22
the following is valid Fortran:

    mystr = "abc&
        &def"

and is equivalent to:

    mystr = "abcdef"

but the current version of the `doxify.sh` script strips it since
`fixcomments.pl` assumed that all ampersands are line continuations.
@dev-zero dev-zero force-pushed the feature/objectification branch 2 times, most recently from 0e5df13 to cde5872 Compare January 7, 2020 20:38
Fortran does pass-by-reference, but `POINTER` was needed for the
procedures acting on the `rng_stream_type`. With those refactored as
type-bound procedures, almost all uses of pointers can be avoided.

One of the two remaining cases (in `helium_methods.F`) could likely be
avoided as well by using an allocatable and `move_alloc`, but this would
need another nested type like `rng_stream_p_type` but with an
allocatable member instead of a pointer (or a complete redesign of the
RNG setup in that part).
@dev-zero dev-zero merged commit 19acd32 into cp2k:master Jan 8, 2020
@dev-zero dev-zero deleted the feature/objectification branch January 8, 2020 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants