matanper opened a new issue, #70505: URL: https://github.com/apache/doris/issues/70505
### Search before asking - [x] I had searched in the [issues](https://github.com/apache/doris/issues?q=is%3Aissue) and found no similar issues. ### Version 4.1.4. master (`e125ea85867`) pins `contrib/clucene` at `08d45e5`, and the current `clucene` branch of apache/doris-thirdparty (`15836a0697e`) has the same code. ### What's Wrong? Destroying a CLucene index reader (for example when the inverted index searcher cache evicts one) has two problems in CLucene's thread-local registry, `src/core/CLucene/util/ThreadLocal.cpp`: 1. **It clears unrelated readers' state.** The destroying thread loses the cached per-thread values (term enumerators and similar) of every other reader it uses, not only the dying reader's. Those readers are still open, so the thread rebuilds those values on its next lookup. 2. **It stalls other scanner threads.** Removal walks every registered thread while holding the process-wide `threadData_LOCK`. `_ThreadLocal::set` takes the same mutex on every call. With many scanner threads and frequent evictions, scanner threads queue on that mutex, and query latency rises with the thread count and the eviction rate. ### Why `_ThreadLocal::~_ThreadLocal` (`ThreadLocal.cpp:155-161`): ```cpp _ThreadLocal::~_ThreadLocal() { setNull(); UnregisterCurrentThread(); RemoveThreadLocal( this ); delete _internal; } ``` - `UnregisterCurrentThread()` (`:274`) is meant for thread exit. It finds the current thread's `ThreadLocals` set and calls `ThreadLocals::UnregisterThread()` (`:324`), which calls `setNull()` on **every** `_ThreadLocal` the thread has registered, then removes the thread's entry. Called from one object's destructor, it nulls (and deletes, through their deletors) the values of all other live `_ThreadLocal` objects on that thread. - `RemoveThreadLocal(this)` (`:298-311`) locks `threadData_LOCK` and iterates over **all** registered threads, calling `ThreadLocals::remove(tl)` on each, including threads that never used this object. Its cost is O(threads × log registrations) under the global mutex, and `_ThreadLocal::set` (`:201`, lock at `:243`) needs the same mutex for every new value. In Doris every `TermInfosReader` holds a `ThreadLocal<SegmentTermEnum*>`, so every evicted searcher runs this destructor once per reader. ### What You Expected? Destroying one `_ThreadLocal` removes only that object's values and registrations. Other objects' values on the same thread stay intact. The work done under the global mutex is proportional to the threads that actually used the object, not to all threads. ### How to Reproduce? 1. Create two `ThreadLocal` objects A and B. On one thread, set a value in both, then destroy A on that thread. B's value on that thread is now NULL, and its deletor has run. 2. For the contention: run many threads (hundreds) that each open and use several index readers, and evict readers frequently (a small inverted-index searcher cache under a high-cardinality workload). Sample the threads: many wait on `threadData_LOCK` inside `_ThreadLocal::set`, and an evicting thread holds it in `RemoveThreadLocal`. A CLucene unit test reproduces (1), and a component benchmark that destroys readers while other threads look up terms reproduces (2). ### Anything Else? Proposed fix, without changing the public `_ThreadLocal` interface: - Drop `UnregisterCurrentThread()` from `~_ThreadLocal`. Remove only this object's value and registration, and keep `UnregisterCurrentThread()` for real thread exit. - Have each `_ThreadLocal` record handles to the registry entries of the threads that registered it. Its destructor then unlinks only those entries, in O(owners), and frees the nodes after releasing `threadData_LOCK`. - Make thread exit remove both directions before running value destructors, so a destructor that destroys another `_ThreadLocal` is safe. We have a patch with unit tests (ASAN, including thread-exit races) and a benchmark, and can open a PR to apache/doris-thirdparty. ### Are you willing to submit PR? - [x] Yes I am willing to submit a PR! ### Code of Conduct - [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) -- 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]
