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

added DTCMP sorts and used them instead of original serial sorts #227

Merged
merged 11 commits into from Mar 29, 2019

Conversation

Projects
None yet
2 participants
@levatin1
Copy link
Collaborator

commented Feb 4, 2019

No description provided.

@levatin1 levatin1 requested a review from adammoody Feb 4, 2019

@adammoody

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

Nice work, @levatin1 ! It's not trivial to figure out DTCMP calls.

Let's iterate on this a bit more. We can reuse the comparison function you have, but let's change this to do two things:

  1. define a fixed length string datatype using
MPI_Datatype dirnam_type;
MPI_Type_contiguous(dnamlen, MPI_CHAR, &dirname_type);
MPI_Type_commit(&dirname_type);
  1. rather than allocate a small piece of memory for each string, store all strings as fixed length items back-to-back in one contiguous block of memory. I think you may already have this in your dnames array.
DTCMP_Op_create(dnam_type,&dnamcomp,&op_dnamcomp);
DTCMP_Sort_local(DTCMP_IN_PLACE, dnames, dirtot, dnam_type, dnam_type, op_dnamcomp, DTCMP_FLAGS_NONE);

rather than

for (i = 0; i < dirtot; i++) darray[i] = (char*) MFU_MALLOC(dnamlen * sizeof(char));
for (i = 0; i < dirtot; i++) strncpy(darray[i], dnames + i * dnamlen, dnamlen);
block = MFU_MALLOC(dirtot * dnamlen * sizeof(char));

It will still function with these two changes, and the benefit will be that we can start with the strings distributed across ranks.

This explanation is a bit handy-wavy, so I'm happy to discuss over the phone.

@levatin1

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 6, 2019

Adam,
Since this version segfaults, could you please take a look at it. I think DTCMP has a bug with strings of characters.
JoAnne

mfu_init();
mfu_debug_level = MFU_LOG_VERBOSE;

int rank, nrank;
MPI_Comm_rank(MPI_COMM_WORLD, &rank);
MPI_Comm_size(MPI_COMM_WORLD, &nrank);
MPI_Datatype dirname_type;

This comment has been minimized.

Copy link
@adammoody

adammoody Feb 9, 2019

Member

@levatin1 , dnamlen has to be defined when we use it to create the MPI datatype. We'll want to move these three lines down to around line 782 after we assign a value to dnamlen.

/* create directory name compare function
/*------------------------------------------*/
//jll if (DTCMP_Op_create(MPI_DOUBLE,&dnamcomp,&op_dnamcomp) !=DTCMP_SUCCESS)
if (DTCMP_Op_create(dirname_type,&dnamcomp,&op_dnamcomp) !=DTCMP_SUCCESS)

This comment has been minimized.

Copy link
@adammoody

adammoody Feb 9, 2019

Member

Oh, and this will have to move down as well so that it still comes after committing dirname_type.

@adammoody

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Hi @levatin1 , given the large amount of new code in this PR, I'm guessing we'll have to iterate several times before we have this running so don't get discouraged if it keeps crashing.

One issue I see here is that dnamlen is not assigned a value before we use it to create a new MPI datatype in these lines:

MPI_Datatype dirname_type;
MPI_Type_contiguous(dnamlen, MPI_CHAR, &dirname_type);

We'll need to move those lines down after the point where dnamlen is given a value.

levatin1 and others added some commits Feb 19, 2019

@adammoody

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@levatin1 , I just pushed a commit to this branch to apply the astyle format that we're using in the project. This looks like a big commit, but it's just style changes.

@adammoody adammoody merged commit f44adda into master Mar 29, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.