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

[Pavel Rappo]

On Sun, 21 Feb 2021 18:11:06 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

> The standard doclet works by creating `HtmlDocument` objects and then writing them out. Writing them can be safely done on a separate thread, and doing so results in about an 8% speedup, by overlapping the write/IO time with the time to generate the next page to be written.
> 
> The overall impact on the codebase is deliberately small, coming down to a critical `if` statement in `HtmlDocletWriter` to "write now" or "write later". As such, it is easy to disable the feature if necessary, although no issues have been found in practice. The background writer uses an `ExecutorService`, which provides a lot of flexibility, but experiments show that it is sufficient to use a single background thread and to block if new tasks come available while writing a file.
> 
> The feature is "default on", with a hidden option to disable it if necessary.

This change is on the right track.

1. Although I'm not sure how "utilization" translates to a similar measurement obtained through a profiling tool, I do think we can use "utilization" as a rough estimate.

FWIW, on my machine the base "utilization" for JDK codebase is 13%. After I have moved the initialization of the `start` timestamp from the creation of the writer closer to the submission of the first task, "utilization" increased to 17%.

I'd recommend computing elapsed times using `java.lang.System.nanoTime`. Using `nanoTime` avoids some hazards of using `currentTimeMillis` for such purposes.

2. The `--background-writer` option is processed **after** the writer has been created. To see the "utilization" numbers I had to tweak the change and rebuild the project.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 40:

> 38: 
> 39: /**
> 40:  * A class to write {@link HtmlDocument} objects on a background thread,

I'd simply say "A class to write HtmlDocument objects in tasks of an ExecutorService" or some such. This is because `ExecutorService` is an abstraction of a level higher than that of `Thread`.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 79:

> 77:     /**
> 78:      * The main executor for the background tasks.
> 79:      * It uses a fixed thread pool of {@value #BACKGROUND_THREADS} threads.

What purpose does the adjective "main" have here?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 94:

> 92:      * The time at which the writer is initialized.
> 93:      */
> 94:     private long start;

I suggest making `start` final.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 105:

> 103:      * background thread.
> 104:      */
> 105:     private boolean verbose;

`verbose` should be final too.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 113:

> 111:      * The writer uses a {@link Executors#newFixedThreadPool fixed thread pool} of
> 112:      * {@value #BACKGROUND_THREADS} background threads, and a blocking queue of up to
> 113:      * {@value #QUEUED_TASKS} queued tasks.

The writer uses the pool but not the queue; the queue is encapsulated in the pool. Could you rephrase that?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 165:

> 163:         try {
> 164:             executor.shutdown();
> 165:             executor.awaitTermination(5, TimeUnit.MINUTES);

The writer can safely read the `taskBusy` field if `awaitTermination` returns `true`. Please check that boolean status.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlOptions.java line 718:

> 716:      * Set by command-lione option {@code --background-writer}.
> 717:      */
> 718:     public boolean isBackgroundWriterVerbose() {

Repeated typo "command-lione".

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 2:

> 1: /*
> 2:  * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.

Is this just an outdated copyright header or this class is really that old?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/BackgroundWriter.java line 97:

> 95: 
> 96:     /**
> 97:      * The cumulative time spent writing documents

Missing the trailing period.

-------------

Changes requested by prappo (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2665