You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/18 21:37:32 UTC
[GitHub] [arrow] marsupialtail opened a new pull request, #13640: [Python/C++] O_DIRECT write support
marsupialtail opened a new pull request, #13640:
URL: https://github.com/apache/arrow/pull/13640
This PR attempts to add direct IO writes to output stream.
The C++ API involves adding a DirectOutputStream class which can be instantiated by calling open_output_stream on a LocalFileSystem with use_directio = True.
The DirectOutputStream implements O_DIRECT writes aligned to 4096 bytes. It performs memcpy to ensure that the address it writes from is always aligned to 4096 bytes. Most disks currently used today has a sector size of 512, but newer disks might have 4096 sector size. It buffers remainder bytes from write calls, which will be padded and flushed upon close().
In Python, if you instantiate a LocalFileSystem with use_directio = True, open_output_stream will return a NativeFile that wraps the DirectOutputStream.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924994670
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
> Also note that any use of synchronous writes means your main thread may block when writing
Fortunately the dataset writer has already been written with blocking writes in mind :wink: . Writes are issued on the I/O thread pool and a backpressure mechanism is in place. In fact, the inability of that backpressure mechanism to prevent swapping/thrashing is exactly what motivated this JIRA in the first place.
> My intuition is just that you may get glass jaws in real-world performance that you won't observe using a simple micro-benchmark on an otherwise local storage.
I'm not sure what you mean by this. Is this a concern regarding direct vs fadvise or a concern regarding sync vs async?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r957865732
##########
cpp/src/arrow/util/io_util.cc:
##########
@@ -1129,6 +1130,14 @@ Result<FileDescriptor> FileOpenWritable(const PlatformFilename& file_name,
oflag |= O_RDWR;
}
+ if (direct) {
+#if defined(__linux__)
+ oflag |= O_DIRECT; // will cause issues on Apple on Windows
+#else
+ return Status::IOError("Direct IO does not support Mac OS.");
Review Comment:
```suggestion
return Status::IOError("Direct IO is only supported on Linux.");
```
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +379,99 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length > 0) {
+ std::memset(aligned_cached_data_ + cached_length, 0, 4096 - cached_length);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, 4096));
+ }
+ auto new_length = Tell().ValueOrDie() - 4096 + cached_length;
+ // this is just to help compile. If you are not on Linux you would have failed out way
+ // before this point.
Review Comment:
I'm not sure I follow this comment
##########
cpp/src/arrow/filesystem/test_util.cc:
##########
@@ -883,7 +883,9 @@ void GenericFileSystemTest::TestOpenOutputStream(FileSystem* fs) {
ASSERT_OK_AND_ASSIGN(stream, fs->OpenOutputStream("CD/ghi"));
ASSERT_OK(stream->Write("some "));
ASSERT_OK(stream->Write(Buffer::FromString("data")));
- ASSERT_OK_AND_EQ(9, stream->Tell());
+ // this following line will cause Direct IO file system to fail
+ // since they will buffer up the nine bytes and not write them!
+ // ASSERT_OK_AND_EQ(9, stream->Tell());
Review Comment:
You could presumably overwrite the implementation of `Tell` to include the buffered bytes right? Thought I'm not sure if that is important.
##########
cpp/src/arrow/io/file.h:
##########
@@ -80,6 +80,52 @@ class ARROW_EXPORT FileOutputStream : public OutputStream {
std::unique_ptr<FileOutputStreamImpl> impl_;
};
+class ARROW_EXPORT DirectFileOutputStream : public OutputStream {
+ public:
+ ~DirectFileOutputStream() override;
+
+ /// \brief Open a local file for writing, truncating any existing file
+ /// \param[in] path with UTF8 encoding
+ /// \param[in] append append to existing file, otherwise truncate to 0 bytes
+ /// \return an open FileOutputStream
+ ///
+ /// When opening a new file, any existing file with the indicated path is
+ /// truncated to 0 bytes, deleting any existing data
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(const std::string& path,
+ bool append = false);
+
+ /// \brief Open a file descriptor for writing. The underlying file isn't
+ /// truncated.
+ /// \param[in] fd file descriptor
+ /// \return an open FileOutputStream
+ ///
+ /// The file descriptor becomes owned by the OutputStream, and will be closed
+ /// on Close() or destruction.
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(int fd);
+
+ // OutputStream interface
+ Status Close() override;
+ bool closed() const override;
+ Result<int64_t> Tell() const override;
+
+ // Write bytes to the stream. Thread-safe
+ Status Write(const void* data, int64_t nbytes) override;
+ /// \cond FALSE
+ using Writable::Write;
+ /// \endcond
+
+ int file_descriptor() const;
+
+ private:
+ DirectFileOutputStream();
+
+ class ARROW_NO_EXPORT DirectFileOutputStreamImpl;
Review Comment:
I had no idea we had `ARROW_NO_EXPORT`. I wonder if we could've used this to avoid our other PIMPL problems like what we ran into with Cuda recently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924958407
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
I ran the experiments myself with 1GB of data for timing. Oddly, on an HHD, I didn't observe much difference. On an SSD, the difference was rather considerable (I reran the ssd tests a number of times and the difference was consistent):
Method | Disk | Time (ms)
--- | --- | ---
direct | hhd | 41290.55257
direct | ssd | 4641.38446
fadvise1 | hhd | 41179.51826
fadvise1 | ssd | 6959.85842
The direct I/O results roughly match what I get for a disk benchmark on the SSD. However, I feel the test is still cheating. The `direct.cpp` example is assuming the memory is previously unaligned. Can you create a version that does not assume this (e.g. memcpy into a temp buffer before writing)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1221236445
Let's recap here -- we started from this PR, then we went to https://github.com/apache/arrow/pull/13662 because we think O_DIRECT is not needed. Turns out fadvise performance is bad, and we should revert to O_DIRECT. We will be doing O_DIRECT without O_SYNC, because we do not need to persist the writes. The direct IO here is not for persistence/fault tolerance but rather to relieve memory pressure, so you don't care if the data is persisted on disk or in "limbo" (most likely SSD cache) as long as it's not in page cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1188354696
:warning: Ticket **has no components in JIRA**, make sure you assign one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1224212517
Thats right. I'm trying to figure out why the OS gating doesn't seem to work in the CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231146444
The Tell() for DirectFileOutputStream is a bit annoying. You could have its behavior to be return the bytes written to the stream, including the cached length, or the bytes actually written to the file. Other OutputStream implementations don't have this distinction. Taking either route will fail some tests in our current test suite. I chose the latter approach since it fails fewer tests (1) vs the former (13). I just removed the ASSERT that fails the latter approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r925007982
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
> I'm not sure what you mean by this. Is this a concern regarding direct vs fadvise or a concern regarding sync vs async?
Both. TBH, it's an entirely unsubstantiated intuition, but taking full control of how and when writes happen (as opposed to letting the kernel decide) means you'll face the same set of issues that kernel developers have dealt with and solved other the years.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13640: [Python/C++] O_DIRECT write support
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1188342950
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924058255
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
`posix_memalign`?
Actually, I'm not sure if an internal buffer is beneficial. AFAIK, direct IO is only for advanced apps like DMBS which implements their own buffer management and dislikes additional buffering (e.g., OS page cache). Looks to me Arrow as a library should not change this behaviour. Another issue is that we'd better leave IO alignment to the apps, as it depends heavily on OS and hardware, hardcode to 4096 implicitly is not good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1227765779
Really hope can pass CI now, this is a script you can use to test performance:
```
SIZE = 1024 * 1024
N = 100 * 32
import time
import os
import pyarrow.fs as fs
#da = fs.LocalFileSystem(use_directio=True)
da = fs.LocalFileSystem(use_directio=True)
#da = fs.LocalFileSystem(reuse=False)
s = da.open_output_stream("bump")
a = bytes("1","utf-8") * SIZE
start = time.time()
for i in range(N):
s.write(a)
s.close()
os.sync()
print(time.time() - start)
```
On my NVME SSD direct IO is about 50% faster than non-direct IO writing this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231898062
Anything that relies on AssertFileContents, since it contains this line:
` ASSERT_OK_AND_ASSIGN(FileInfo info, fs->GetFileInfo(path));
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1490653231
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward π
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924639199
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
And `posix_fadvise` is a much more generally applicable solution than `O_DIRECT`, which might very well be detrimental in many use cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail closed pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail closed pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
URL: https://github.com/apache/arrow/pull/13640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924782442
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
For the record, when compiling your example I get:
```
fadvise.cpp: In function βint main()β:
fadvise.cpp:18:32: warning: integer overflow in expression of type βintβ results in β-2147483648β [-Woverflow]
18 | posix_fadvise(fd, 0, N * SIZE, POSIX_FADV_DONTNEED);
| ^
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960978713
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,109 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
Review Comment:
The use of std vector is not just to implement the alignment, but also resolve memory freeing issues especially if the function fails in the middle and returns some Invalid status in the future. If you use std vector they will automatically be destroyed, if you had used posix_memalign that memory region will not be freed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231909390
Yeah you are right. I think I had a bug :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1268982725
can you review again @pitrou
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1244032919
should be good to go now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924638408
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
I have trouble believing that `posix_fadvise` is really that much slower. You can also choose whatever granularity for `posix_fadvise` calls if you want to amortize the system call cost.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1189186899
https://issues.apache.org/jira/browse/ARROW-14635
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924807723
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
Just to be clear, my experiment was with `O_DSYNC` not with `posix_fadvise`. With `O_DSYNC`, writing to a single file runs at 0.406 GB/s, while with `O_DIRECT` I get 3.50 GB/s and with no sync flag I get 4.23 GB/s. This is on a Samsung 970 Evo NVMe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924799071
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
See also https://stackoverflow.com/a/3756466 which suggests a more sophisticated approach (perhaps too sophisticated?).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959848631
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
Review Comment:
I see, then how about using a ifdef around the entire implementation? It would definitely be clearer and less error-prone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959864363
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
Review Comment:
Spent a while trying not to hard code 4096 actually at the way beginning. I think hard coding it to 4096 is the best solution. Firstly this is not the page size. This is the physical disk sector size. Getting this size in C is a bit tricky. The best way is to do something like this
```
#include <linux/fs.h>
#include <iostream>
#include <stdio.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <errno.h>
#include <string.h>
int main(int argc, char **argv)
{
long logicalsectsize = 0;
int retval;
int fd = open("/dev/nvme0n1p2", O_RDONLY);
std::cout << fd << std::endl;
std::cout << strerror(errno) << std::endl;
/*retval=ioctl(fd, BLKSSZGET, &logicalsectsize);*/
if (ioctl(fd, BLKSSZGET, &logicalsectsize) < 0)
{
printf("Can't get logical sector size. Return code: %d\n",retval);
logicalsectsize = 0;
}
std::cout << logicalsectsize << std::endl;
}
```
However, this could run into permissions issues and on my machine at least can only be run under sudo.
4096 I think is a good hard coded number. Current disk sector sizes are either 512 or 4096, with more and more disks expected to become 4096 over the next decade. Please refer to this: https://en.wikipedia.org/wiki/Disk_sector#:~:text=In%20computer%20disk%20storage%2C%20a,%2DROMs%20and%20DVD%2DROMs.
If it's 512, setting the number to 4096 will still work. It just needs to be a multiple of the disk sector size.
There has been no discussion at all in making the disk sector size even larger than 4096, so I think the choice is safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1230902313
Looks like you hit a Docker hiccup. I've rerun the failed jobs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959811087
##########
cpp/src/arrow/filesystem/localfs.cc:
##########
@@ -448,17 +448,40 @@ Result<std::shared_ptr<io::OutputStream>> OpenOutputStreamGeneric(const std::str
return maybe_stream;
}
+Result<std::shared_ptr<io::OutputStream>> OpenDirectIOOutputStreamGeneric(
+ const std::string& path) {
+ RETURN_NOT_OK(ValidatePath(path));
+ ARROW_ASSIGN_OR_RAISE(auto fn, PlatformFilename::FromString(path));
+ const bool write_only = true;
+ ARROW_ASSIGN_OR_RAISE(
+ auto fd, ::arrow::internal::FileOpenWritable(fn, write_only, true, false, true));
+ int raw_fd = fd.Detach();
+ auto maybe_stream = io::DirectFileOutputStream::Open(raw_fd);
+ if (!maybe_stream.ok()) {
+ ARROW_UNUSED(::arrow::internal::FileClose(raw_fd));
+ }
+ return maybe_stream;
+}
+
} // namespace
Result<std::shared_ptr<io::OutputStream>> LocalFileSystem::OpenOutputStream(
const std::string& path, const std::shared_ptr<const KeyValueMetadata>& metadata) {
bool truncate = true;
bool append = false;
- return OpenOutputStreamGeneric(path, truncate, append);
+ if (options_.use_directio) {
+ return OpenDirectIOOutputStreamGeneric(path);
+ } else {
+ return OpenOutputStreamGeneric(path, truncate, append);
+ }
}
Result<std::shared_ptr<io::OutputStream>> LocalFileSystem::OpenAppendStream(
const std::string& path, const std::shared_ptr<const KeyValueMetadata>& metadata) {
+ if (options_.use_directio) {
+ return Status::NotImplemented("DirectIO append stream not implemented yet.");
+ }
Review Comment:
If it were an internal optimization (e.g. something the user didn't really have direct control over but we tried to use if available) then I would agree. However, given that the user is asking for this behavior, I think an error is correct.
That being said, if it is significantly complicating tests because this doesn't exist, maybe a warning is a reasonable compromise?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r985396712
##########
cpp/src/arrow/io/file.h:
##########
@@ -80,6 +80,56 @@ class ARROW_EXPORT FileOutputStream : public OutputStream {
std::unique_ptr<FileOutputStreamImpl> impl_;
};
+#if defined(__linux__)
+/// \brief An operating system file open in write-only and O_DIRECT mode, only works on
+/// Linux.
+class ARROW_EXPORT DirectFileOutputStream : public OutputStream {
+ public:
+ ~DirectFileOutputStream() override;
+
+ /// \brief Open a local file for writing, truncating any existing file
+ /// \param[in] path with UTF8 encoding
+ ///
+ /// When opening a new file, any existing file with the indicated path is
+ /// truncated to 0 bytes, deleting any existing data
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(const std::string& path);
+
+ /// \brief Open a file descriptor for writing. The underlying file isn't
+ /// truncated.
+ /// \param[in] fd file descriptor
+ /// \param[in] sector_size the physical disk sector size hosting this fd
+ /// \return an open FileOutputStream
+ ///
+ /// The file descriptor becomes owned by the OutputStream, and will be closed
+ /// on Close() or destruction.
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(int fd,
+ int64_t sector_size);
+
+ // OutputStream interface
+ Status Close() override;
+ bool closed() const override;
+ Result<int64_t> Tell() const override;
+
+ // Write bytes to the stream. Thread-safe
+ Status Write(const void* data, int64_t nbytes) override;
+ /// \cond FALSE
+ using Writable::Write;
+ /// \endcond
+
+ int file_descriptor() const;
+
+ private:
+ explicit DirectFileOutputStream(int64_t sector_size);
+
+ class ARROW_NO_EXPORT DirectFileOutputStreamImpl;
+ std::unique_ptr<DirectFileOutputStreamImpl> impl_;
+ int64_t sector_size_ = 0;
+ std::vector<uint8_t> cached_data_;
+ uint8_t* aligned_cached_data_;
+ int64_t cached_length_ = 0;
Review Comment:
What's the problem with having these attributes here instead of inside the private impl class?
I don't think it makes sense to have them inside the private impl class, because they are used by the DirectFileOutputStream class methods not the impl_ class methods. The impl_ class methods just override basic OSFile functionality and should not have to know about cached_data etc.
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,106 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in disk-sector-size byte blocks.
+// Buffers leftover bytes in an internal data structure, which will be padded to
+// disk-sector-size bytes and written upon call to close.
+
+#if defined(__linux__)
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream(int64_t sector_size) {
+ sector_size_ = sector_size;
+ uintptr_t mask = (uintptr_t)(sector_size - 1);
Review Comment:
I can check if the sector_size is a power of two. But a constructor can't return a Result right, what's the right way to do this in Arrow? I don't think just adding this code:
```
if !(sector_size != 0 && (sector_size & (sector_size-1)) == 0)
{
return Status::IOError("Length must be non-negative");
}
```
is going to work here because return Status shouldn't be allowed in constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1550492740
welcome contribution
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960137984
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,109 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
Review Comment:
Can we use `posix_memalign`? Should be easier to read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231899886
That would only be the case if `AssertFileContents` is called *before* the file is closed, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960953133
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
Review Comment:
OK I think I figured out what to do here. Turns out the command you want is : pathconf("/dev/nvme0n1p2", _PC_REC_XFER_ALIGN)
This obviously only works on Linux, but since everything we do here only works on Linux anyways it should be fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r925059114
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
[directio.zip](https://github.com/apache/arrow/files/9145458/directio.zip)
Please disregard the previous uploaded code. @westonpace helped me get these new benchmarks. Got fadvise working. You have to use fadvise after the write sys call. Unaligned/aligned doesn't make too much of a difference for the O_DIRECT since memcpy compared to NVMe is cheap. Doing nothing (nothing.cpp) is around same speed as O_DIRECT if you call sync at the end. fadvise is marginally slower (by about 15%).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924779403
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
[directio.zip](https://github.com/apache/arrow/files/9143325/directio.zip)
Here is an experiment with three ways of writing: O_DIRECT, O_SYNC + fadvise, and normal write. I run the experiments on a 10GB write to NVME SSD on AWS i3.2xlarge. With O_DIRECT no page cache mem increase but the other two ways of writing caused 10GB page cache usage. O_SYNC + fadvise had no visible difference with normal write in terms of page cache usage. Perhaps I am not using the fadvise API properly? If so please let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231266231
> Other OutputStream implementations don't have this distinction.
Have you looked at BufferedOutputStream or CompressedOutputStream?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1233330909
Have addressed most of @pitrou comments except the hard coded 4096.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959812591
##########
cpp/src/arrow/filesystem/localfs.cc:
##########
@@ -448,17 +448,40 @@ Result<std::shared_ptr<io::OutputStream>> OpenOutputStreamGeneric(const std::str
return maybe_stream;
}
+Result<std::shared_ptr<io::OutputStream>> OpenDirectIOOutputStreamGeneric(
+ const std::string& path) {
+ RETURN_NOT_OK(ValidatePath(path));
+ ARROW_ASSIGN_OR_RAISE(auto fn, PlatformFilename::FromString(path));
+ const bool write_only = true;
+ ARROW_ASSIGN_OR_RAISE(
+ auto fd, ::arrow::internal::FileOpenWritable(fn, write_only, true, false, true));
+ int raw_fd = fd.Detach();
+ auto maybe_stream = io::DirectFileOutputStream::Open(raw_fd);
+ if (!maybe_stream.ok()) {
+ ARROW_UNUSED(::arrow::internal::FileClose(raw_fd));
+ }
+ return maybe_stream;
+}
+
} // namespace
Result<std::shared_ptr<io::OutputStream>> LocalFileSystem::OpenOutputStream(
const std::string& path, const std::shared_ptr<const KeyValueMetadata>& metadata) {
bool truncate = true;
bool append = false;
- return OpenOutputStreamGeneric(path, truncate, append);
+ if (options_.use_directio) {
+ return OpenDirectIOOutputStreamGeneric(path);
+ } else {
+ return OpenOutputStreamGeneric(path, truncate, append);
+ }
}
Result<std::shared_ptr<io::OutputStream>> LocalFileSystem::OpenAppendStream(
const std::string& path, const std::shared_ptr<const KeyValueMetadata>& metadata) {
+ if (options_.use_directio) {
+ return Status::NotImplemented("DirectIO append stream not implemented yet.");
+ }
Review Comment:
Ok, let's keep it an error then.
It should be easy to skip the tests by the way, just override `allow_append_to_file`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960985969
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,109 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
Review Comment:
There might also be a future PR in the Arrow system allocator to allocate aligned memory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959832822
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
Review Comment:
On other OS you will never get a chance to call this function since you will not be able to open the file in the first place. The ifdef here is just to help compile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1188354709
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
save-buffer commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924809581
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
Just to be clear, my experiment was with `O_DSYNC` not with `posix_fadvise`. With `O_DSYNC`, writing to a single file runs at 0.406 GB/s, while with `O_DIRECT` I get 3.50 GB/s and with no sync flag I get 4.23 GB/s. This is on a Samsung 970 Evo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r925091153
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
What about adding these benchmarks to Arrow code base?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924964325
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
Also note that any use of synchronous writes means your main thread may block when writing (normal writes are usually non-blocking). This kind of benchmark is really not testing much IMHO.
That said, we can of course offer both. My intuition is just that you may get glass jaws in real-world performance that you won't observe using a simple micro-benchmark on an otherwise local storage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mgaunard commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by "mgaunard (via GitHub)" <gi...@apache.org>.
mgaunard commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1549900476
I have taken this as a base and rewrote a better implementation of this (the original code had a number of correctness and performance issues).
I can clean it up and send it upstream if there is interest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- closed pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
URL: https://github.com/apache/arrow/pull/13640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1188354680
https://issues.apache.org/jira/browse/ARROW-17114
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924195380
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
Indeed, the `open` man page says this about `O_DIRECT`:
> Try to minimize cache effects of the I/O to and from this file. In general this will degrade performance, but it is useful in special situations, such as when applications do their own caching.
@marsupialtail Are you seeing any actual benefits from this? In which situations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1224001449
There seem to be multiple CI failures @marsupialtail . Some of them at least are related to this PR, can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231864904
> But the current tests in test_utils for filesystem assumes that Tell reflects the bytes actually written.
That's possible, can you point to a particular test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959660237
##########
cpp/src/arrow/filesystem/localfs.cc:
##########
@@ -448,17 +448,40 @@ Result<std::shared_ptr<io::OutputStream>> OpenOutputStreamGeneric(const std::str
return maybe_stream;
}
+Result<std::shared_ptr<io::OutputStream>> OpenDirectIOOutputStreamGeneric(
+ const std::string& path) {
+ RETURN_NOT_OK(ValidatePath(path));
+ ARROW_ASSIGN_OR_RAISE(auto fn, PlatformFilename::FromString(path));
+ const bool write_only = true;
+ ARROW_ASSIGN_OR_RAISE(
+ auto fd, ::arrow::internal::FileOpenWritable(fn, write_only, true, false, true));
+ int raw_fd = fd.Detach();
+ auto maybe_stream = io::DirectFileOutputStream::Open(raw_fd);
+ if (!maybe_stream.ok()) {
+ ARROW_UNUSED(::arrow::internal::FileClose(raw_fd));
+ }
+ return maybe_stream;
+}
+
} // namespace
Result<std::shared_ptr<io::OutputStream>> LocalFileSystem::OpenOutputStream(
const std::string& path, const std::shared_ptr<const KeyValueMetadata>& metadata) {
bool truncate = true;
bool append = false;
- return OpenOutputStreamGeneric(path, truncate, append);
+ if (options_.use_directio) {
+ return OpenDirectIOOutputStreamGeneric(path);
+ } else {
+ return OpenOutputStreamGeneric(path, truncate, append);
+ }
}
Result<std::shared_ptr<io::OutputStream>> LocalFileSystem::OpenAppendStream(
const std::string& path, const std::shared_ptr<const KeyValueMetadata>& metadata) {
+ if (options_.use_directio) {
+ return Status::NotImplemented("DirectIO append stream not implemented yet.");
+ }
Review Comment:
Since direct I/O is only an optimization, I think we should simply ignore it here.
##########
cpp/src/arrow/filesystem/test_util.cc:
##########
@@ -883,6 +883,8 @@ void GenericFileSystemTest::TestOpenOutputStream(FileSystem* fs) {
ASSERT_OK_AND_ASSIGN(stream, fs->OpenOutputStream("CD/ghi"));
ASSERT_OK(stream->Write("some "));
ASSERT_OK(stream->Write(Buffer::FromString("data")));
+ // this following line will cause Direct IO file system to fail
+ // since they will buffer up the nine bytes and not write them!
Review Comment:
Can you remove this comment?
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
Review Comment:
Why only Linux? Other OSes won't get a proper close?
##########
cpp/src/arrow/util/io_util.cc:
##########
@@ -1129,6 +1130,14 @@ Result<FileDescriptor> FileOpenWritable(const PlatformFilename& file_name,
oflag |= O_RDWR;
}
+ if (direct) {
+#if defined(__linux__)
+ oflag |= O_DIRECT; // will cause issues on Apple on Windows
+#else
+ return Status::IOError("Direct IO is only supported on Linux.");
Review Comment:
Please make this `Status::NotImplemented`.
##########
python/pyarrow/_fs.pyx:
##########
@@ -782,6 +782,11 @@ cdef class LocalFileSystem(FileSystem):
use_mmap : bool, default False
Whether open_input_stream and open_input_file should return
a mmap'ed file or a regular file.
+ use_directio : bool, default False
Review Comment:
Let's call this `use_direct_io`?
##########
cpp/src/arrow/io/file.h:
##########
@@ -80,6 +80,52 @@ class ARROW_EXPORT FileOutputStream : public OutputStream {
std::unique_ptr<FileOutputStreamImpl> impl_;
};
+class ARROW_EXPORT DirectFileOutputStream : public OutputStream {
Review Comment:
Please add a docstring here. The user should not have to wonder what this is.
##########
cpp/src/arrow/filesystem/test_util.h:
##########
@@ -238,9 +238,39 @@ class ARROW_TESTING_EXPORT GenericFileSystemTest {
GENERIC_FS_TEST_FUNCTION(TEST_MACRO, TEST_CLASS, OpenInputFileAsync) \
GENERIC_FS_TEST_FUNCTION(TEST_MACRO, TEST_CLASS, SpecialChars)
+#define GENERIC_FS_TEST_FUNCTIONS_WITHOUT_APPEND_MACROS(TEST_MACRO, TEST_CLASS) \
Review Comment:
Can you remove this after you've made `OpenAppendStream` fall back gracefully?
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0, 4096 - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, 4096));
+ }
+ auto new_length = impl_->Tell().ValueOrDie() - 4096 + cached_length_;
+ fsync(impl_->fd());
Review Comment:
Why `fsync` here? This will make IO more expensive, no?
##########
cpp/src/arrow/filesystem/localfs.cc:
##########
@@ -448,17 +448,40 @@ Result<std::shared_ptr<io::OutputStream>> OpenOutputStreamGeneric(const std::str
return maybe_stream;
}
+Result<std::shared_ptr<io::OutputStream>> OpenDirectIOOutputStreamGeneric(
+ const std::string& path) {
+ RETURN_NOT_OK(ValidatePath(path));
+ ARROW_ASSIGN_OR_RAISE(auto fn, PlatformFilename::FromString(path));
+ const bool write_only = true;
+ ARROW_ASSIGN_OR_RAISE(
+ auto fd, ::arrow::internal::FileOpenWritable(fn, write_only, true, false, true));
+ int raw_fd = fd.Detach();
+ auto maybe_stream = io::DirectFileOutputStream::Open(raw_fd);
+ if (!maybe_stream.ok()) {
+ ARROW_UNUSED(::arrow::internal::FileClose(raw_fd));
+ }
+ return maybe_stream;
+}
Review Comment:
Instead of copy-pasting, perhaps reconcile with `OpenOutputStreamGeneric`, one way or the other?
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0, 4096 - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, 4096));
+ }
+ auto new_length = impl_->Tell().ValueOrDie() - 4096 + cached_length_;
Review Comment:
Please don't use `ValueOrDie` and instead correctly propagate the error, for example:
```suggestion
ARROW_ASSIGN_OR_RAISE(const auto file_pos, impl_->Tell());
const auto new_length = ...;
```
##########
cpp/src/arrow/util/io_util.cc:
##########
@@ -1129,6 +1130,14 @@ Result<FileDescriptor> FileOpenWritable(const PlatformFilename& file_name,
oflag |= O_RDWR;
}
+ if (direct) {
+#if defined(__linux__)
+ oflag |= O_DIRECT; // will cause issues on Apple on Windows
Review Comment:
Did you mean "Apple and Windows" perhaps?
##########
cpp/src/arrow/io/file.h:
##########
@@ -80,6 +80,52 @@ class ARROW_EXPORT FileOutputStream : public OutputStream {
std::unique_ptr<FileOutputStreamImpl> impl_;
};
+class ARROW_EXPORT DirectFileOutputStream : public OutputStream {
+ public:
+ ~DirectFileOutputStream() override;
+
+ /// \brief Open a local file for writing, truncating any existing file
+ /// \param[in] path with UTF8 encoding
+ /// \param[in] append append to existing file, otherwise truncate to 0 bytes
+ /// \return an open FileOutputStream
+ ///
+ /// When opening a new file, any existing file with the indicated path is
+ /// truncated to 0 bytes, deleting any existing data
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(const std::string& path,
Review Comment:
I think the two factory functions `Open` should return a `NotImplementedError` if the OS is not supported.
Also, an `append` flag should probably not be accepted if it is not supported.
##########
cpp/src/arrow/filesystem/localfs.h:
##########
@@ -37,6 +37,7 @@ struct ARROW_EXPORT LocalFileSystemOptions {
/// Whether OpenInputStream and OpenInputFile return a mmap'ed file,
/// or a regular one.
bool use_mmap = false;
+ bool use_directio = false;
Review Comment:
```suggestion
/// Whether OpenOutputStream opens the file with O_DIRECT,
/// if the operating system supports it.
/// This flag can have non-trivial performance implications.
bool use_direct_io = false;
```
##########
python/pyarrow/_fs.pyx:
##########
@@ -782,6 +782,11 @@ cdef class LocalFileSystem(FileSystem):
use_mmap : bool, default False
Whether open_input_stream and open_input_file should return
a mmap'ed file or a regular file.
+ use_directio : bool, default False
+ Whether open_output_stream will return an output stream with direct io.
+ Does not support open_append_stream.
+ Will only work on Linux. Slightly different write semantics.
Review Comment:
Why are the semantics slightly different?
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
Review Comment:
Please let's not hard-code 4096, use `GetPageSize()` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960733239
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
Review Comment:
What would happen if a disk happened to have a larger sector size? Would there be a reasonable error? Would it fall back to non-direct? Or would it silently corrupt data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960971606
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0, 4096 - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, 4096));
+ }
+ auto new_length = impl_->Tell().ValueOrDie() - 4096 + cached_length_;
+ fsync(impl_->fd());
Review Comment:
Yes this is probably legacy concern. Back when I still had the file opened in O_DSYNC, not syncing at the end felt wrong. Now since data can be in limbo in SSD cache probably noone cares if it's actually synced anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924756597
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
Will produce some benchmark experiments that compare the posix_fadvise with O_DIRECT and link here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924793017
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
By the way:
> With O_DIRECT no page cache mem increase but the other two ways of writing caused 10GB page cache usage
I don't think this is conclusive. There is no reason for the OS to release some page cache is there's no memory pressure. I think a more sophisticated benchmark is needed, but I'm not sure what to suggest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r925059321
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
So I am going to close this PR and make a new one where we just use fadvise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924061697
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
Also, the biggest advantage of direct IO is to transfer data directly between the user mode buffer and disk controller. This additional buffering may render the advantage useless IIUC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1221580853
Note that the current implementation relies on a memcpy to ensure alignment. In general, if each write starts off from a potentially different buffer with an arbitrary length this memcpy is most likely required. This memcpy introduces a 2x slowdown compared to "pure" direct IO where the buffers are all aligned in the first place. It's probably too much to ask for that all Arrow buffers be 4096 aligned, so we might have to live with this memcpy unfortunately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r961197498
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,109 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
Review Comment:
Can a smart pointer do the job, if it's only to manage lifecycle?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959834366
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0, 4096 - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, 4096));
+ }
+ auto new_length = impl_->Tell().ValueOrDie() - 4096 + cached_length_;
+ fsync(impl_->fd());
Review Comment:
This is okay because this is only done at the end, and you need to sync the file when you are going to close it, particularly since I am going to fruncate if afterwards to the right length. I think it's good practice to flush all the writes before you close a file anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r959848140
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0, 4096 - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, 4096));
+ }
+ auto new_length = impl_->Tell().ValueOrDie() - 4096 + cached_length_;
+ fsync(impl_->fd());
Review Comment:
Hmm, I'm curious, why do you need it before truncating and closing? It doesn't sound right. You should let the OS decide when to commit the writes to disk (or elsewhere).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1230953003
OK seems like it passes CI now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924803445
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
[directio.zip](https://github.com/apache/arrow/files/9143508/directio.zip)
OK. I updated it to avoid overflow. The naive posix_fadvise approach (where you call this before ALL the writes) still doesn't work in controlling page cache. (fadvise.cpp)
However I discovered if you call posix_fadvise *every* time before a write on the *entire* region you intend to write, it works in controlling the page cache, despite being 50% slower than O_DIRECT. This is fadvise1.cpp. If you call posix_fadvise every time only on the next region you intend to write, it still doesn't control the page cache.
Bottom line is posix_fadvise probably works if we manage to guess at producing the right behavior, but it is a bit slower.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r961809524
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,109 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
Review Comment:
No strong feeling from me. I'm ok with `std::vector` and manual alignment given that this is quite a small memory area anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r961804596
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,109 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
Review Comment:
@pitrou what do you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924785615
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
Also, the "nothing" version is much faster here than the "direct" version (the former writes at ~300 MB/s, the latter at ~100 MB/s).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924789043
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
this is valid. Let me fix this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-17114: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924621904
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
@marsupialtail this should maybe be linked against the JIRA https://issues.apache.org/jira/browse/ARROW-14635 which explains the goals a bit better. The problem we are trying to solve here is not a performance problem. Instead, we are trying to keep large writes out of the OS cache.
An example use case is converting a large (~400GB) CSV dataset to parquet (maybe 100GB). Another example might be repartitioning a large parquet dataset. In both cases the user is unlikely to turn around and read the entire dataset and so there is no need to put the entire dataset into the OS cache, and this has detrimental effects.
What I've seen happen in these cases is that all of the data being written is stored in the page cache and writes return immediately (after the memcpy into the OS cache and before the data is persisted to disk). Writes don't actually become blocking until the OS cache fills up. The OS cache will use all available RAM and then the system grinds to a halt. Furthermore, in the face of such memory pressure, even mildly inactive processes end up swapping, which increases the disk pressure.
As I mention in the JIRA, direct I/O is only one way to solve this problem. Another way is by using sync writes and aggressively marking pages as DONTNEED. @save-buffer did an experiment a while back and found that the sync/fadvise approach was considerably slower than the direct I/O approach (though I don't recall if this experiment involved caching on the direct I/O approach or not) and so we opted to go with direct I/O.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924958407
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
I ran the experiments myself with 1GB of data for timing. Oddly, on an HHD, I didn't observe much difference. On an SSD, the difference was rather considerable (I reran the ssd tests a number of times and the difference was consistent):
Method | Disk | Time (ms)
--- | --- | ---
direct | hhd | 41290.55257
direct | ssd | 4641.38446
fadvise1 | hhd | 41179.51826
fadvise1 | ssd | 6959.85842
The direct I/O results roughly match what I get for a disk benchmark on the SSD. However, I feel the test is still cheating. The `direct.cpp` example is assuming the memory is previously aligned. Can you create a version that does not assume this (e.g. memcpy into a temp buffer before writing)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r924959447
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,77 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers leftover bytes
+// in an internal data structure, which will be padded to 4096 bytes and flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+ uint8_t *mem = static_cast<uint8_t *>(malloc(4096 + 4095));
+ cached_data = reinterpret_cast<uint8_t *>( reinterpret_cast<uintptr_t>(mem+4095) & ~(mask));
Review Comment:
That being said...I wonder how hard it would be to offer both? I suppose if one is a clear winner we shouldn't burden users with the choice but, if there are pros/cons, or different environments support different options, we should probably offer the choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r957892404
##########
cpp/src/arrow/io/file.h:
##########
@@ -80,6 +80,52 @@ class ARROW_EXPORT FileOutputStream : public OutputStream {
std::unique_ptr<FileOutputStreamImpl> impl_;
};
+class ARROW_EXPORT DirectFileOutputStream : public OutputStream {
+ public:
+ ~DirectFileOutputStream() override;
+
+ /// \brief Open a local file for writing, truncating any existing file
+ /// \param[in] path with UTF8 encoding
+ /// \param[in] append append to existing file, otherwise truncate to 0 bytes
+ /// \return an open FileOutputStream
+ ///
+ /// When opening a new file, any existing file with the indicated path is
+ /// truncated to 0 bytes, deleting any existing data
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(const std::string& path,
+ bool append = false);
+
+ /// \brief Open a file descriptor for writing. The underlying file isn't
+ /// truncated.
+ /// \param[in] fd file descriptor
+ /// \return an open FileOutputStream
+ ///
+ /// The file descriptor becomes owned by the OutputStream, and will be closed
+ /// on Close() or destruction.
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(int fd);
+
+ // OutputStream interface
+ Status Close() override;
+ bool closed() const override;
+ Result<int64_t> Tell() const override;
+
+ // Write bytes to the stream. Thread-safe
+ Status Write(const void* data, int64_t nbytes) override;
+ /// \cond FALSE
+ using Writable::Write;
+ /// \endcond
+
+ int file_descriptor() const;
+
+ private:
+ DirectFileOutputStream();
+
+ class ARROW_NO_EXPORT DirectFileOutputStreamImpl;
Review Comment:
Cool I don't know what that is but I'll resolve this comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231858497
Yeah I was wrong. The DirectOutputStream's Tell should be exactly the same as BufferedOutputStream's Tell in my opinion. But the current tests in test_utils for filesystem assumes that Tell reflects the bytes actually written.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960972464
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
Review Comment:
But this only works if you know what the path is. So I don't know how you'd be able to support and Open from just a file descriptor. I will probably have to change that API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r960969444
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
+
+ cached_data_.reserve(4096 + 4095);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + 4095) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path, bool append) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(path, append));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(int fd) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream());
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+#if defined(__linux__)
Review Comment:
Yes this will probably lead to more CI problems since I can't test ifdef on other OS locally, more annoyance but sure I agree it's probably better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r961812146
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,101 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in 4096 byte blocks. Buffers
+// leftover bytes in an internal data structure, which will be padded to 4096 bytes and
+// flushed upon call to close.
+
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path, bool append) {
+ const bool truncate = !append;
+ return OpenWritable(path, truncate, append, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream() {
+ uintptr_t mask = (uintptr_t)(4095);
Review Comment:
> But this only works if you know what the path is. So I don't know how you'd be able to support and Open from just a file descriptor. I will probably have to change that API.
Oh, then let's forget about it and keep the hard-coded 4096 (should be good enough I suppose?).
However, please make it a named constant instead of repeating the magic number everywhere, e.g.:
```c++
// The minimum write size and alighment for direct I/O.
// Unfortunately there's no easy way to query the actual number
// even at runtime (it depends on the kernel and filesystem version),
// so we hard-code a reasonable value instead.
static constexpr int64_t kDirectIoMinimumSize = 4096;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] marsupialtail commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
marsupialtail commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1231973397
Addressed the Tell issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13640:
URL: https://github.com/apache/arrow/pull/13640#discussion_r980087050
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,106 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in disk-sector-size byte blocks.
+// Buffers leftover bytes in an internal data structure, which will be padded to
+// disk-sector-size bytes and written upon call to close.
+
+#if defined(__linux__)
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream(int64_t sector_size) {
+ sector_size_ = sector_size;
+ uintptr_t mask = (uintptr_t)(sector_size - 1);
Review Comment:
Please use C++ casts, not C, so e.g. `static_cast`.
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,106 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in disk-sector-size byte blocks.
+// Buffers leftover bytes in an internal data structure, which will be padded to
+// disk-sector-size bytes and written upon call to close.
+
+#if defined(__linux__)
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream(int64_t sector_size) {
+ sector_size_ = sector_size;
+ uintptr_t mask = (uintptr_t)(sector_size - 1);
+ cached_data_.reserve(sector_size * 2 - 1);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + sector_size - 1) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(
+ new DirectFileOutputStream(pathconf(path.c_str(), _PC_REC_XFER_ALIGN)));
+ RETURN_NOT_OK(stream->impl_->Open(path));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ int fd, int64_t sector_size) {
+ auto stream =
+ std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream(sector_size));
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0,
+ sector_size_ - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, sector_size_));
+ }
+ ARROW_ASSIGN_OR_RAISE(auto file_pos, impl_->Tell());
+ auto new_length = file_pos - sector_size_ + cached_length_;
+ // fsync(impl_->fd());
+ ftruncate(impl_->fd(), new_length);
Review Comment:
Should check for errors.
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,106 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in disk-sector-size byte blocks.
+// Buffers leftover bytes in an internal data structure, which will be padded to
+// disk-sector-size bytes and written upon call to close.
+
+#if defined(__linux__)
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream(int64_t sector_size) {
+ sector_size_ = sector_size;
+ uintptr_t mask = (uintptr_t)(sector_size - 1);
Review Comment:
Also can we check that `sector_size` is a power of two?
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,106 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in disk-sector-size byte blocks.
+// Buffers leftover bytes in an internal data structure, which will be padded to
+// disk-sector-size bytes and written upon call to close.
+
+#if defined(__linux__)
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream(int64_t sector_size) {
+ sector_size_ = sector_size;
+ uintptr_t mask = (uintptr_t)(sector_size - 1);
+ cached_data_.reserve(sector_size * 2 - 1);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + sector_size - 1) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(
+ new DirectFileOutputStream(pathconf(path.c_str(), _PC_REC_XFER_ALIGN)));
+ RETURN_NOT_OK(stream->impl_->Open(path));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ int fd, int64_t sector_size) {
+ auto stream =
+ std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream(sector_size));
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0,
+ sector_size_ - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, sector_size_));
+ }
+ ARROW_ASSIGN_OR_RAISE(auto file_pos, impl_->Tell());
+ auto new_length = file_pos - sector_size_ + cached_length_;
+ // fsync(impl_->fd());
+ ftruncate(impl_->fd(), new_length);
+ }
+ cached_length_ = 0;
+ return impl_->Close();
+}
+
+bool DirectFileOutputStream::closed() const { return !impl_->is_open(); }
+
+Result<int64_t> DirectFileOutputStream::Tell() const {
+ ARROW_ASSIGN_OR_RAISE(auto tell_length, impl_->Tell());
+ return tell_length + cached_length_;
+}
+
+Status DirectFileOutputStream::Write(const void* data, int64_t length) {
+ RETURN_NOT_OK(impl_->CheckClosed());
+
+ if (cached_length_ + length < sector_size_) {
+ std::memcpy(aligned_cached_data_ + cached_length_, data, length);
+ cached_length_ += length;
+ return Status::OK();
+ }
+
+ auto bytes_to_write = (cached_length_ + length) / sector_size_ * sector_size_;
+ auto bytes_leftover = cached_length_ + length - bytes_to_write;
+ uintptr_t mask = (uintptr_t)(sector_size_ - 1);
+ std::vector<uint8_t> mem;
+ mem.reserve(bytes_to_write + sector_size_ - 1);
+ uint8_t* new_ptr = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&mem[0] + sector_size_ - 1) & ~(mask));
+ std::memcpy(new_ptr, aligned_cached_data_, cached_length_);
+ std::memcpy(new_ptr + cached_length_, data, bytes_to_write - cached_length_);
+ std::memset(aligned_cached_data_, 0, cached_length_); // this is not required.
Review Comment:
Why do it, then? :-)
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,106 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in disk-sector-size byte blocks.
+// Buffers leftover bytes in an internal data structure, which will be padded to
+// disk-sector-size bytes and written upon call to close.
+
+#if defined(__linux__)
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream(int64_t sector_size) {
+ sector_size_ = sector_size;
+ uintptr_t mask = (uintptr_t)(sector_size - 1);
+ cached_data_.reserve(sector_size * 2 - 1);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + sector_size - 1) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(
+ new DirectFileOutputStream(pathconf(path.c_str(), _PC_REC_XFER_ALIGN)));
+ RETURN_NOT_OK(stream->impl_->Open(path));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ int fd, int64_t sector_size) {
+ auto stream =
+ std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream(sector_size));
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0,
+ sector_size_ - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, sector_size_));
+ }
+ ARROW_ASSIGN_OR_RAISE(auto file_pos, impl_->Tell());
+ auto new_length = file_pos - sector_size_ + cached_length_;
+ // fsync(impl_->fd());
+ ftruncate(impl_->fd(), new_length);
+ }
+ cached_length_ = 0;
+ return impl_->Close();
+}
+
+bool DirectFileOutputStream::closed() const { return !impl_->is_open(); }
+
+Result<int64_t> DirectFileOutputStream::Tell() const {
+ ARROW_ASSIGN_OR_RAISE(auto tell_length, impl_->Tell());
+ return tell_length + cached_length_;
+}
+
+Status DirectFileOutputStream::Write(const void* data, int64_t length) {
+ RETURN_NOT_OK(impl_->CheckClosed());
+
+ if (cached_length_ + length < sector_size_) {
+ std::memcpy(aligned_cached_data_ + cached_length_, data, length);
+ cached_length_ += length;
+ return Status::OK();
+ }
+
+ auto bytes_to_write = (cached_length_ + length) / sector_size_ * sector_size_;
+ auto bytes_leftover = cached_length_ + length - bytes_to_write;
+ uintptr_t mask = (uintptr_t)(sector_size_ - 1);
Review Comment:
Please only C++ casts.
##########
cpp/src/arrow/io/file.h:
##########
@@ -80,6 +80,56 @@ class ARROW_EXPORT FileOutputStream : public OutputStream {
std::unique_ptr<FileOutputStreamImpl> impl_;
};
+#if defined(__linux__)
+/// \brief An operating system file open in write-only and O_DIRECT mode, only works on
+/// Linux.
+class ARROW_EXPORT DirectFileOutputStream : public OutputStream {
+ public:
+ ~DirectFileOutputStream() override;
+
+ /// \brief Open a local file for writing, truncating any existing file
+ /// \param[in] path with UTF8 encoding
+ ///
+ /// When opening a new file, any existing file with the indicated path is
+ /// truncated to 0 bytes, deleting any existing data
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(const std::string& path);
+
+ /// \brief Open a file descriptor for writing. The underlying file isn't
+ /// truncated.
+ /// \param[in] fd file descriptor
+ /// \param[in] sector_size the physical disk sector size hosting this fd
+ /// \return an open FileOutputStream
+ ///
+ /// The file descriptor becomes owned by the OutputStream, and will be closed
+ /// on Close() or destruction.
+ static Result<std::shared_ptr<DirectFileOutputStream>> Open(int fd,
+ int64_t sector_size);
+
+ // OutputStream interface
+ Status Close() override;
+ bool closed() const override;
+ Result<int64_t> Tell() const override;
+
+ // Write bytes to the stream. Thread-safe
+ Status Write(const void* data, int64_t nbytes) override;
+ /// \cond FALSE
+ using Writable::Write;
+ /// \endcond
+
+ int file_descriptor() const;
+
+ private:
+ explicit DirectFileOutputStream(int64_t sector_size);
+
+ class ARROW_NO_EXPORT DirectFileOutputStreamImpl;
+ std::unique_ptr<DirectFileOutputStreamImpl> impl_;
+ int64_t sector_size_ = 0;
+ std::vector<uint8_t> cached_data_;
+ uint8_t* aligned_cached_data_;
+ int64_t cached_length_ = 0;
Review Comment:
Let's put this inside the private impl class instead of here?
##########
cpp/src/arrow/io/file.cc:
##########
@@ -378,6 +378,106 @@ Status FileOutputStream::Write(const void* data, int64_t length) {
int FileOutputStream::file_descriptor() const { return impl_->fd(); }
+// ----------------------------------------------------------------------
+// DirectFileOutputStream, change the Open, Write and Close methods from FileOutputStream
+// Uses DirectIO for writes. Will only write out things in disk-sector-size byte blocks.
+// Buffers leftover bytes in an internal data structure, which will be padded to
+// disk-sector-size bytes and written upon call to close.
+
+#if defined(__linux__)
+class DirectFileOutputStream::DirectFileOutputStreamImpl : public OSFile {
+ public:
+ Status Open(const std::string& path) {
+ return OpenWritable(path, true, false, true /* write_only */, true);
+ }
+ Status Open(int fd) { return OpenWritable(fd); }
+};
+
+DirectFileOutputStream::DirectFileOutputStream(int64_t sector_size) {
+ sector_size_ = sector_size;
+ uintptr_t mask = (uintptr_t)(sector_size - 1);
+ cached_data_.reserve(sector_size * 2 - 1);
+ aligned_cached_data_ = reinterpret_cast<uint8_t*>(
+ reinterpret_cast<uintptr_t>(&cached_data_[0] + sector_size - 1) & ~(mask));
+
+ impl_.reset(new DirectFileOutputStreamImpl());
+}
+
+DirectFileOutputStream::~DirectFileOutputStream() { internal::CloseFromDestructor(this); }
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ const std::string& path) {
+ auto stream = std::shared_ptr<DirectFileOutputStream>(
+ new DirectFileOutputStream(pathconf(path.c_str(), _PC_REC_XFER_ALIGN)));
+ RETURN_NOT_OK(stream->impl_->Open(path));
+ return stream;
+}
+
+Result<std::shared_ptr<DirectFileOutputStream>> DirectFileOutputStream::Open(
+ int fd, int64_t sector_size) {
+ auto stream =
+ std::shared_ptr<DirectFileOutputStream>(new DirectFileOutputStream(sector_size));
+ RETURN_NOT_OK(stream->impl_->Open(fd));
+ return stream;
+}
+
+Status DirectFileOutputStream::Close() {
+ // some operations will call Close() on a file that is not open. In which case don't do
+ // all this.
+ if (!closed()) {
+ // have to flush out the temprorary data, but then trim the file
+ if (cached_length_ > 0) {
+ std::memset(aligned_cached_data_ + cached_length_, 0,
+ sector_size_ - cached_length_);
+ RETURN_NOT_OK(impl_->Write(aligned_cached_data_, sector_size_));
+ }
+ ARROW_ASSIGN_OR_RAISE(auto file_pos, impl_->Tell());
+ auto new_length = file_pos - sector_size_ + cached_length_;
+ // fsync(impl_->fd());
Review Comment:
Please don't leave commented out code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #13640: ARROW-14635: [Python][C++] add O_DIRECT support to writes
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #13640:
URL: https://github.com/apache/arrow/pull/13640#issuecomment-1553038523
@mgaunard yes, I would appreciate this as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org