ARROW-6141: [C++] Enable memory-mapping a file region #5101

zhouyuan · 2019-08-16T07:38:44Z

This patch adds an Open() method for MemoryMappedFile() with
length and offset params. In this way user can memory map a file
region just like mmap(). The new API is:

MemoryMappedFile::Open(path, mode, length, offset, &mmap)

The original API is still available. Calling the original API
will memory map the whole file:

MemoryMappedFile::Open(path, mode, &mmap)

A new field map_len_ is added in MemoryMappedFile::MemoryMap to
track the real memory map length.

Also MemoryMappedFile::Read()/ReadAt()/Write()/WriteAt() are changed
to check the memory map length if it's a region based memory map.

Note the MemoryMappedFile::Resize() is not supported if it's a
region based memory map.

Signed-off-by: Yuan Zhou yuan.zhou@intel.com

fsaintjacques

Remove the const for all off_t type.

fsaintjacques · 2019-08-16T16:24:24Z

cpp/src/arrow/io/file.h

@@ -183,10 +183,12 @@ class ARROW_EXPORT MemoryMappedFile : public ReadWriteFileInterface {

  /// Create new file with indicated size, return in read/write mode
  static Status Create(const std::string& path, int64_t size,
-                       std::shared_ptr<MemoryMappedFile>* out);
+                       std::shared_ptr<MemoryMappedFile>* out,


Store the offset in a property for debugging/reference purpose.

fsaintjacques · 2019-08-16T16:25:16Z

cpp/src/arrow/io/file.cc

@@ -412,7 +412,7 @@ class MemoryMappedFile::MemoryMap : public MutableBuffer {

  bool closed() const { return !file_->is_open(); }

-  Status Open(const std::string& path, FileMode::type mode) {
+  Status Open(const std::string& path, FileMode::type mode, const off_t offset) {


I'd add a check that the offset is a multiple of PAGE_SIZE.

Will add this logic. Thanks!

bkietz · 2019-08-16T16:45:18Z

cpp/src/arrow/io/test-common.cc

-                                       std::shared_ptr<MemoryMappedFile>* mmap) {
-  RETURN_NOT_OK(MemoryMappedFile::Create(path, size, mmap));
+                                       std::shared_ptr<MemoryMappedFile>* mmap,
+				       const off_t offset) {


I think you need to run clang-format

Suggested change

const off_t offset) {

const off_t offset) {

bkietz · 2019-08-16T16:47:50Z

cpp/src/arrow/io/test-common.h

@@ -47,7 +47,8 @@ class ARROW_EXPORT MemoryMapFixture {
  void CreateFile(const std::string& path, int64_t size);

  Status InitMemoryMap(int64_t size, const std::string& path,
-                       std::shared_ptr<MemoryMappedFile>* mmap);
+                       std::shared_ptr<MemoryMappedFile>* mmap,
+		       const off_t offset = 0);


Code style quibble: adding an optional parameter after the output parameter, which should be last. Could you add this as an overload (for example Status InitMemoryMap(int64_t offset, int64_t size, const std::string& path, std::shared_ptr<MemoryMappedFile>* mmap))?

Agree, adding a overload seems more clean. Will add this in the next push. Thanks!

wesm · 2019-08-19T15:29:30Z

cpp/src/arrow/io/file.h


  static Status Open(const std::string& path, FileMode::type mode,
-                     std::shared_ptr<MemoryMappedFile>* out);
+                     std::shared_ptr<MemoryMappedFile>* out,
+		     const off_t offset = 0);


I think it's better to use int64_t for the off_t is a platform-specific type.

I also agree with Ben about adding an overload Open(path, mode, offset, &mmap)

Hi @wesm we may need to add another param length here, with this user could memory map a portion of the file just like the semantics of mmap().

MemoryMappedFile::Open(path, mode, &mmap)

MemoryMappedFile::Open(path, mode, length, offset, &mmap)

In the 2nd case, since only part of the file is mmaped, I think we may need to make MemoryMappedFile::GetSize() return length instead of the size_ of the corresponding file.
Does this looks like the right approach to you?

wesm · 2019-08-19T15:30:35Z

cpp/src/arrow/io/file.h

@@ -183,10 +183,12 @@ class ARROW_EXPORT MemoryMappedFile : public ReadWriteFileInterface {

  /// Create new file with indicated size, return in read/write mode
  static Status Create(const std::string& path, int64_t size,
-                       std::shared_ptr<MemoryMappedFile>* out);
+                       std::shared_ptr<MemoryMappedFile>* out,
+		       const off_t offset = 0);


Adding offset to this function is a little bit odd. Is there an immediate use case for this (I would think that mapping a portion of a pre-existing file would be the main thing)

cpp/src/arrow/io/file.cc

pitrou · 2019-08-27T15:45:48Z

@zhouyuan Is there a reason this PR allows passing an offset and not a length? Intuitively, if you want to map a file region, you should be able to pass both.

zhouyuan · 2019-08-28T00:05:20Z

@zhouyuan Is there a reason this PR allows passing an offset and not a length? Intuitively, if you want to map a file region, you should be able to pass both.

@pitrou thanks for the look, yes I was trying to add the missing the length param. #5101 (comment)
This would allow us to have the same semantics of mmap(). There're also some changes needed on some APIs like Resize() - but I'm not sure if this is a correct approach. I'll push the code out for review first

pitrou

Thanks for the update. Still a couple comments.

cpp/src/arrow/io/file.cc

pitrou · 2019-08-28T11:34:24Z

cpp/src/arrow/io/file.cc

-                        map_mode_, file_->fd(), 0);
+
+    size_t mmap_length = static_cast<size_t>(initial_size);
+    if (length > 0 && length < initial_size) {


If length has an invalid value (e.g. greater than the file size), we should return an error instead of silently creating a smaller map, IMHO.

pitrou · 2019-08-28T11:35:31Z

cpp/src/arrow/io/file.cc

@@ -484,6 +491,8 @@ class MemoryMappedFile::MemoryMap : public MutableBuffer {

  int64_t size() const { return size_; }


Is it still useful to expose this? Perhaps size() should simply return map_len_?

@pitrou thanks for the careful review, I'm uncertain on this - is it possible some workload only memory map a file region, but still want to check the whole file size?

I don't know. But GetSize should return the amount of data that's readable through the file region, not the entire size.

I see, will make GetSize() return the memory map data length and change the corresponding tests.

pitrou · 2019-08-28T11:40:06Z

cpp/src/arrow/io/file.h

  static Status Open(const std::string& path, FileMode::type mode,
                     std::shared_ptr<MemoryMappedFile>* out);

+  // mmap() with a region of file, the offset must be a multiple of the page size
+  static Status Open(const std::string& path, FileMode::type mode, const int64_t length,
+                     const int64_t offset, std::shared_ptr<MemoryMappedFile>* out);


I would favor (offset, length) rather than (length, offset) in the signature.

pitrou · 2019-08-28T11:40:42Z

cpp/src/arrow/io/file_test.cc

+  ASSERT_RAISES(IOError, result->Resize(4096));
+
+  // Write beyond memory mapped length
+  ASSERT_RAISES(Invalid, result->WriteAt(4096, buffer.data(), buffer_size));


Should you check GetSize() and Seek() as well?

Tests on GetSize(), Seek(), Tell() added.

pitrou · 2019-08-29T11:03:48Z

Thanks for the changes @zhouyuan . The GetSize semantics still need to be changed, then I think it will be good.

This patch adds an Open() method for MemoryMappedFile() with length and offset params. In this way user can memory map a file region just like mmap(). The new API is: * MemoryMappedFile::Open(path, mode, length, offset, &mmap) The original API is still available. Calling the original API will memory map the whole file: * MemoryMappedFile::Open(path, mode, &mmap) A new field map_len_ is added in MemoryMappedFile::MemoryMap to track the real memory map length. Also MemoryMappedFile::Read()/ReadAt()/Write()/WriteAt() are changed to check the memory map length if it's a region based memory map. Note the MemoryMappedFile::Resize() is not supported if it's a region based memory map. Signey-off-by: Yuan Zhou <yuan.zhou@intel.com> Signed-off-by: Yuan Zhou <yuan.zhou@intel.com>

codecov-io · 2019-08-30T04:59:18Z

Codecov Report

Merging #5101 into master will increase coverage by 1.56%.
The diff coverage is 98.14%.

@@            Coverage Diff             @@
##           master    #5101      +/-   ##
==========================================
+ Coverage   87.64%   89.21%   +1.56%     
==========================================
  Files        1030      747     -283     
  Lines      148327   107494   -40833     
  Branches     1437        0    -1437     
==========================================
- Hits       129995    95896   -34099     
+ Misses      17970    11598    -6372     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/arrow/io/file_test.cc	`95.45% <100%> (+0.2%)`	⬆️
cpp/src/arrow/io/file.cc	`97.68% <96.15%> (+0.12%)`	⬆️
cpp/src/arrow/filesystem/s3_internal.h	`95.12% <0%> (-4.88%)`	⬇️
python/pyarrow/tests/test_parquet.py	`96.5% <0%> (-0.13%)`	⬇️
js/src/util/fn.ts
go/arrow/memory/memory_avx2_amd64.go
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
rust/datafusion/src/bin/main.rs
go/arrow/ipc/file_reader.go
... and 278 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcf5897...030994b. Read the comment docs.

pitrou

+1, than you @zhouyuan !

zhouyuan force-pushed the wip_6141 branch 2 times, most recently from 52ac371 to 8069940 Compare August 16, 2019 08:16

fsaintjacques changed the title ~~RROW-6141: [C++] Enable memory-mapping a file region~~ ARROW-6141: [C++] Enable memory-mapping a file region Aug 16, 2019

fsaintjacques requested changes Aug 16, 2019

View reviewed changes

bkietz requested changes Aug 16, 2019

View reviewed changes

wesm reviewed Aug 19, 2019

View reviewed changes

zhouyuan force-pushed the wip_6141 branch 2 times, most recently from 282cb95 to f676167 Compare August 20, 2019 13:52

fsaintjacques reviewed Aug 20, 2019

View reviewed changes

cpp/src/arrow/io/file.cc Outdated Show resolved Hide resolved

zhouyuan force-pushed the wip_6141 branch 4 times, most recently from f57d6c4 to 2e15766 Compare August 26, 2019 06:23

zhouyuan force-pushed the wip_6141 branch 2 times, most recently from 81f9246 to 4dc5d13 Compare August 28, 2019 07:45

pitrou reviewed Aug 28, 2019

View reviewed changes

zhouyuan force-pushed the wip_6141 branch from 4dc5d13 to fab91ff Compare August 29, 2019 06:39

zhouyuan force-pushed the wip_6141 branch from fab91ff to 030994b Compare August 30, 2019 00:54

pitrou approved these changes Sep 2, 2019

View reviewed changes

pitrou closed this in 1c42e6f Sep 2, 2019

asfimport mentioned this pull request Sep 2, 2019

[C++] Enable memory-mapping a file region that is offset from the beginning of the file #22535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-6141: [C++] Enable memory-mapping a file region #5101

ARROW-6141: [C++] Enable memory-mapping a file region #5101

zhouyuan commented Aug 16, 2019 •

edited

fsaintjacques left a comment

fsaintjacques Aug 16, 2019

fsaintjacques Aug 16, 2019

zhouyuan Aug 19, 2019

bkietz Aug 16, 2019

bkietz Aug 16, 2019

zhouyuan Aug 19, 2019

wesm Aug 19, 2019

zhouyuan Aug 27, 2019

wesm Aug 19, 2019

pitrou commented Aug 27, 2019

zhouyuan commented Aug 28, 2019 •

edited

pitrou left a comment

pitrou Aug 28, 2019

zhouyuan Aug 29, 2019

pitrou Aug 28, 2019

zhouyuan Aug 29, 2019

pitrou Aug 29, 2019

zhouyuan Aug 29, 2019

pitrou Aug 28, 2019

zhouyuan Aug 29, 2019

pitrou Aug 28, 2019

zhouyuan Aug 29, 2019

pitrou commented Aug 29, 2019

codecov-io commented Aug 30, 2019

pitrou left a comment

		@@ -484,6 +491,8 @@ class MemoryMappedFile::MemoryMap : public MutableBuffer {

		int64_t size() const { return size_; }

ARROW-6141: [C++] Enable memory-mapping a file region #5101

ARROW-6141: [C++] Enable memory-mapping a file region #5101

Conversation

zhouyuan commented Aug 16, 2019 • edited

fsaintjacques left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Aug 27, 2019

zhouyuan commented Aug 28, 2019 • edited

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Aug 29, 2019

codecov-io commented Aug 30, 2019

Codecov Report

pitrou left a comment

Choose a reason for hiding this comment

zhouyuan commented Aug 16, 2019 •

edited

zhouyuan commented Aug 28, 2019 •

edited