[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/19475 )
Change subject: IMPALA-11886: Data cache should support asynchronous writes ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/19475/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19475/4//COMMIT_MSG@24 PS4, Line 24: Testing: > I'm not convinced that this change will improve performance noticeably. Whe Thank you for your question, I have done some simple performance tests for this. Firstly, I modified ThreadFn function in data-cache-test.cc and added a timer for writing, like this:X Then I calculated the average writing time for each test case. Some of the tests had similar results between synchronous and asynchronous; while there were obvious differences in some cases as shown in the following table: Test case | Policy | Sync/Async | write time(ms) MultiThreadedNoMisses | LRU | Sync | 12.20 MultiThreadedNoMisses | LRU | Async | 20.74 MultiThreadedNoMisses | LIRS | Sync | 9.42 MultiThreadedNoMisses | LIRS | Async | 16.75 MultiThreadedWithMisses | LRU | Sync | 510.87 MultiThreadedWithMisses | LRU | Async | 10.06 MultiThreadedWithMisses | LIRS | Sync | 1872.11 MultiThreadedWithMisses | LIRS | Async | 11.02 MultiPartitions | LRU | Sync | 1.20 MultiPartitions | LRU | Async | 5.23 MultiPartitions | LIRS | Sync | 1.26 MultiPartitions | LIRS | Async | 7.91 AccessTraceAnonymization | LRU | Sync | 1963.89 AccessTraceAnonymization | LRU | Sync | 2073.62 AccessTraceAnonymization | LRU | Async | 9.43 AccessTraceAnonymization | LRU | Async | 13.13 AccessTraceAnonymization | LIRS | Sync | 1663.93 AccessTraceAnonymization | LIRS | Sync | 1501.86 AccessTraceAnonymization | LIRS | Async | 12.83 AccessTraceAnonymization | LIRS | Async | 12.74 I also conducted some practical application tests on the HDFS data of the production environment. I selected some queries that were bottlenecks for HDFS scans and executed them several times on the newly started cluster, and recorded the time consumption (in seconds) for the HDFS scans. The results are as follows: Test case | M/H | No DataCache | Sync DataCache | Async DataCache case1 | MISS | 29.06 | 89.00 | 34.15 | HIT | 25.23 | 23.13 | | HIT | 30.49 | 22.75 | case2 | MISS | 6.92 | 9.63 | 6.33 | HIT | 5.19 | 4.04 | | HIT | 6.66 | 3.40 | case3 | MISS | 11.96 | 36.73 | 17.41 | HIT | 7.94 | 5.75 | | HIT | 15.82 | 5.73 | >From the test result, I think asynchronous writing can still improve the >performance of scanning when cache miss, especially when there is cache entry >eviction. http://gerrit.cloudera.org:8080/#/c/19475/4/be/src/runtime/io/data-cache.h File be/src/runtime/io/data-cache.h: http://gerrit.cloudera.org:8080/#/c/19475/4/be/src/runtime/io/data-cache.h@271 PS4, Line 271: uint8_t* buffer_; > Yes, this should be unique_ptr<uint8_t[]>. It will have a similar amount of Done http://gerrit.cloudera.org:8080/#/c/19475/4/be/src/runtime/io/data-cache.h@203 PS4, Line 203: : /// The key used for look up in the cache. : struct CacheKey { : public: : explicit CacheKey(const string& filename, int64_t mtime, int64_t offset) : : key_(filename.size() + sizeof(mtime) + sizeof(offset)) { : DCHECK_GE(mtime, 0); : DCHECK_GE(offset, 0); : key_.append(&mtime, sizeof(mtime)); : key_.append(&offset, sizeof(offset)); : key_.append(filename); : } : : int64_t Hash() const { : return HashUtil::FastHash64(key_.data(), key_.size(), 0); : } : : Slice filename() const { : return Slice(key_.data() + OFFSETOF_FILENAME, key_.size() - OFFSETOF_FILENAME); : } : : int64_t mtime() const { : return UNALIGNED_LOAD64(key_.data() + OFFSETOF_MTIME); : } : : int64_t offset() const { : return UNALIGNED_LOAD64(key_.data() + OFFSETOF_OFFSET); : } : : Slice ToSlice() const { : return key_; : } : : private: : // Key encoding stored in key_: : // : // int64_t mtime; : // int64_t offset; : // <variable length bytes> filename; : static constexpr int OFFSETOF_MTIME = 0; : static constexpr int OFFSETOF_OFFSET = OFFSETOF_MTIME + sizeof(int64_t); : static constexpr int OFFSETOF_FILENAME = OFFSETOF_OFFSET + sizeof(int64_t); : kudu::faststring key_; : }; : : /// The class to abstruct store behavior, including copying the buffer and holding it : /// until store complete. : class StoreTask { : public: : /// Creating a store task requires the filename, mtime, offset that constitutes the : /// cache key, and the buffer and length of the cached data is required too. We : /// allocate a new buffer in the constructor and copy the cache data and update : /// total_size which keeps track of the total buffer size allocate by all store tasks. : explicit StoreTask(const std::string& filename, int64_t mtime, int64_t offset, : const uint8_t* buffer, int64_t buffer_len, AtomicInt64& total_size); : : /// When the store task is destroyed, the allocated buffer is freed and total_size is : /// updated. : ~StoreTask(); : : const CacheKey& key() const { return key_; } : const uint8_t* buffer() const { return buffer_; } : int64_t buffer_len() const { return buffer_len_; } : : private: : DISALLOW_COPY_AND_ASSIGN(StoreTask); : : CacheKey key_; : uint8_t* buffer_; : int64_t buffer_len_; : AtomicInt64& total_size_; : }; > The reason the compiler gives an error about the incomplete type is because You are right, I forgot to move the c'tor and d'tor of DataCache so it failed to compile. Now it has been fixed. http://gerrit.cloudera.org:8080/#/c/19475/5/be/src/runtime/io/data-cache.cc File be/src/runtime/io/data-cache.cc: http://gerrit.cloudera.org:8080/#/c/19475/5/be/src/runtime/io/data-cache.cc@117 PS5, Line 117: DEFINE_int32(data_cache_num_write_threads, 0, : "Number of data cache write threads. Write threads will write the cache " : "asynchronously after IO thread read data, so IO thread will return more quickly. " : "Write threads need to bound the extra memory consumption for holding the temporary " : "buffer though. If this's 0, then write will be synchronous."); : DEFINE_string(data_cache_write_buffer_limit, "1GB", : "Limit of the total buffer size used by asynchronous store tasks."); > Let's describe these options as experimental by adding an "(Experimental)" Done -- To view, visit http://gerrit.cloudera.org:8080/19475 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I878f7486d485b6288de1a9145f49576b7155d312 Gerrit-Change-Number: 19475 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Wed, 01 Mar 2023 10:31:57 +0000 Gerrit-HasComments: Yes
