RFR: JDK-8002152: javadoc could write out files on a worker thread [v3]

[Hannes Wallnöfer]

On Fri, 12 Mar 2021 14:48:12 GMT, Pavel Rappo <prappo at 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