wgtmac commented on code in PR #1158: URL: https://github.com/apache/orc/pull/1158#discussion_r906918128
########## c++/src/Utils.hh: ########## @@ -0,0 +1,63 @@ +/** + * 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. + */ + +#ifndef ORC_UTILS_HH +#define ORC_UTILS_HH + +#include <atomic> +#include <chrono> + +namespace orc { + +class AutoStopwatch { + std::chrono::high_resolution_clock::time_point start; + std::atomic<uint64_t>* latencyUs; + std::atomic<uint64_t>* count; + +public: + AutoStopwatch(std::atomic<uint64_t>* _latencyUs, + std::atomic<uint64_t>* _count) + : latencyUs(_latencyUs), + count(_count) { + if (latencyUs) { + start = std::chrono::high_resolution_clock::now(); + } + } + + ~AutoStopwatch() { + if (latencyUs) { + std::chrono::microseconds elapsedTime = + std::chrono::duration_cast<std::chrono::microseconds>( + std::chrono::high_resolution_clock::now() - start); + latencyUs->fetch_add(static_cast<uint64_t>(elapsedTime.count())); + } + + if (count) { + count->fetch_add(1); + } + } +}; + +#define DEFINE_AUTO_STOPWATCH(METRICS_PTR, LATENCY_VAR, COUNT_VAR) \ Review Comment: I'd suggest to rename DEFINE_AUTO_STOPWATCH to SCOPED_STOPWATCH because usually DEFINE_ prefix is used to define a class or something via marco expansion. ########## c++/src/Compression.cc: ########## @@ -463,6 +469,7 @@ DIAGNOSTIC_PUSH } bool DecompressionStream::Next(const void** data, int*size) { + DEFINE_AUTO_STOPWATCH(metrics, DecompLatencyUs, DecompCount); Review Comment: The stop watch should not be put here since it includes the io latency. If you move this line to line 498 right before the definition of **size_t availableSize**, what we got is the pure decompression time. ########## c++/src/sargs/SargsApplier.cc: ########## @@ -126,10 +127,14 @@ namespace orc { } while (rowGroup != 0); // update stats - mStats.first = std::accumulate( - mNextSkippedRows.cbegin(), mNextSkippedRows.cend(), mStats.first, - [](bool rg, uint64_t s) { return rg ? 1 : 0 + s; }); - mStats.second += groupsInStripe; + uint64_t selectedRGs = std::accumulate( + mNextSkippedRows.cbegin(), mNextSkippedRows.cend(), 0UL, + [](uint64_t initVal, uint64_t rg) { + return rg > 0 ? initVal + 1 : initVal; }); + if (mMetrics != nullptr) { Review Comment: What if the file or stripe has been skipped and no row group evaluation happens here? We may have missed them. ########## c++/include/orc/Reader.hh: ########## @@ -32,13 +32,40 @@ #include <set> #include <string> #include <vector> +#include <atomic> namespace orc { // classes that hold data members so we can maintain binary compatibility struct ReaderOptionsPrivate; struct RowReaderOptionsPrivate; + /** + * Expose the reader metrics including the latency and + * number of calls of the decompression/decoding/IO modules. + */ + struct ReaderMetrics { + std::atomic<uint64_t> ReaderCount{0}; + // ReaderInclusiveLatencyUs contains the latency of + // the decompression/decoding/IO modules. + std::atomic<uint64_t> ReaderInclusiveLatencyUs{0}; + std::atomic<uint64_t> DecompCount{0}; + // DecompLatencyUs contains the IO blocking latency. + std::atomic<uint64_t> DecompLatencyUs{0}; + std::atomic<uint64_t> DecodingCount{0}; + // DecodingLatencyUs contains the latency of the decompression/IO modules. Review Comment: I think we can do better here to get the pure decoding time. Please see the inline comments below. ########## c++/include/orc/Reader.hh: ########## @@ -32,13 +32,40 @@ #include <set> #include <string> #include <vector> +#include <atomic> namespace orc { // classes that hold data members so we can maintain binary compatibility struct ReaderOptionsPrivate; struct RowReaderOptionsPrivate; + /** + * Expose the reader metrics including the latency and + * number of calls of the decompression/decoding/IO modules. + */ + struct ReaderMetrics { + std::atomic<uint64_t> ReaderCount{0}; + // ReaderInclusiveLatencyUs contains the latency of + // the decompression/decoding/IO modules. + std::atomic<uint64_t> ReaderInclusiveLatencyUs{0}; + std::atomic<uint64_t> DecompCount{0}; Review Comment: I'd prefer to use the full name here. And maybe replace **Count** with **Call** ? ########## c++/src/RleDecoderV2.cc: ########## @@ -432,6 +435,7 @@ void RleDecoderV2::skip(uint64_t numValues) { void RleDecoderV2::next(int64_t* const data, const uint64_t numValues, const char* const notNull) { + DEFINE_AUTO_STOPWATCH(metrics, DecodingLatencyUs, DecodingCount); Review Comment: Where is the stop watch call in the RleDecoderV1 ? ########## c++/src/RleDecoderV2.cc: ########## @@ -432,6 +435,7 @@ void RleDecoderV2::skip(uint64_t numValues) { void RleDecoderV2::next(int64_t* const data, const uint64_t numValues, const char* const notNull) { + DEFINE_AUTO_STOPWATCH(metrics, DecodingLatencyUs, DecodingCount); Review Comment: In addition, all IOs happen in the call of readByte(). We probably should make metrics a member varialbe of the RleDecoder and counts its io time in readByte(). Finally, we can exlucde the io time from decoding latancy here to get the pure RLE decoding time. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
