Parallel Write performance improvements - #695
Conversation
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
|
|
||
| // One channel.write() for the entire task range — one syscall, one OS I/O request. | ||
| rangeBuffer.flip(); | ||
| channel.write(rangeBuffer, baseOffset + (long) startOrdinal * recordSize).get(); |
There was a problem hiding this comment.
There is a comment in the main branch that says: This is critical for correctness as AsynchronousFileChannel.write() may not write all bytes in one call.. Doesn't this apply here as well? This .write() need not complete writing all the bytes? The previous code had a loop to account for this:
while (buffer.hasRemaining()) {
int written = channel.write(buffer, currentPosition).get();
....
}
There was a problem hiding this comment.
You are correct that the comments originally stated that the blocking Future.get() calls were necessary this proved to be a misinterpretation of an error condition that occurred during testing and the comments were not correctly updated to reflect this. In the original implementation the asynchronicity of this class was essentially broken by blocking on the Future.get() unnecessarily because of an interpretation that the AsynchronousFileChannel.write() call was not writing all bytes causing data corruption. However the root cause of the issue was later determined to be a bug in the ByteBufferIndexWriter implementation which was resolved by PR 607 (#607). The original comments never got cleaned up, but should be removed with this PR.
There was a problem hiding this comment.
Looking up the JAVA doc of AsynchronousByteChannel (which is supposed to work the same way as AsynchronousFileChannel): https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/nio/channels/AsynchronousByteChannel.html:
This method initiates an asynchronous write operation to write a sequence of bytes to this channel from the given buffer. The handler parameter is a completion handler that is invoked when the write operation completes (or fails). The result passed to the completion handler is the number of bytes written.
The write operation may write up to r bytes to the channel, where r is the number of bytes remaining in the buffer, that is, src.remaining() at the time that the write is attempted.
Future.get() returns the number of bytes written which may or may not match src.remaining() because it may write up to r bytes. Don't we need to check that all the bytes were written before proceeding?
There was a problem hiding this comment.
The situation where <r bytes are written would be extremely rare. AsynchronousFileChannel issues blocking pwrite() calls under the hood and the only time that should fail to write the full amount would be if the disk fills up partway through the write, if the filesystem quota limits are hit partway through or if the write exceeds the per-syscall write cap which is around 2G. Under normal circumstances that should never occur.
That said "extremely rare" is still a non-zero possibility so you have a point. If one of those circumstances were to occur it would basically be a silent failure which is no good. I'll push an update today to address this.
| buf.putInt(0); // neighbor count | ||
| for (int n = 0; n < graph.getDegree(0); n++) buf.putInt(-1); | ||
| buf.flip(); | ||
| pending.add(channel.write(buf, recordBase)); |
There was a problem hiding this comment.
Same thing here: do we need to account for partial writes?
There was a problem hiding this comment.
see previous comment above
| `OnDiskGraphIndexWriter`, without any code changes. | ||
|
|
||
| **Performance** | ||
| Example run writing with NVQ + FUSED_ADC features. Before this change, parallel writes |
There was a problem hiding this comment.
If we include benchmark data in docs, we need to qualify the test scenario with system specs:
CPU/RAM/Storage type and layout
| // CopyOnWriteArrayList handles concurrent additions safely | ||
| // Thread-local views for safe neighbor iteration. | ||
| // CopyOnWriteArrayList handles concurrent additions safely. | ||
| this.viewPerThread = ThreadLocal.withInitial(() -> { |
There was a problem hiding this comment.
Although it can be a twist, we should start trying to remove usage of ThreadLocal where possible. Of course, this pushes us closer to explicit task structuring, but there are caveats for virtual threads with ThreadLocal which re not ideal.
There was a problem hiding this comment.
(ah noticed the change was for comments only, sorry if superfluous)
jshook
left a comment
jshook
left a comment
There was a problem hiding this comment.
The filesystem used for testing here is not obvious. I would like to see the tests run on a small handful of fileystems at least, and this might require us to improve our GHA runner configs. It would be good to have some stabilizing performance data to compare with later, though.
| // CopyOnWriteArrayList handles concurrent additions safely | ||
| // Thread-local views for safe neighbor iteration. | ||
| // CopyOnWriteArrayList handles concurrent additions safely. | ||
| this.viewPerThread = ThreadLocal.withInitial(() -> { |
There was a problem hiding this comment.
(ah noticed the change was for comments only, sorry if superfluous)
3 participants
This PR updates the graph parallel write process to take full advantage of the async write process. Whereas before although the write tasks were batched and handed off to achieve better performance they were not truly asynchronous. This update changes that to allow fully async graph index writes and increases the benefit of using parallel writes significantly. As an example prior to this update graph write time showed speedups of 4x - 8x using parallel writes
After this update the speedup is more along the lines of 24x - 32x
This PR also introduces the ability to specify parallel writes as part of the test configuration through yaml files which is standard for BenchYAML and AutoBenchYAML test. The new parameter
parallelGraphConstructionhas been added toConstructionParametersas a Boolean value. If set toYesGrid will use parallel writes in graph construction. If set toNoGrid will use the legacy serial writing pattern. If the parameter is omitted it will default to the legacy serial writes during graph construction.