Merge lp:~albertog/siesta/4.0-json-time into lp:siesta/4.0
- 4.0-json-time
- Merge into rel-4.0
Status: | Merged |
---|---|
Merged at revision: | 558 |
Proposed branch: | lp:~albertog/siesta/4.0-json-time |
Merge into: | lp:siesta/4.0 |
Diff against target: |
173 lines (+79/-12) (has conflicts) 4 files modified
Docs/siesta.tex (+6/-1) Src/siesta_analysis.F (+4/-0) Src/timer_tree.f90 (+63/-11) version.info (+6/-0) Text conflict in version.info |
To merge this branch: | bzr merge lp:~albertog/siesta/4.0-json-time |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Nick Papior | Approve | ||
Review via email: mp+337797@code.launchpad.net |
Commit message
Description of the change
When using the tree-timer, a 'time.json' file in JSON format is written.
This file can be used by third-party scripts to process timing data.
Also, add a new intermediate timing section for 'siesta_analysis'.
Alberto Garcia (albertog) wrote : | # |
Nick Papior (nickpapior) 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.
Alberto Garcia (albertog) wrote : | # |
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_
>> --- 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_
>> + open(unit=
>> + action=
>> +
>> write(*
>> - call walk_tree(p,0)
>>
>> + write(js_lun,*) '{'
>> + call walk_tree(
>> + write(js_lun,*) '}'
>> + if (json_output) close(js_lun)
>> +
>> end subroutine timer_report_global
>>
>> !------
>> - recursive subroutine walk_tree(
>> + recursive subroutine walk_tree(
>> 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,
>> +
>> write(*,fmtstr) pd%name, pd%nCalls, &
>> pd%totTime, 100*pd%
>> + if (json_output) then
>> + write(fmt_
>> + write(fmt_
>> + write(js_
>> + '"' // trim(pd%name) // '": { "_calls": ', pd%nCalls, &
>> ...
Nick Papior (nickpapior) wrote : | # |
Dear Alberto,
m_io: no worries. I wouldn't mind having that dependency, but your call.
tree vs branch: I see, perhaps we could mark them with "leaf_calls" which clarifies this?
Agreed! ;)
- 559. By Alberto Garcia
-
Use only the presence of json unit as signal to dump JSON
(Thanks to Nick Papior)
We keep the get_unit_number internal routine for now, to avoid
an extra dependency on m_io.
Nick Papior (nickpapior) wrote : | # |
@Alberto, should I merge in this branch, or will you?
Alberto Garcia (albertog) wrote : | # |
I will merge it shortly.
----- El 20 de Feb 2018, a las 21:15, Nick Rubner Papior <email address hidden> escribió:
| @Alberto, should I merge in this branch, or will you?
| --
| https:/
| You are the owner of lp:~albertog/siesta/4.0-json-time.
Preview Diff
1 | === modified file 'Docs/siesta.tex' | |||
2 | --- Docs/siesta.tex 2017-12-28 13:00:52 +0000 | |||
3 | +++ Docs/siesta.tex 2018-02-19 13:31:01 +0000 | |||
4 | @@ -6595,7 +6595,12 @@ | |||
5 | 6595 | \index{UseTreeTimer@{\bf UseTreeTimer}} | 6595 | \index{UseTreeTimer@{\bf UseTreeTimer}} |
6 | 6596 | 6596 | ||
7 | 6597 | Enable an experimental timer which is based on wall time on the master | 6597 | Enable an experimental timer which is based on wall time on the master |
9 | 6598 | node and is aware of the tree-structure of the timed sections. | 6598 | node and is aware of the tree-structure of the timed sections. At the |
10 | 6599 | end of the program, a report is generated in the output file, and a | ||
11 | 6600 | {\tt time.json} file in JSON format is also | ||
12 | 6601 | written. \index{JSON timing report@{\bf JSON timing report}} | ||
13 | 6602 | This file can be | ||
14 | 6603 | used by third-party scripts to process timing data. | ||
15 | 6599 | 6604 | ||
16 | 6600 | {\it Default value:} {\tt .false.} | 6605 | {\it Default value:} {\tt .false.} |
17 | 6601 | 6606 | ||
18 | 6602 | 6607 | ||
19 | === modified file 'Src/siesta_analysis.F' | |||
20 | --- Src/siesta_analysis.F 2017-10-04 09:48:27 +0000 | |||
21 | +++ Src/siesta_analysis.F 2018-02-19 13:31:01 +0000 | |||
22 | @@ -94,6 +94,8 @@ | |||
23 | 94 | endif | 94 | endif |
24 | 95 | endif | 95 | endif |
25 | 96 | 96 | ||
26 | 97 | call timer( 'siesta_analysis', 1 ) | ||
27 | 98 | |||
28 | 97 | ! All the comments below assume that this compatibility option | 99 | ! All the comments below assume that this compatibility option |
29 | 98 | ! is not used. | 100 | ! is not used. |
30 | 99 | ! Note also that full compatibility cannot be guaranteed | 101 | ! Note also that full compatibility cannot be guaranteed |
31 | @@ -456,6 +458,8 @@ | |||
32 | 456 | ! -- Set xa to xa_last at the top of this file. Write any "next iter" coordinate file | 458 | ! -- Set xa to xa_last at the top of this file. Write any "next iter" coordinate file |
33 | 457 | ! in 'siesta_move' | 459 | ! in 'siesta_move' |
34 | 458 | 460 | ||
35 | 461 | call timer( 'siesta_analysis', 2 ) | ||
36 | 462 | |||
37 | 459 | !--------------------------------------------------------------------------- END | 463 | !--------------------------------------------------------------------------- END |
38 | 460 | END subroutine siesta_analysis | 464 | END subroutine siesta_analysis |
39 | 461 | END module m_siesta_analysis | 465 | END module m_siesta_analysis |
40 | 462 | 466 | ||
41 | === modified file 'Src/timer_tree.f90' | |||
42 | --- Src/timer_tree.f90 2016-01-25 16:00:16 +0000 | |||
43 | +++ Src/timer_tree.f90 2018-02-19 13:31:01 +0000 | |||
44 | @@ -218,9 +218,10 @@ | |||
45 | 218 | end subroutine timer_all_off | 218 | end subroutine timer_all_off |
46 | 219 | 219 | ||
47 | 220 | !------------------------------------------------ | 220 | !------------------------------------------------ |
49 | 221 | subroutine timer_report(secname) | 221 | subroutine timer_report(secname,file_unit) |
50 | 222 | character(len=*), optional :: secname | 222 | character(len=*), optional :: secname |
52 | 223 | 223 | integer, intent(in), optional :: file_unit | |
53 | 224 | |||
54 | 224 | integer :: i, loc | 225 | integer :: i, loc |
55 | 225 | logical :: full | 226 | logical :: full |
56 | 226 | 227 | ||
57 | @@ -253,7 +254,7 @@ | |||
58 | 253 | !------------------------------------------------ | 254 | !------------------------------------------------ |
59 | 254 | subroutine timer_report_global() | 255 | subroutine timer_report_global() |
60 | 255 | 256 | ||
62 | 256 | integer :: i | 257 | integer :: i, js_lun |
63 | 257 | type(times_t), pointer :: qd | 258 | type(times_t), pointer :: qd |
64 | 258 | 259 | ||
65 | 259 | p => global_section | 260 | p => global_section |
66 | @@ -266,31 +267,68 @@ | |||
67 | 266 | enddo | 267 | enddo |
68 | 267 | p%data%totTime = globaltime + 1.0e-6_dp | 268 | p%data%totTime = globaltime + 1.0e-6_dp |
69 | 268 | 269 | ||
70 | 270 | ! Open JSON | ||
71 | 271 | call get_unit_number(js_lun) | ||
72 | 272 | open(unit=js_lun, file="time.json", form="formatted", & | ||
73 | 273 | action="write",position="rewind") | ||
74 | 274 | |||
75 | 269 | write(*,"(/,a20,T30,a6,a12,a8)") "Section","Calls","Walltime","%" | 275 | write(*,"(/,a20,T30,a6,a12,a8)") "Section","Calls","Walltime","%" |
78 | 270 | call walk_tree(p,0) | 276 | ! Due to border logic we need this top-level wrapping |
79 | 271 | 277 | write(js_lun,"(a)") "{" | |
80 | 278 | call walk_tree(p,0, js_lun=js_lun) | ||
81 | 279 | write(js_lun,"(a)") "}" | ||
82 | 280 | |||
83 | 281 | close(js_lun) | ||
84 | 282 | |||
85 | 272 | end subroutine timer_report_global | 283 | end subroutine timer_report_global |
86 | 273 | 284 | ||
87 | 274 | !------------------------------------------------ | 285 | !------------------------------------------------ |
89 | 275 | recursive subroutine walk_tree(p,level,maxlevel) | 286 | recursive subroutine walk_tree(p,level,maxlevel,js_lun) |
90 | 276 | type(section_t), intent(in),target :: p | 287 | type(section_t), intent(in),target :: p |
91 | 277 | integer, intent(in) :: level | 288 | integer, intent(in) :: level |
92 | 278 | integer, intent(in), optional:: maxlevel | 289 | integer, intent(in), optional:: maxlevel |
93 | 290 | integer, intent(in), optional:: js_lun | ||
94 | 279 | 291 | ||
95 | 280 | integer :: i | 292 | integer :: i |
97 | 281 | character(len=40) fmtstr | 293 | character(len=64) fmtstr, fmt_json, fmt_json_head |
98 | 294 | logical :: json_output | ||
99 | 282 | 295 | ||
100 | 296 | ! Determine whether we should output JSON | ||
101 | 297 | ! If the unit is there, we output to JSON as well | ||
102 | 298 | json_output = present(js_lun) | ||
103 | 299 | |||
104 | 283 | if (present(maxlevel)) then | 300 | if (present(maxlevel)) then |
105 | 284 | if (level > maxlevel) RETURN | 301 | if (level > maxlevel) RETURN |
107 | 285 | endif | 302 | end if |
108 | 303 | |||
109 | 286 | pd => p%data | 304 | pd => p%data |
110 | 287 | write(fmtstr,"(a,i0,a1,a)") "(", level+1, "x", ",a20,T30,i6,f12.3,f8.2)" | 305 | write(fmtstr,"(a,i0,a1,a)") "(", level+1, "x", ",a20,T30,i6,f12.3,f8.2)" |
113 | 288 | write(*,fmtstr) pd%name, pd%nCalls, & | 306 | |
114 | 289 | pd%totTime, 100*pd%totTime/globaltime | 307 | write(*,fmtstr) pd%name, pd%nCalls, pd%totTime, 100*pd%totTime/globaltime |
115 | 308 | if (json_output) then | ||
116 | 309 | write(fmt_json,"(a,i0,a1,a)") "(",2*level+1,"x",",a,i0,a,f12.3,a,f8.2)" | ||
117 | 310 | write(fmt_json_head,"(a,i0,a)") "(", 2*level+1, "x,a)" | ||
118 | 311 | write(js_lun,fmt=fmt_json,advance="no") & | ||
119 | 312 | '"' // trim(pd%name) // '": { "_calls": ', pd%nCalls, & | ||
120 | 313 | ', "_time": ', pd%totTime, & | ||
121 | 314 | ', "_%": ', 100*pd%totTime/globaltime | ||
122 | 315 | end if | ||
123 | 290 | if (p%nchildren /= 0) then | 316 | if (p%nchildren /= 0) then |
124 | 317 | if (json_output) write(js_lun,"(a)") "," | ||
125 | 291 | do i=1,p%nchildren | 318 | do i=1,p%nchildren |
127 | 292 | call walk_tree(p%child(i),level+1,maxlevel) | 319 | call walk_tree(p%child(i),level+1,maxlevel, js_lun) |
128 | 320 | if (json_output) then | ||
129 | 321 | if (i < p%nchildren) then | ||
130 | 322 | write(js_lun,fmt="(a)") ',' | ||
131 | 323 | else | ||
132 | 324 | write(js_lun,*) ! just new line | ||
133 | 325 | endif | ||
134 | 326 | endif | ||
135 | 293 | enddo | 327 | enddo |
136 | 328 | ! End the parent section with an indented '}' | ||
137 | 329 | if (json_output) write(js_lun,fmt=fmt_json_head,advance="no") "}" | ||
138 | 330 | else | ||
139 | 331 | if (json_output) write(js_lun,fmt="(a)",advance="no") "}" | ||
140 | 294 | endif | 332 | endif |
141 | 295 | 333 | ||
142 | 296 | end subroutine walk_tree | 334 | end subroutine walk_tree |
143 | @@ -353,4 +391,18 @@ | |||
144 | 353 | endif | 391 | endif |
145 | 354 | end subroutine current_time | 392 | end subroutine current_time |
146 | 355 | 393 | ||
147 | 394 | subroutine get_unit_number(lun) | ||
148 | 395 | integer, intent(out) :: lun | ||
149 | 396 | |||
150 | 397 | logical :: used | ||
151 | 398 | integer :: iostat | ||
152 | 399 | |||
153 | 400 | do lun= 10, 99 | ||
154 | 401 | inquire(unit=lun, opened=used, iostat=iostat) | ||
155 | 402 | if (iostat .ne. 0) used = .true. | ||
156 | 403 | if (.not. used) return | ||
157 | 404 | enddo | ||
158 | 405 | call die("Cannot get unit for timer output") | ||
159 | 406 | end subroutine get_unit_number | ||
160 | 407 | |||
161 | 356 | end module m_timer_tree | 408 | end module m_timer_tree |
162 | 357 | 409 | ||
163 | === modified file 'version.info' | |||
164 | --- version.info 2018-02-19 08:15:36 +0000 | |||
165 | +++ version.info 2018-02-19 13:31:01 +0000 | |||
166 | @@ -1,1 +1,7 @@ | |||
167 | 1 | <<<<<<< TREE | ||
168 | 1 | siesta-4.0--557 | 2 | siesta-4.0--557 |
169 | 3 | ======= | ||
170 | 4 | siesta-4.0--555--json-time-4 | ||
171 | 5 | |||
172 | 6 | |||
173 | 7 | >>>>>>> MERGE-SOURCE |
I forgot to add that this has been prompted by the need to set up a benchmarking workflow in AiiDA. The Siesta plugin now processes the timing information in the time.json file.