On Fri, 12 Mar 2021 14:48:12 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>>> 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). Having looked at the the code changes, I think both of you have a point. I think the task of writing HTML documents to disk is very well suited to be done on a dedicated thread. However, it's also true that multi-threading can easily lead to unintended consequences even in seemingly simple cases. However, I think it would be a shame to give up so easily, as the changes so far are pretty simple, the benefits quite solid, and the problems not unsurmountable. Pavel, how exactly does `HtmlDocument.write()` access mutable objects like `JavacFileManager`? It wasn't quite obvious to me in a quick examination of the code. It seems like that method just writes out the content of the page. Of course, there's quite a few subclasses of `Content`, so it's quite possible I overlooked something critical. ------------- PR: https://git.openjdk.java.net/jdk/pull/2665
