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

Issue 700 enable multi allocatable arrays #714

Merged
merged 14 commits into from
Sep 30, 2020

Conversation

vehre
Copy link
Collaborator

@vehre vehre commented Aug 17, 2020

coverage on master
Codecov branch

Summary of changes

Fixing issue-511 correctly. Adding ability to really use global dynamic window. Change to fine granular mandatory locking.

Rationale for changes

The pull reQuest is split into several commits, to simplify review. The most interesting one is the last one.

The initial fix for issue #511 while seeming effective did not work when more than one scalar array-reference was present in the references of a coarray. The last commit fixes this by dedicatedly analysing the array references and not relying on the reallocation flag, but using its own notion to figure error situations.

Furthermore had the way of locking dedicated windows and the global dynamic one by changed to a fine granular and near access locking. I.e. each MPI_Get or _Put is now encased in the appropriate lock and unlock calls. Only this made using the global dynamic window possible which then fixed #700 . At least with mpich 3.3.2. OpenMPI 4.0.2 still has several regressions, that are not understandable.

Additional info and certifications

This pull request (PR) is a:

  • Bug fix
  • Feature addition
  • Other, Please describe:

I certify that

  • I certify that:
    • I have reviewed and followed the contributing guidelines
    • I will wait at least 24 hours before self-approving the PR to give another
      OpenCoarrays developer a chance to review my proposed code
    • I have not introduced errant white space (no trailing white space or white space errors may
      be introduced)
    • I have added an explanation of what these changes do and why they should be included
    • I have checked to ensure there aren't other open Pull Requests for the same change
    • I have you written new tests for these changes
    • I have successfully tested these changes locally
    • I have commented any non-trivial, non-obvious code changes
    • The commits are logically atomic, self consistent and coherent
    • The commit messages follow best practices
    • Test coverage is maintained or increased after this is merged

Code coverage data

coverage on master

@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #714 into master will increase coverage by 0.66%.
The diff coverage is 65.16%.

@@            Coverage Diff             @@
##           master     #714      +/-   ##
==========================================
+ Coverage   54.25%   54.92%   +0.66%     
==========================================
  Files           3        3              
  Lines        2938     3028      +90     
==========================================
+ Hits         1594     1663      +69     
- Misses       1344     1365      +21     

@rouson rouson requested a review from afanfa September 10, 2020 05:31
@rouson
Copy link
Member

rouson commented Sep 25, 2020

@vehre thanks for this PR. I think it's safe to merge so I'll do so shortly. Alessarndo took a look at it for me and passed along the following question: "Why did you use MPI_Win_lock instead of a combination of MPI_Win_lockall and MPI_Win_Flush?"

@vehre
Copy link
Collaborator Author

vehre commented Sep 25, 2020 via email

@rouson
Copy link
Member

rouson commented Sep 25, 2020

@vehre I see that there's a conflict showing on this PR. Presumably that's because one or more PR's were merged into master since this PR branched off of master. It appears the conflict is trivial so I'll see if I can resolve it and then rerun the tests later today or this weekend. If you happen to get to it first, let me know.

@vehre
Copy link
Collaborator Author

vehre commented Sep 25, 2020 via email

@vehre vehre force-pushed the issue-700-enable-multi-allocatable-arrays branch from 159aa9f to 20718e7 Compare September 27, 2020 14:47
@vehre
Copy link
Collaborator Author

vehre commented Sep 27, 2020

Resolved the merge conflict.

…m:sourceryinstitute/opencoarrays into issue-700-enable-multi-allocatable-arrays
@rouson
Copy link
Member

rouson commented Sep 30, 2020

@vehre I see that you resolved the merge conflict. I just tested locally on macOS and all tests pass. I'll wait for the Linux tests to pass on Travis CI and then merge. I think something is wrong with the macOS tests on Travis CI so I'll ignore any failures there. I think we'll transition soon to using GitHub's CI capabilities, which might resolve the issue.

@rouson rouson merged commit 5853542 into master Sep 30, 2020
@rouson rouson deleted the issue-700-enable-multi-allocatable-arrays branch September 30, 2020 23:49
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.

RFE: support nested allocatable derived-type array components inside derived-type coarrays
2 participants