ARROW-3170: [C++] Experimental readahead spooler #2492

pitrou · 2018-08-29T09:54:04Z

No description provided.

codecov-io · 2018-08-29T12:36:25Z

Codecov Report

Merging #2492 into master will decrease coverage by 0.06%.
The diff coverage is 96.63%.

@@            Coverage Diff             @@
##           master    #2492      +/-   ##
==========================================
- Coverage   86.28%   86.21%   -0.07%     
==========================================
  Files         308      311       +3     
  Lines       47099    47338     +239     
==========================================
+ Hits        40639    40814     +175     
- Misses       6388     6450      +62     
- Partials       72       74       +2

Impacted Files	Coverage Δ
cpp/src/arrow/io/readahead.h	`100% <100%> (ø)`
cpp/src/plasma/test/client_tests.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/test-util.h	`76.25% <100%> (-0.49%)`	⬇️
cpp/src/arrow/io/readahead.cc	`93.68% <93.68%> (ø)`
cpp/src/arrow/io/io-readahead-test.cc	`98.49% <98.49%> (ø)`
go/arrow/math/int64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/float64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/memory/memory_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/uint64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/uint64_amd64.go	`33.33% <0%> (-33.34%)`	⬇️
... and 8 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 ef21ec9...3cbaa80. Read the comment docs.

xhochy · 2018-08-29T13:09:07Z

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

+  std::shared_ptr<Buffer> buffer;
+  ABORT_NOT_OK(AllocateBuffer(data.length(), &buffer));
+  memcpy(buffer->mutable_data(), data.data(), data.length());
+  return std::make_shared<BufferReader>(std::move(buffer));


Why not return buffer? Shouldn't RVO take care of not duplicating the pointer twice?

Because I'm returning a BufferReader, not a Buffer ;-)

xhochy · 2018-08-29T13:11:55Z

cpp/src/arrow/io/readahead.h

+  ~ReadaheadSpooler();
+
+  /// Configure zero-padding at beginning and end of buffers (default 0 bytes).
+  int64_t GetLeftPadding();


Can you add more descriptive comments about what the paddings do?

xhochy

+1, LGTM

xhochy · 2018-08-29T13:34:31Z

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

+  std::shared_ptr<Buffer> buffer;
+  ABORT_NOT_OK(AllocateBuffer(data.length(), &buffer));
+  memcpy(buffer->mutable_data(), data.data(), data.length());
+  return std::make_shared<BufferReader>(std::move(buffer));


wesm · 2018-08-29T15:53:07Z

I am working on a review, probably will finish later today

wesm

This looks mostly OK to me. My main question is how we could begin to employ asynchronous IO in some of our performance critical paths, like reading Parquet files etc. I suppose after the parquet-cpp merge we can begin to explore this question more quickly

wesm · 2018-08-29T14:11:01Z

cpp/src/arrow/io/CMakeLists.txt

@@ -26,6 +26,7 @@ if (ARROW_HDFS AND NOT ARROW_BOOST_HEADER_ONLY)
 endif()

 ADD_ARROW_TEST(io-memory-test)
+ADD_ARROW_TEST(io-readahead-test)


We might consider putting all async, buffering, etc. tests in a single test module to help avoid having a too large number of test executables longer term

I'd rather have separate test modules. array-test.cc is unreadable for me.

Also this means 1) better parallelization of tests 2) when running tests for a module I don't endure the costs of testing other modules.

OK. There's other tools we can use to help with this if testing link times become an issue, like linking multiple *-test.cc files into a single test executable (I think that should work...)

wesm · 2018-08-29T14:45:38Z

cpp/src/arrow/io/readahead.h

