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

negative throughput numbers reported #511

Closed
tclune opened this issue Jul 31, 2020 · 7 comments
Closed

negative throughput numbers reported #511

tclune opened this issue Jul 31, 2020 · 7 comments
Assignees
Labels
0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.)

Comments

@tclune
Copy link
Collaborator

tclune commented Jul 31, 2020

Several users have reported negative values for throughput from MAPL_Cap.

The timers are using system_clock() with default integers instead of 64 bit which greatly increases the odds that the clock will "roll over" during a run. Should be a simple fix.

@tclune tclune added the 0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) label Jul 31, 2020
@tclune
Copy link
Collaborator Author

tclune commented Jul 31, 2020

Mentioning @lizziel, who reported this concern.

@mathomp4
Copy link
Member

@tclune Would moving our system_clock() calls to int64 be sufficient? Or should we move to something like MPI_Wtime()?

@tclune
Copy link
Collaborator Author

tclune commented Jul 31, 2020

I don't think accuracy is a concern. Just the rollover at MAX_TICK. But I'm not opposed to an MP_Wtime() solution either.

@mathomp4
Copy link
Member

Well, we have a good mix.

Files using system_clock:

❯ rg system_clock -l
base/MAPL_Cap.F90
base/cub2latlon_regridder.F90
base/tests/testbin.F90
base/MAPL_Profiler.F90
profiler/FortranTimerGauge.F90
pfio/pfio_io_demo.F90
pfio/pfio_collective_demo.F90
pfio/tests/pfio_ctest_io.F90

Files using MPI_Wtime():

❯ rg mpi_wtime -l
base/MAPL_Generic.F90
base/Regrid_Util.F90
base/MAPL_IO.F90
base/MAPL_CapGridComp.F90
profiler/MpiTimerGauge.F90
pfio/BaseServer.F90
pfio/tests/pfio_performance.F90

Luckily we have no files using both! 😄

@tclune
Copy link
Collaborator Author

tclune commented Jul 31, 2020

Yeah - I (probably for no good reason) tend to favor the Fortran intrinsics over use of a library. But if one knows that MPI is always being linked, it's hard to argue against MPI_Wtime().

All of the ones you found that are not tests/demos should be considered for adoption of the new profiler. (Except of course, inside the profiler itself which intentionally provides multiple interfaces.)

@mathomp4
Copy link
Member

mathomp4 commented Feb 22, 2021

@tclune @weiyuan-jiang Has the new profiler been spread enough in GEOS to consider this "fixed"? It does look like we still have a good mix:

rg mpi_wtime -l | sort
base/MAPL_Generic.F90
base/MAPL_IO.F90
gridcomps/Cap/MAPL_CapGridComp.F90
pfio/BaseServer.F90
pfio/MultiLayerServer.F90
pfio/tests/pfio_performance.F90
profiler/MpiTimerGauge.F90rg system_clock -l | sort
base/MAPL_Profiler.F90
base/cub2latlon_regridder.F90
base/tests/testbin.F90
gridcomps/Cap/MAPL_Cap.F90
pfio/pfio_collective_demo.F90
pfio/pfio_io_demo.F90
pfio/tests/pfio_ctest_io.F90
profiler/FortranTimerGauge.F90

@mathomp4 mathomp4 added the triage label Mar 8, 2021
mathomp4 added a commit that referenced this issue Mar 22, 2021
mathomp4 added a commit that referenced this issue Mar 23, 2021
…ock-int64

Fixes #511. Use INT64 for SYSTEM_CLOCK
@mathomp4
Copy link
Member

Reexamined on 2021-Apr-12 with @bena-nasa and @tclune

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.)
Projects
None yet
Development

No branches or pull requests

3 participants