Code review comment for lp:~albertog/siesta/4.0-json-time

Dear Nick,

Thank you for reviewing this.
My comments:

— I did not want to introduce another dependency for m_io. This write operation is very localized in time, so there is no need to reserve
a unit in the way of m_io.
- My json schema is not perfect: the “info nodes: time, calls, and %, are logically also children of the section objects, but conceptually different (they are “leaf info” instead of tree branches. So with a view to a future scripting capability of the json file, I put the “_” in front to clearly mark them as different. This is obviously not fool-proof.
- I agree that we do not need two different optional arguments.

Let me know if you agree, and I will just merge the interface simplification from your changes.

  Best regards,

  Alberto

> On 19 Feb 2018, at 09:21, Nick Papior <email address hidden> wrote:
>
> I have made a merge request that tries to solve some of the things I have remarked below.
>
> Note they are comments/remarks. I however think the logic is easier by not having two optional arguments.
>
> Diff comments:
>
>>
>> === modified file 'Src/timer_tree.f90'
>> --- Src/timer_tree.f90 2016-01-25 16:00:16 +0000
>> +++ Src/timer_tree.f90 2018-02-15 15:48:27 +0000
>> @@ -266,31 +269,71 @@
>> enddo
>> p%data%totTime = globaltime + 1.0e-6_dp
>>
>> + json_output = .true.
>> + call get_unit_number(js_lun)
>> + open(unit=js_lun,file="time.json",form="formatted", &
>> + action="write",position="rewind")
>> +
>> write(*,"(/,a20,T30,a6,a12,a8)") "Section","Calls","Walltime","%"
>> - call walk_tree(p,0)
>>
>> + write(js_lun,*) '{'
>> + call walk_tree(p,0,json=json_output,js_lun=js_lun)
>> + write(js_lun,*) '}'
>> + if (json_output) close(js_lun)
>> +
>> end subroutine timer_report_global
>>
>> !------------------------------------------------
>> - recursive subroutine walk_tree(p,level,maxlevel)
>> + recursive subroutine walk_tree(p,level,maxlevel,json,js_lun)
>> type(section_t), intent(in),target :: p
>> integer, intent(in) :: level
>> integer, intent(in), optional:: maxlevel
>> + logical, intent(in), optional:: json
>> + integer, intent(in), optional:: js_lun
>>
>> integer :: i
>> - character(len=40) fmtstr
>> + character(len=40) fmtstr, fmt_json, fmt_json_head
>> + logical :: json_output
>>
>> + json_output = .false.
>> + if (present(json)) then
>> + json_output = json
>> + if (.not. present(js_lun)) then
>> + call die("Need a js_lun for json timer output")
>> + endif
>> + endif
>> if (present(maxlevel)) then
>> if (level > maxlevel) RETURN
>> endif
>> pd => p%data
>> write(fmtstr,"(a,i0,a1,a)") "(", level+1, "x", ",a20,T30,i6,f12.3,f8.2)"
>> +
>> write(*,fmtstr) pd%name, pd%nCalls, &
>> pd%totTime, 100*pd%totTime/globaltime
>> + if (json_output) then
>> + write(fmt_json,"(a,i0,a1,a)") "(",2*level+1,"x",",a,i0,a,f12.3,a,f8.2)"
>> + write(fmt_json_head,"(a,i0,a)") "(", 2*level+1, "x,a)"
>> + write(js_lun,fmt=fmt_json,advance="no") &
>> + '"' // trim(pd%name) // '": { "_calls": ', pd%nCalls, &
>> + ', "_time": ', pd%totTime, &
>> + ', "_%": ', 100*pd%totTime/globaltime
>> + endif
>
> Why are they prefixed with "_", wouldn't "calls", "time" and "%" make more sense?
>
>> if (p%nchildren /= 0) then
>> + if (json_output) write(js_lun,"(a)") ","
>> do i=1,p%nchildren
>> - call walk_tree(p%child(i),level+1,maxlevel)
>> + call walk_tree(p%child(i),level+1,maxlevel,json,js_lun)
>> + if (json_output) then
>> + if (i < p%nchildren) then
>> + write(js_lun,fmt="(a)") ','
>> + else
>> + write(js_lun,*) ! just new line
>> + endif
>> + endif
>> enddo
>> + ! End the parent section with an indented '}'
>> + if (json_output) write(js_lun,fmt=fmt_json_head,advance="no") "}"
>> + else
>> + if (json_output) write(js_lun,fmt="(a)",advance="no") "}"
>> endif
>>
>> end subroutine walk_tree
>> @@ -353,4 +396,18 @@
>> endif
>> end subroutine current_time
>>
>> +subroutine get_unit_number(lun)
>
> Is there a reason not to use m_io instead of get_unit_number?
>
>> +integer, intent(out) :: lun
>> +
>> +logical :: used
>> +integer :: iostat
>> +
>> +do lun= 10, 99
>> + inquire(unit=lun, opened=used, iostat=iostat)
>> + if (iostat .ne. 0) used = .true.
>> + if (.not. used) return
>> +enddo
>> +call die("Cannot get unit for timer output")
>> +end subroutine get_unit_number
>> +
>> end module m_timer_tree
>
>
> --
> https://code.launchpad.net/~albertog/siesta/4.0-json-time/+merge/337797
> You are the owner of lp:~albertog/siesta/4.0-json-time.