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

Provide high bandwidth performance for bulk frame writes. #236

Closed
joaander opened this issue Apr 24, 2023 · 6 comments
Closed

Provide high bandwidth performance for bulk frame writes. #236

joaander opened this issue Apr 24, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@joaander
Copy link
Member

joaander commented Apr 24, 2023

Description

Allow gsd to provide high bandwidth writes while ensuring file integrity.

Proposed solution

  • Split gsd_end_frame() into gsd_end_frame() and gsd_commit().
  • Call gsd_commit() in gsd_close().

The new gsd_end_frame() will only increment the frame counter:

gsd/gsd/gsd.c

Lines 1875 to 1887 in aad91d2

int gsd_end_frame(struct gsd_handle* handle)
{
if (handle == NULL)
{
return GSD_ERROR_INVALID_ARGUMENT;
}
if (handle->open_flags == GSD_OPEN_READONLY)
{
return GSD_ERROR_FILE_MUST_BE_WRITABLE;
}
// increment the frame counter
handle->cur_frame++;

The new gsd_commit() will flush the buffers and sync:

gsd/gsd/gsd.c

Lines 1889 to 1954 in aad91d2

// flush the namelist buffer
int retval = gsd_flush_name_buffer(handle);
if (retval != GSD_SUCCESS)
{
return retval;
}
// flush the write buffer
retval = gsd_flush_write_buffer(handle);
if (retval != GSD_SUCCESS)
{
return retval;
}
// sync the data before writing the index
retval = fsync(handle->fd);
if (retval != 0)
{
return GSD_ERROR_IO;
}
// write the frame index to the file
if (handle->frame_index.size > 0)
{
// ensure there is enough space in the index
if ((handle->file_index.size + handle->frame_index.size) > handle->file_index.reserved)
{
gsd_expand_file_index(handle, handle->file_index.size + handle->frame_index.size);
}
// sort the index before writing
retval = gsd_index_buffer_sort(&handle->frame_index);
if (retval != 0)
{
return retval;
}
// write the frame index entries to the file
int64_t write_pos = handle->header.index_location
+ sizeof(struct gsd_index_entry) * handle->file_index.size;
size_t bytes_to_write = sizeof(struct gsd_index_entry) * handle->frame_index.size;
ssize_t bytes_written
= gsd_io_pwrite_retry(handle->fd, handle->frame_index.data, bytes_to_write, write_pos);
if (bytes_written == -1 || bytes_written != bytes_to_write)
{
return GSD_ERROR_IO;
}
#if !GSD_USE_MMAP
// add the entries to the file index
memcpy(handle->file_index.data + handle->file_index.size,
handle->frame_index.data,
sizeof(struct gsd_index_entry) * handle->frame_index.size);
#endif
// update size of file index
handle->file_index.size += handle->frame_index.size;
// clear the frame index
handle->frame_index.size = 0;
}
return GSD_SUCCESS;
}

Additional context

With this API, the caller can push many frames into the in-memory buffer and flush them all at an appropriate time (e.g. after buffering a full batch). Otherwise, the remaining buffer will be written when the file is closed.

#232 introduced a per-frame fsync call to ensure data integrity in the file. This lowers performance on long latency filesystems. On Frontier's Orion, realistic uses in HOOMD-blue are limited to ~10 frames per second written to the file. Commenting out the fsync gives ~10x performance improvements, depending on system size as the writes are now bandwidth limited. This proposed API allows the caller to gain the bandwidth efficiency by batching many frames in memory and finally writing the buffer with gsd_commit(), ensuring data integrity for the batch.

As an API breaking change, this should be introduced in gsd 3.0.

@joaander joaander added the enhancement New feature or request label Apr 24, 2023
@joaander
Copy link
Member Author

@b-butler any thoughts or suggestions?

@joaander
Copy link
Member Author

On second thought, this does not need to be API breaking. Make the new methods gsd_buffer_frame() and gsd_commit(). Then:

gsd_end_frame()
    {
    gsd_buffer_frame();
    gsd_commit();
    }

remains with the current behavior, but tools can opt-in to the new API.

@b-butler
Copy link
Member

I like the proposed option of creating two new functions which gsd_end_frame performs, except perhaps that then gsd_buffer_frame also increments the frame number preventing new data to be written to that frame.

@joaander
Copy link
Member Author

Yes, the buffer frame method would increment the frame counter. Maybe a better name is gsd_buffer_end_frame() / gsd_buffer_commit().

@joaander
Copy link
Member Author

Unfortunately, the implementation is not so simple as I wrote in the description. The commit call should not write out partial frame data that has not been ended. Otherwise, a killed job might include an incomplete frame at the end. Therefore, we will need another index buffer to track the buffered index writes separate from those that are specifically buffered for the current (partial frame).

On second thought, I would like to provide high performance for all users. GSD versions 2.0 - 2.8.0 buffered output (at the OS's discretion), so we could release 2.9 (or call it 3.0 if concerned) that buffers output internally and requires either a gsd_flush or a gsd_close to write the final buffered data. I'm working on this now. If possible, I will include an explicit API that dupin can use to attain higher performance. At a minimum, I will make the buffer size configurable by the caller.

One challenge with this is that Python treats SIGTERM as an instant kill (https://stackoverflow.com/questions/9930576/python-what-is-the-default-handling-of-sigterm). SLURM (and I presume other job queues) send SIGTERM first, then SIGKILL after a timeout (SLURM's default is 30s). For users to obtain partial output from a cancelled job, they will either need to flush manually, or set a SIGTERM handler that allows a graceful shutdown (implicitly calling gsd_close() in a destructor for example). We could aid users by installing such a handler by default when importing gsd and/or HOOMD.

The HOOMD tutorials that write a gsd file and read in the same notebook will need to call flush.

@joaander
Copy link
Member Author

joaander commented May 1, 2023

Completed in #237.

@joaander joaander closed this as completed May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants