morningman commented on code in PR #68713: URL: https://github.com/apache/doris/pull/68713#discussion_r4179274644
########## be/src/util/jni_scan_heap_gate.cpp: ########## @@ -0,0 +1,144 @@ +// 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. + +#include "util/jni_scan_heap_gate.h" + +#include <algorithm> +#include <chrono> +#include <limits> +#include <utility> + +#include "common/config.h" +#include "common/logging.h" +#include "util/jni-util.h" +#include "util/time.h" + +namespace doris { + +namespace { + +// How often a waiting reader looks again although nobody told it to: to see its query cancelled and +// its wait run out. +constexpr auto POLL_INTERVAL = std::chrono::milliseconds(100); +constexpr int64_t MB = 1024 * 1024; + +// A share of the -Xmx the JVM was started with, read from the same options the JVM was created from +// - not measured, so nothing here calls into the JVM. +int64_t jvm_heap_budget() { + const double budget = static_cast<double>(Jni::Util::get_max_jni_heap_memory_size()) * + config::jni_scanner_heap_budget_ratio; + // A BE_TEST build reports an unlimited heap (SIZE_MAX), which stays unlimited here. + constexpr auto UNLIMITED = std::numeric_limits<int64_t>::max(); + return budget >= static_cast<double>(UNLIMITED) ? UNLIMITED : static_cast<int64_t>(budget); +} + +} // namespace + +void JniScanHeapGate::Permit::release() { + if (_gate != nullptr) { + _gate->_release(_bytes); + _gate = nullptr; + _bytes = 0; + } +} + +JniScanHeapGate::JniScanHeapGate(std::function<int64_t()> budget) : _budget(std::move(budget)) {} + +JniScanHeapGate* JniScanHeapGate::instance() { + // Never destroyed: BE's exit runs static destructors while scanner threads are still alive, and + // a reader closing then releases its permit into this gate. + static auto* gate = new JniScanHeapGate(jvm_heap_budget); + return gate; +} + +void JniScanHeapGate::acquire(int64_t bytes, const std::function<bool()>& stop_waiting, + Permit* permit, int64_t* wait_ns) { + DORIS_CHECK(bytes > 0); + DORIS_CHECK(permit != nullptr); + DORIS_CHECK(!permit->held()); + DORIS_CHECK(wait_ns != nullptr); + const int64_t start = MonotonicNanos(); + std::unique_lock lock(_lock); + const uint64_t ticket = _next_ticket++; + _waiting.push_back(ticket); + while (true) { + // Asked without the lock: it is the caller's code. + lock.unlock(); + const bool stop = stop_waiting(); + lock.lock(); + const int64_t waited = MonotonicNanos() - start; + bool admit = stop || _fits(ticket, bytes); + if (!admit && waited >= config::jni_scanner_heap_max_wait_ms * 1000 * 1000) { Review Comment: Fixed in 1ce1680237d. `acquire()` now returns whether it admitted the reader. A reader whose scan stopped while it waited -- a cancelled query, or on V2 a scanner told to stop -- leaves the line without a permit and without touching the account, and the readers behind it are woken, since one of them may now be first and fit. `JniReader::open` then returns the query's cancel reason, and `JniTableReader::_open_jni_scanner` ends the split the way `get_block()` ends one whose scan stopped (eof, then close), so no Java scanner is created. Both now wait before attaching to the JVM at all. On V1 it was worse than an extra open: `FileScanner` checks cancellation only at the top of its loop, so a reader admitted this way read its first block in the same pass -- for a merge split, the block that loads the row groups of all its files. A cancelled query's waiting readers all did their heaviest step at once, above the budget. Tests: `JniScanHeapGateTest.AReaderWhoseScanStopsLeavesWithoutAShare` (in place of the case that expected an over-budget permit), `AReaderWhoseScanHasStoppedTakesNothingEvenWhenItFits` and `AStoppedReaderFirstInLineLetsTheNextOneIn`; and `JniTableReaderTest.ASplitWhoseScanStoppedOpensNoJavaScannerForTheHeapItDeclared`, which drives the real `_open_jni_scanner()` with a stopped scan and a range that declares its heap: no permit, no Java scanner, the split ends with eos. ########## be/src/util/jni_scan_heap_gate.cpp: ########## @@ -0,0 +1,144 @@ +// 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. + +#include "util/jni_scan_heap_gate.h" + +#include <algorithm> +#include <chrono> +#include <limits> +#include <utility> + +#include "common/config.h" +#include "common/logging.h" +#include "util/jni-util.h" +#include "util/time.h" + +namespace doris { + +namespace { + +// How often a waiting reader looks again although nobody told it to: to see its query cancelled and +// its wait run out. +constexpr auto POLL_INTERVAL = std::chrono::milliseconds(100); +constexpr int64_t MB = 1024 * 1024; + +// A share of the -Xmx the JVM was started with, read from the same options the JVM was created from +// - not measured, so nothing here calls into the JVM. +int64_t jvm_heap_budget() { + const double budget = static_cast<double>(Jni::Util::get_max_jni_heap_memory_size()) * + config::jni_scanner_heap_budget_ratio; Review Comment: Both premises hold: a non-empty `JAVA_OPTS` without `-Xmx` goes to the JVM as is, and `Util::get_max_jni_heap_memory_size()` then assumes 1 GiB. It takes an operator to get there: the shipped `be.conf` sets `-Xmx2048m`, and `start_be.sh` supplies `-Xmx1024m` when `JAVA_OPTS_FOR_JDK_17` is unset, so the fallback needs `-Xmx` removed, written as `-XX:MaxHeapSize=` (which the parser does not read), or replaced by a percentage. On a BE host the JVM's own default heap is far above 1 GiB, so the realistic error is a budget too small -- readers queue sooner than they need to -- and an out-of-heap hint that names the wrong maximum; a budget larger than the heap needs a heap set below 1 GiB by percentage, as in the example. The parser predates this PR and also rate-limits HDFS writes through JNI (`max_hdfs_wirter_jni_heap_usage_ratio`). The right fix -- read `Runtime.getRuntime().maxMemory()` once when `JvmLauncher` creates the JVM, a constant rather than a measurement -- changes all three of its users, so I'll make it a change of its own rather than fold it in here. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
