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

Fms2 io update:domain_reads #1226

Merged
merged 16 commits into from
May 25, 2023

Conversation

uramirez8707
Copy link
Contributor

@uramirez8707 uramirez8707 commented May 17, 2023

Description

  1. Modified domain_reads_2d and domain_reads_3d:
    • Root pe reads the data
    • Uses mpp_scatter to send the data to the other pes
  2. Extended mpp_scatter to work for int8
  3. Added unit tests to test all of the domain_read/domain_write interfaces
  4. Domain_reads_4d and domains_reads_5d were unchanged
    • Mpp_scatter only support 2d and 3d arrays
  5. Mpp_scatter assumes the data is (x,y,z), which may not always be the case, so I added a kludge for it …

Fixes #1218 #1219

This should give some performance gain in initialization time for high resolutions.

How Has This Been Tested?
CI, SHiELD

The initialization timings for C3072 SHiELD run:
Original:
Min: 206.756363 Max: 215.601578 Avg: 209.762848

New:
Min: 153.169189 Max: 161.906982 Avg: 156.292923

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

@uramirez8707 uramirez8707 changed the title Fms2 io update Fms2 io update:domain_reads May 18, 2023
@uramirez8707 uramirez8707 marked this pull request as ready for review May 18, 2023 11:03
@uramirez8707 uramirez8707 requested a review from ganganoaa May 18, 2023 11:03
ganganoaa
ganganoaa previously approved these changes May 19, 2023
Copy link
Contributor

@ganganoaa ganganoaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

integer :: e(2) !< The number of points (edges)
logical :: buffer_includes_halos !< .True. if vdata includes halo points
integer :: xgbegin !< Starting x index of global io domain
integer :: xgsize !< Size of global io domain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in x dimension?

integer :: xgbegin !< Starting x index of global io domain
integer :: xgsize !< Size of global io domain
integer :: ygbegin !< Starting y index of global io domain
integer :: ygsize !< Ending y index of global io domain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ygsize may not be the ending y index.

integer :: ypos !< The position of the y axis
integer :: i !< For do loops
integer :: isd !< The starting x position of the data io_domain
integer :: isc !< The starting y position of the data io_domain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't isc related to the x dimension because it is used to compute halo in that dimension?

@uramirez8707
Copy link
Contributor Author

@ganganoaa thank you for catching those documentation errors.

Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks sound.

@rem1776
Copy link
Contributor

rem1776 commented May 23, 2023

@uramirez8707 did we want to add this to main or patch 2023.01 first? there's arelease/2023.01 branch to use if we're patching, its on the same commit as main currently.

@uramirez8707
Copy link
Contributor Author

Let's patch 2023.01 first, so merge this and #1227 to release/2023.01 and tag as 2023.01.01-alpha1 so we can do more testing.

bensonr
bensonr previously approved these changes May 23, 2023
integer :: ygmin !< Starting y index of global io domain
type(FmsNetcdfDomainFile_t), intent(in) :: fileobj !< File object.
character(len=*), intent(in) :: variable_name !< Variable name.
class(*), contiguous, target, intent(inout) :: vdata(:,:) !< Data that will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to mention your comments mention writes instead of reads here and in the 3d version.

@rem1776 rem1776 changed the base branch from main to release/2023.01 May 23, 2023 20:42
@rem1776 rem1776 dismissed bensonr’s stale review May 23, 2023 20:42

The base branch was changed.

@rem1776 rem1776 merged commit c5b7b2d into NOAA-GFDL:release/2023.01 May 25, 2023
rem1776 added a commit that referenced this pull request Jun 16, 2023
* fix: fms2 io performance update for domain_reads (#1226)

* fix: fms2 io performance update for compressed writes (#1227)

* chore: build/log updates for patch (#1247)

Co-authored-by: uramirez8707 <[email protected]>
Co-authored-by: rem1776 <[email protected]>
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.

Improve fms2_io performance
4 participants