rmuir commented on PR #816: URL: https://github.com/apache/lucene/pull/816#issuecomment-1108554479
Thanks for getting the tests passing. I'm honestly torn on this issue, here are my thoughts: 1. If you have 100% cpu in `java.lang.ThreadLocal.remove()`, you are creating and destroying **far too many*** threads. It is not lucene's fault. The application needs to be fixed to not churn out so many threads. This problem is unique to the java ecosystem, I don't see it with any other programming languages. Again, there are two problems: a) developers using far too many threads (e.g. solr defaulting to `10000`!!!), and b) developers using "resizable thread pools" (min != max), which doesn't help anything and only magnifies these kinds of issues. The resizable ones just cause constant "churn" and GC pressure, and I know tomcat, jetty, etc love to default to that, but its so inefficient. Java developers should use reasonable fixed-sized threadpools just like everyone else does. Lucene can't fix these apps, they need to be fixed themselves. 2. I don't like that lucene now has basically a reimplementation of ThreadLocal in its codebase to cater to the problems of such bad applications. IMO, we are a search engine library, we should use `java.lang.ThreadLocal` and if apps use threads in an insane way, they should get the OOM that they deserve. 3. CloseableThreadLocal is confusing because it is doing two different things as opposed to java Threadlocal. At least let's consider updating the javadocs. The major differences are a) explicit close() for all threads, basically this clears the map when the storage is not needed anymore, and b) faster purging of values for threads that are "dead but not yet GC'd" via periodic liveliness check. 4. That being said, I don't have real opposition to this patch, but I want us to be careful about correctness. I am also concerned about not hurting the performance of well-behaved apps that don't do bad things. I'm not the best one to review the concurrency/performance implications, as I only have a small 2-core laptop and I can barely remember how Java language works. But let's not punish apps that use threads reasonably. I'm not concerned about performance of badly behaved apps, because they need to fix how they use threads, and we can't help them do that. -- 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]
