-
Notifications
You must be signed in to change notification settings - Fork 58
Calipers performance25 #176
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
Conversation
c202c23 to
5c9f1d0
Compare
|
timer.txt ✅: timer'txt'
|
|
testing is complete, this is ready for Sci/Tech review @tommbendall |
tommbendall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @mo-marqh
I spotted one unclosed timer and a typo (that wasn't yours). In the timer.txt that you posted, I noticed several 'control_*' calipers appear in gungho_transport_control. Could you also remove these?
Other than that should be good to go.
| if (du_tot_skeb_flag) call du_tot_skeb%write_field('stochastic__du_tot_skeb') | ||
| if (dv_tot_skeb_flag) call dv_tot_skeb%write_field('stochastic__dv_tot_skeb') | ||
| end if | ||
| if ( LPROF ) call start_timing( id_diags, 'diags.skeb' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if ( LPROF ) call start_timing( id_diags, 'diags.skeb' ) | |
| if ( LPROF ) call stop_timing( id_diags, 'diags.skeb' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in b70d7a6
| if ( LPROF ) call start_timing( id_skeb, 'skeb.ndisp' ) | ||
|
|
||
| !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! | ||
| !! 1) Initialize feilds and operators !! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| !! 1) Initialize feilds and operators !! | |
| !! 1) Initialize fields and operators !! |
Just because I spotted this typo while scrolling through!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in b70d7a6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove all of the 'control_*' timers from this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in b70d7a6
|
thanks @tommbendall please confirm whether this is now okay by you? developer tests ran and passed Test Suite Results - lfric_apps - calipersPerformance25/run7Suite Information
Task Information✅ succeeded tasks - 1106 |
tommbendall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for doing this. I'm very happy to have these organised nicely.
This passes science review, so over to @oakleybrunt
|
I am currently doing this review and noticed that there is quite a lot of inconsistent use of the Since the Taking the This is just my view and I am open to discussing whether this is a convention we would like to follow or not. So please reply to this with your views on this. |
the naming of this integer value is just within the scope of each code section that is calling one or more timers. I'm not keen to rename further timer_index integers in code committed in #80 within the scope of this PR, which is already large. I feel like the timer names are what should be searchable, and that using the tik integers as search items will always be fragile. I do understand your desire for consistency @oakleybrunt , i just fear that it'll be difficult to deliver in this PR, and then hard to maintain. |
Thanks Mark, I see your perspective and perhaps trying to define a convention as part of this PR is too ambitious. I think that I will start a wider discussion on naming conventions for timer hash ids instead. |
i think that consistency in the string identity naming for each timer is more useful & important. |
oakleybrunt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, naming conventions for hash argument not covered in this PR. Otherwise, changes are good. Thanks - approved.
resolved |
PR Summary
Sci/Tech Reviewer: @tommbendall
Code Reviewer: @oakleybrunt
Comprehensive timing caliper location review.
This is a rework of work done for performance analyses in late 2025, migrating everything caliper related from:
https://code.metoffice.gov.uk/trac/lfric_apps/browser/main/branches/pkg/Share/r15393_kpi_benchmark
essentially from this commit:
https://code.metoffice.gov.uk/trac/lfric_apps/changeset?reponame=&new=15498%40main%2Fbranches%2…
but updated from source migration and adoption of
timingwrapper from #80Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - calipersPerformance25/run6
Suite Information
Task Information
✅ succeeded tasks - 1456
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review