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

CkIO Example #3649

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

CkIO Example #3649

wants to merge 3 commits into from

Conversation

mjacob1002
Copy link
Contributor

Wrote example code to get across how to use the writing functionality of CkIO. Also tried to be thorough in the commenting so that the user can easily understand how it works. Let me know if there is anything that needs to be changed.

@mjacob1002
Copy link
Contributor Author

Could I get a review on this? Thanks!

@rbuch rbuch self-requested a review October 5, 2022 22:42
@mjacob1002 mjacob1002 requested a review from ZwFink October 6, 2022 07:02
Copy link
Contributor

@rbuch rbuch left a comment

Choose a reason for hiding this comment

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

Use the suggested changes I provided in the .ci file to serve as a model for amending you other comments in that file. Also, try to avoid using explicit line numbers in the comments since future changes to the file may alter the numbering and people will probably forget to update the line numbers in the comments.

Comment on lines +1 to +2
-include ./home/ec2-user/charm-project/charm/examples/common.mk
CHARMC=/home/ec2-user/charm-project/charm/bin/charmc $(OPTS)
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be relative paths (see the Makefiles of other examples).


all: iotest

iotest: iotest.ci iotest.C
Copy link
Contributor

Choose a reason for hiding this comment

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

The files and executable should probably be called something other than iotest since that's already the name of the test program in tests/.

@@ -0,0 +1,57 @@
#include "iotest.decl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Run this and the .ci file through some kind of formatter (I recommend clang-format, we already have a format file in the root directory of the repo). Also, remove the extra newlines if the formatter doesn't take them out (e.g. line 35 below).

Comment on lines +18 to +21
_num_iterations = atoi(msg -> argv[2]); // assign the number of files to write
int num_files = _num_iterations; // save the number of files
_files.resize(_num_iterations);
for(int i = 0; i < num_files; ++i){
Copy link
Contributor

Choose a reason for hiding this comment

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

num_files and _num_iterations look like they store the same value and are used for the same thing, I'd remove one of them.

delete msg;

}
// standard bookkeeping for how many iterations we need to go through
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// standard bookkeeping for how many iterations we need to go through
// Exit after every file has finished being written

Comment on lines +13 to +19
/**
Set the Options struct so that CkIO opens the file and sets the correct configuration for writing.
The writeStripe parameter determines the amount of contiguous bytes each writer chare will write to file.
The peStripe parameters determines how much actual data each writer chare will aggregate at a given time. This is used so that
a bunch of tiny data gets distributed across many writing chares when it would be better to all go to a single chare.
It is required that peStripe >= writeStripe.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to be a bit more concise in these descriptions of what the code is doing so the key points don't get lost too much in text.

Suggested change
/**
Set the Options struct so that CkIO opens the file and sets the correct configuration for writing.
The writeStripe parameter determines the amount of contiguous bytes each writer chare will write to file.
The peStripe parameters determines how much actual data each writer chare will aggregate at a given time. This is used so that
a bunch of tiny data gets distributed across many writing chares when it would be better to all go to a single chare.
It is required that peStripe >= writeStripe.
*/
/**
Begin the write procedure by opening the file via providing CkIO the filename and I/O options.
*/

Comment on lines +21 to +23
Ck::IO::Options opts; // struct containing the options for the writer
opts.writeStripe = 1024; // collect up to 1kB of data before writing; use the specific number you'd like or it defaults to 4MB
opts.peStripe = 4 * opts.writeStripe; // the amount of data that is aggregated by each "write chare"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ck::IO::Options opts; // struct containing the options for the writer
opts.writeStripe = 1024; // collect up to 1kB of data before writing; use the specific number you'd like or it defaults to 4MB
opts.peStripe = 4 * opts.writeStripe; // the amount of data that is aggregated by each "write chare"
Ck::IO::Options opts;
// Write to disk in contiguous chunks of 1024 bytes
opts.writeStripe = 1024;
// Each intermediate CkIO chare aggregates for contiguous 4096 bytes chunks of the file
opts.peStripe = 4 * opts.writeStripe;

*/
ckout << "starting the writing cycle for " << file_number << endl;
Ck::IO::Options opts; // struct containing the options for the writer
opts.writeStripe = 1024; // collect up to 1kB of data before writing; use the specific number you'd like or it defaults to 4MB
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this PR is old and probably just collecting dust on a shelf, but in terms of exemplifying CkIO usage, the values should probably be in the range of megabytes, to be comparable in scale to typical Lustre stripe sizes. The original design was that they should be equal to (a multiple of) the FS stripe size, to limit the contention on storage servers and the individual stripes.

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.

None yet

3 participants