On Tue, 23 Feb 2021 00:41:49 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
>>> there is very little change in the utilization of the background writer as >>> a function of the question size >> >> Did you mean "queue size"? If so, then I also noticed that in my observation >> no. 2 under the results table here: >> https://github.com/openjdk/jdk/pull/2665#issuecomment-783503529 For me, >> utilization roughly stayed at the same 20% mark regardless of the number of >> queued tasks. >> >> To me, this indicates conflicting readings. On the one hand, we have 20% of >> "utilization". On the other hand, 1/4 of attempts to acquire a permit to >> hand over a task to the executor fail. Put differently, in 1/4 of the cases >> the producer waits for the consumer who works at 20% capacity. Why? >> >> I think we should use suitable tools (e.g. profilers) and/or ask experts in >> performance. If nothing else, this will help us diagnose such issues in the >> future. > >> I think we should use suitable tools (e.g. profilers) and/or ask experts in >> performance. If nothing else, this will help us diagnose such issues in the >> future. > > Yes, but only a little bit yes. I think the "utilization" of > minimal/questionable value. The bottom line is a real-time reduction of 10% > or more, and, in my opinion, that is enough to declare success. > > The latest commit provides more fine-grain control over the number of > background threads and queue size, and increasing the number of threads or > the queue size actually starts to degrade overall performance, as measured in > real time over several runs of building the JDK docs. > > I think there are diminishing returns beyond this point, given that we cannot > easily introduce concurrency into the page generation (sigh) because of the > underlying javac architecture. Earlier in this review I said that the change is on the right track, but after examining the change more closely I'm no longer sure about that. This is because the change introduces multithreading to the code that wasn't designed with it in mind. Not only does such retrofitting require extreme care and resources, but it also changes the code forever. After the code is retrofitted, all future changes to it will have to be examined from multithreading perspective. Are we okay with that given modest benefits the change brings (i.e. 8%)? As an example of typical issues consider a couple of hazards which I overlooked when first reviewed the change: 1. Accesses to `jdk.javadoc.internal.tool.Messager` fields are only partially synchronized. 2.` HtmlDocument.write()` accesses objects that are shared across multiple threads. One such object is not thread-safe `JavacFileManager`, which has mutable state (e.g. a map of locations). *** I suppose that the motivation for this change was a performance improvement, right? If so, we could consider alternative options. One such option would be to switch file I/O to modern NIO. We could evaluate and benchmark that option to see if it achieves speedup similar to what you saw with this change (i.e. background threads). ------------- PR: https://git.openjdk.java.net/jdk/pull/2665
