Add SsdCache to CachedBufferedInput #911

oerling · 2022-01-21T05:46:11Z

Adds wiring to CacheInputStream and CachedBufferedInput for reading
from SsdCache where possible. Adds accounting of file and file group
level access to ScanTracker/FileGroupStats. FileGroupStats is a
placeholder for logic for deciding which data to stage on SSD.

CachedBufferedInput divides enqueued regions between RAM, SSD and
storage. Such a division is made for different access frequency
buckets. Columns that are accessed at the same frequency are assumed
to be correlated. This is a reasonable assumption in uses that do a
conjunction of column filters. Loads are coalesced within these
buckets. Loads of the most densely accessed bucket are scheduled on an
executor for prefetching if the executor is supplied. Other loads are
made on first use, combining all nearby loads in one IO.

When accessing a single addressable unit, e.g. values or nulls of a
column, we look up the entry in the RAM cache, then in the SSD cache
and finally load from storage. An entry can be cached with a different
length than requested if we have loads using a different load
quantum. If the entry that is found in a cache is shorter than the
requested size, the entry is ignored and will be replaced by a fresh
longer entry. If loading an entry rom SSD fails, we log the error and
proceed to load the data from storage, We remove said entry from SSD
so as not to repeatedly hit the same error.

mbasmanova · 2022-01-21T14:29:19Z

@oerling There are test failures in linux-parquet-s3-build and build failures in macos-build.

99% tests passed, 1 tests failed out of 121

Total Test time (real) = 1500.11 sec

The following tests FAILED:
	  3 - velox_cache_test (Timeout)

FAILED: velox/common/caching/CMakeFiles/velox_caching.dir/AsyncDataCache.cpp.o 
ccache /Applications/Xcode-11.7.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_CONTEXT_DYN_LINK -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_REGEX_DYN_LINK -DBOOST_SYSTEM_DYN_LINK -DBOOST_THREAD_DYN_LINK -DCODEGEN_ENABLED=1 -DFMT_LOCALE -DGFLAGS_IS_A_DLL=0 -I/usr/local/opt/icu4c/include -I/Users/distiller/project/. -isystem /Users/distiller/deps/include -isystem /Users/distiller/project/velox -isystem /Users/distiller/project/velox/external -isystem /Users/distiller/project/velox/external/duckdb -isystem /Users/distiller/deps/opt/openssl@1.1/include -mavx2 -mfma -mavx -mf16c -mbmi2 -march=native -D USE_VELOX_COMMON_BASE -D HAS_UNCAUGHT_EXCEPTIONS -Werror -Wall -Wextra -Wno-unused        -Wno-unused-parameter        -Wno-sign-compare        -Wno-ignored-qualifiers        -Wno-range-loop-analysis -Wno-mismatched-tags -g -isysroot /Applications/Xcode-11.7.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -std=gnu++17 -MD -MT velox/common/caching/CMakeFiles/velox_caching.dir/AsyncDataCache.cpp.o -MF velox/common/caching/CMakeFiles/velox_caching.dir/AsyncDataCache.cpp.o.d -o velox/common/caching/CMakeFiles/velox_caching.dir/AsyncDataCache.cpp.o -c /Users/distiller/project/velox/common/caching/AsyncDataCache.cpp
In file included from /Users/distiller/project/velox/common/caching/AsyncDataCache.cpp:17:
In file included from /Users/distiller/project/./velox/common/caching/AsyncDataCache.h:26:
In file included from /Users/distiller/project/./velox/common/caching/FileGroupStats.h:19:
/Users/distiller/project/./velox/common/caching/ScanTracker.h:128:37: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
      std::function<void(ScanTracker*)> unregisterer,
                                    ^
/Users/distiller/project/./velox/common/caching/ScanTracker.h:128:37: note: insert '_Nullable' if the pointer may be null
      std::function<void(ScanTracker*)> unregisterer,
                                    ^
                                     _Nullable