+class Status;
+
+namespace io {
+namespace internal {


It would be useful to develop a public API for this that is useful for people that use Arrow as a library

Yes... OTOH, this is experimental and the API is likely to change depending on our internal uses. Do you think it should be made public?

For now let's develop this for our own uses and then we can always promote to public API later

wesm · 2018-08-30T04:33:32Z

cpp/src/arrow/io/CMakeLists.txt

@@ -38,4 +39,5 @@ install(FILES
  hdfs.h
  interfaces.h
  memory.h
+  readahead.h


May consider creating a module with many async interfaces, arrow/io/async.h

I'd rather not conflate terminology here. There's nothing async in this class: the Read method is blocking.

wesm · 2018-08-30T04:35:39Z

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

+  return pos;
+}
+
+static void AssertEventualPosition(std::shared_ptr<RandomAccessFile> file,


Better to get in habit of passing RandomAccessFile* unless you need the shared_ptr<T>

Ah... I thought our policy was to pass a managed pointer most of the time?
This is just a private test helper regardless :-)

Indeed, not a big deal here, but in general utility code if a function doesn't need to return ownership of shared_ptr<T> then passing T* makes it more versatile

Ok, updated.

wesm · 2018-08-30T04:36:55Z

cpp/src/arrow/io/readahead.h

+  /// \brief EXPERIMENTAL: Create a readahead spooler wrapping the given input stream.
+  ///
+  /// The spooler launches a background thread that reads up to a given number
+  /// of fixed-size blocks in advance from the underlying stream.


Could use a bit more documentation about what left padding and right padding are

wesm · 2018-08-30T04:41:01Z

cpp/src/arrow/io/readahead.h

+  int64_t GetRightPadding();
+  void SetRightPadding(int64_t size);
+
+  /// \brief Close the spooler.  This implicitly closes the underlying input stream.


You think closing the InputStream is the correct thing (curious)? If we were more consistent about using unique_ptr I would say we should pass the InputStream to the ctor as that

Well, the InputStream is left in an undefined state, since its position has advanced more than the user has read, so I don't think it's useful to keep it open.

wesm · 2018-08-30T04:42:13Z

cpp/src/arrow/io/readahead.h

+  /// Otherwise, the buffer will contain at most read_size bytes in addition
+  /// to the configured padding (short reads are possible at the end of a file).
+  // How do we allow reusing the buffer in ReadaheadBuffer? perhaps by using
+  // a caching memory pool?


This shouldn't be too complicated; we need an API to "return" a buffer to the pool so that it will be popped off a deque in the future. This would mean that we could end up with queue_size + 1 buffers but that's true anyway in general

Yes, that is the solution with an explicit ReadaheadSpooler API. The other solution would be to have a caching memory pool that puts freed buffers (up to a given number) in a freelist.

Right. We could either create a memory pool-compatible API or define some kind of explicit TemporaryBufferQueue to coordinate different kinds of consumers and producers

wesm · 2018-08-30T04:43:29Z

cpp/src/arrow/io/readahead.h

+ private:
+  static constexpr int64_t kDefaultReadSize = 1 << 20;  // 1 MB
+
+  class ARROW_NO_EXPORT Impl;


Nit: name this ReadaheadSpoolerImpl

Even for a nested class like this?

OK to leave as is

wesm · 2018-08-30T04:47:36Z

cpp/src/arrow/io/readahead.cc

+        return read_status_;
+      }
+      if (eof_) {
+        // XXX maybe we need a EOF status code


Maybe, can open an issue to consider in more detail. I think it's OK to return null for now

Fair enough, I'll remove the comment.

wesm · 2018-08-30T04:50:47Z

cpp/src/arrow/io/readahead.cc

+
+    while (true) {
+      if (please_close_) {
+        goto eof;


Haven't seen goto in this codebase yet, but if it's the best way to write this code it is ok with me

It's useful for cleanup code at the end of a function like this. I know C++ uses RAII when possible but it would be unwieldy here to define a local class just for this.

wesm · 2018-09-04T21:19:54Z

I opened ARROW-3170 as this is not exactly what is scoped there

wesm · 2018-09-07T04:06:42Z

Anything more you wanted to do on this? Can merge otherwise

pitrou force-pushed the ARROW-501-readahead branch 3 times, most recently from 2f11399 to 3cbaa80 Compare August 29, 2018 11:48

xhochy reviewed Aug 29, 2018

View reviewed changes

xhochy approved these changes Aug 29, 2018

View reviewed changes

pitrou requested a review from wesm August 29, 2018 15:37

wesm reviewed Aug 30, 2018

View reviewed changes

pitrou force-pushed the ARROW-501-readahead branch from f38562c to 6f72e54 Compare September 3, 2018 18:51

wesm changed the title ~~ARROW-501: [C++] Experimental readahead spooler~~ ARROW-3170: [C++] Experimental readahead spooler Sep 4, 2018

pitrou added 2 commits September 10, 2018 17:52

ARROW-501: [C++] Experimental readahead spooler

4f8f70f

Improve paddings description

cbbd3db

pitrou force-pushed the ARROW-501-readahead branch from 6f72e54 to cbbd3db Compare September 10, 2018 16:42

pitrou closed this in 9f9563f Sep 10, 2018

pitrou deleted the ARROW-501-readahead branch March 2, 2021 16:55

asfimport mentioned this pull request Aug 27, 2022

[C++] Implement concurrent / buffering InputStream for streaming data use cases #16143

Closed

ARROW-3170: [C++] Experimental readahead spooler #2492

ARROW-3170: [C++] Experimental readahead spooler #2492

Conversation

pitrou commented Aug 29, 2018

codecov-io commented Aug 29, 2018

Codecov Report

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

xhochy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesm commented Aug 29, 2018

wesm 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

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

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

wesm commented Sep 4, 2018

wesm commented Sep 7, 2018