On Sun, 21 Feb 2021 18:11:06 GMT, Jonathan Gibbons <[email protected]> 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