/

facebook-github-bot · 2022-01-21T15:40:23Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-01-22T00:15:25Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-01-23T01:51:08Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-01-23T03:21:59Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-01-24T17:21:30Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

oerling · 2022-01-24T18:28:55Z

Masha, this should now build on Mac and fbcode with no warnings.

mbasmanova · 2022-01-24T18:31:49Z

Masha, this should now build on Mac and fbcode with no warnings.

Thank you, Orri. Will take a look later today or tomorrow.

mbasmanova

@oerling I find these changes hard to read. Some small comments below.

mbasmanova · 2022-01-24T19:37:11Z

velox/common/caching/ScanTracker.h

  folly::F14FastMap<TrackingId, TrackingData> data_;
  TrackingData sum_;
+  // Maximum size of a read. A to 10MB would count as two references


typos: "A to 10MB"

mbasmanova · 2022-01-24T19:37:44Z

velox/common/caching/ScanTracker.h

  folly::F14FastMap<TrackingId, TrackingData> data_;
  TrackingData sum_;
+  // Maximum size of a read. A to 10MB would count as two references
+  // if the quantim were 8MB. At the same time this would count as a
+  // single 10MB reference for 'fileGroupTracker_'. 0 means the read


typo: fileGroupTracker_ -> fileGroupStats_ ?

mbasmanova · 2022-01-24T19:39:15Z

velox/dwio/dwrf/common/BufferedInput.h

    return std::make_unique<BufferedInput>(input, pool, dataCacheConfig);
  }

-  static BufferedInputFactory* baseFactory();
+  virtual folly::Executor* FOLLY_NULLABLE executor() const {


Would you document executor(), copy() and baseFactory() methods?

mbasmanova · 2022-01-24T19:40:59Z

velox/dwio/dwrf/common/CacheInputStream.cpp

+        if (!ssdPin.empty() && ssdPin.run().size() < entry->size()) {
+          LOG(INFO) << fmt::format(
+              "IOERR: Ssd entry for {}:{} {} b shorter than requested {}b",
+              entry->key().fileNum.id(),


Would it be useful to add entry->toString() method?

mbasmanova · 2022-01-24T19:42:12Z

velox/dwio/dwrf/common/CacheInputStream.cpp

+            file.find(cache::RawFileCacheKey{fileNum_, region.offset});
+        if (!ssdPin.empty() && ssdPin.run().size() < entry->size()) {
+          LOG(INFO) << fmt::format(
+              "IOERR: Ssd entry for {}:{} {} b shorter than requested {}b",


This message seems off. It says "entry N b shorter than requested Mb". I assume "b" is for bytes. It reads as entry is N bytes shorter than M, but N is not a difference in bytes, rather is it the size of the entry. It would be nice to reword this message for clarity.

Also, why is this an OK condition? Why is logging enough?

mbasmanova · 2022-01-24T19:43:14Z

velox/dwio/dwrf/common/CacheInputStream.cpp

+              entry->size());
+          ssdPin.clear();
+        }
+        if (!ssdPin.empty()) {


This method is very long. It would be nice to refactor the logic inside this "if" statement into a helper method.

mbasmanova · 2022-01-24T19:44:23Z

velox/dwio/dwrf/common/CacheInputStream.cpp

+                         region.offset - region_.offset,
+                         fileNum_,
+                         fileIds().string(fileNum_));
+              // Remove the non-loadable entry so that next access goes to


Why is it Ok to ignore this error? It would be nice to describe the error handing strategy in the PR description.

mbasmanova · 2022-01-24T19:45:36Z

velox/dwio/dwrf/common/CachedBufferedInput.cpp

@@ -23,13 +23,19 @@ DEFINE_int32(
    80,
    "Minimum percentage of actual uses over references to a column for prefetching. No prefetch if > 100");

+DEFINE_int32(
+    ssd_max_coalesce_distance,
+    (50 << 10),


Is this 50KB? perhaps, add a comment for clarity.

mbasmanova · 2022-01-24T19:46:27Z

velox/dwio/dwrf/common/CachedBufferedInput.cpp

-      for (auto part : parts) {
-        if (cache_->exists(part->key)) {
-          continue;
+  for (auto readPct : std::vector<int32_t>{80, 50, 20, 0}) {


It would be nice to document the constants used here.

oerling · 2022-01-25T06:14:28Z

Thank you, Masha. I made the requested edits.

mbasmanova

@oerling Orri, thank you for update the PR description to provide more details. These are very helpful and will be useful in the future as well when folks look up file history in Git. I edited the description to fix a few typos.

The PR is good to go except there is an accidental change to build/deps/github_hashes/facebook/folly-rev.txt file. Please, revert.

Adds wiring to CacheInputStream and CachedBufferedInput for reading from SsdCache where possible. Adds accounting of file and file group level access to ScanTracker/FileGroupStats. FileGroupStats is is a placeholder for logic for deciding which data to stage on SSD. CachedBufferedInput divides enqueued regions between RAM, SSD and storage. Such a division is made for different access frequency buckets. Columns that are accessed at the same frequency are assumed to be correlated. This is a reasonable assumption in uses that do a conjunction of column filters. Loads are coalesced within these buckets. Loads of the most densely accessed bucket are scheduled on an executor for prefetching if the executor is supplied. Other loads are made on first use, combining all nearby loads in one IO. When accessing a single addressable unit, e.g. values or nulls of a column, we look up the entry n the RAM cache, then in the SSD cache and finally load from storage. An entry can be cached with a different length than requested if we have loadss using a different load quantum. If the entry that is found in a cache is shorter than the requested size, the entry is ignored and will be replaced by a fresh longer entry. If loading an entry rom SSD fails, we log the error and proceed to load the data from storage, We remove said entry from SSD so as not to repeatedly hit the same error.

oerling · 2022-01-25T16:50:52Z

Thank you, Masha. I reexported it based on the new head. Git show does not show the mentioned accidental update, so this should be OK far as I can tell. ,

facebook-github-bot · 2022-01-25T16:52:05Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-01-25T17:00:20Z

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: X-link: rsocket/rsocket-cpp#911 X-link: facebookexperimental/rust-shed#32 X-link: facebookincubator/reindeer#3 X-link: https://github.com/fairinternal/AIRStore/pull/36 Pull Request resolved: #1197 X-link: facebook/mvfst#242 X-link: facebook/sapling#116 X-link: facebookincubator/fizz#75 X-link: facebookincubator/katran#157 X-link: facebook/watchman#1011 X-link: facebook/wangle#205 X-link: facebook/proxygen#401 X-link: facebook/openr#129 X-link: facebookarchive/fbzmq#35 X-link: facebook/fb303#26 X-link: facebookarchive/bistro#59 X-link: facebook/folly#1734 X-link: facebook/fboss#113 Adds an environment variable to getdeps to provide `hg` info to avoid calling `hg` directly. When using `getdeps` inside a containerized environment (which we need to build Research Super Cluster tooling with the correct linker attributes), `getdeps` fails because of unregistered mercurial extensions in the `hgrc`. This allows `getdeps` to be useable in an environment where the mercurial extensions used in a project aren't installed/available. Reviewed By: vivekspai Differential Revision: D34732506 fbshipit-source-id: 6475088fbf3323347095ca5af8a902382cf8f9d0

Summary: X-link: rsocket/rsocket-cpp#911 X-link: facebookexperimental/rust-shed#32 X-link: facebookincubator/reindeer#3 X-link: https://github.com/fairinternal/AIRStore/pull/36 Pull Request resolved: facebookincubator#1197 X-link: facebook/mvfst#242 X-link: facebook/sapling#116 X-link: facebookincubator/fizz#75 X-link: facebookincubator/katran#157 X-link: facebook/watchman#1011 X-link: facebook/wangle#205 X-link: facebook/proxygen#401 X-link: facebook/openr#129 X-link: facebookarchive/fbzmq#35 X-link: facebook/fb303#26 X-link: facebookarchive/bistro#59 X-link: facebook/folly#1734 X-link: facebook/fboss#113 Adds an environment variable to getdeps to provide `hg` info to avoid calling `hg` directly. When using `getdeps` inside a containerized environment (which we need to build Research Super Cluster tooling with the correct linker attributes), `getdeps` fails because of unregistered mercurial extensions in the `hgrc`. This allows `getdeps` to be useable in an environment where the mercurial extensions used in a project aren't installed/available. Reviewed By: vivekspai Differential Revision: D34732506 fbshipit-source-id: 6475088fbf3323347095ca5af8a902382cf8f9d0

oerling requested a review from mbasmanova January 21, 2022 05:46

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 21, 2022

mbasmanova requested a review from majetideepak January 21, 2022 14:27

oerling force-pushed the ssd-input-pr branch from 3fceb00 to 9ea1ff7 Compare January 21, 2022 17:44

oerling force-pushed the ssd-input-pr branch from 9ea1ff7 to 288f639 Compare January 23, 2022 01:48

oerling force-pushed the ssd-input-pr branch from 288f639 to 41a1e31 Compare January 23, 2022 03:21

oerling force-pushed the ssd-input-pr branch 7 times, most recently from 8093aee to a7b3ecc Compare January 24, 2022 17:20

oerling force-pushed the ssd-input-pr branch from a7b3ecc to d3e71a8 Compare January 24, 2022 18:16

mbasmanova reviewed Jan 24, 2022

View reviewed changes

oerling force-pushed the ssd-input-pr branch from d3e71a8 to 5261cbf Compare January 25, 2022 06:11

mbasmanova requested a review from spershin January 25, 2022 15:09

mbasmanova requested changes Jan 25, 2022

View reviewed changes

oerling force-pushed the ssd-input-pr branch from 5261cbf to 031b18e Compare January 25, 2022 16:48

mbasmanova approved these changes Jan 25, 2022

View reviewed changes

facebook-github-bot closed this in 2b90ad2 Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SsdCache to CachedBufferedInput #911

Add SsdCache to CachedBufferedInput #911

oerling commented Jan 21, 2022 •

edited by mbasmanova

Loading

mbasmanova commented Jan 21, 2022

facebook-github-bot commented Jan 21, 2022

facebook-github-bot commented Jan 22, 2022

facebook-github-bot commented Jan 23, 2022

facebook-github-bot commented Jan 23, 2022

facebook-github-bot commented Jan 24, 2022

oerling commented Jan 24, 2022 via email

mbasmanova commented Jan 24, 2022

mbasmanova left a comment

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

mbasmanova Jan 24, 2022

oerling commented Jan 25, 2022 via email

mbasmanova left a comment

oerling commented Jan 25, 2022 via email

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 25, 2022

Add SsdCache to CachedBufferedInput #911

Add SsdCache to CachedBufferedInput #911

Conversation

oerling commented Jan 21, 2022 • edited by mbasmanova Loading

mbasmanova commented Jan 21, 2022

facebook-github-bot commented Jan 21, 2022

facebook-github-bot commented Jan 22, 2022

facebook-github-bot commented Jan 23, 2022

facebook-github-bot commented Jan 23, 2022

facebook-github-bot commented Jan 24, 2022

oerling commented Jan 24, 2022 via email

mbasmanova commented Jan 24, 2022

mbasmanova 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

oerling commented Jan 25, 2022 via email

mbasmanova left a comment

Choose a reason for hiding this comment

oerling commented Jan 25, 2022 via email

facebook-github-bot commented Jan 25, 2022

facebook-github-bot commented Jan 25, 2022

oerling commented Jan 21, 2022 •

edited by mbasmanova

Loading