RFR [15] 8238969: Miscellaneous cleanup

Tue Feb 18 21:07:58 UTC 2020

Looks good.  I've been through it all again, and it looks OK. There's 
minor additional specific cleanup that comes to mind while reading the 
affected areas, but I think we should wrap up this round at this point.

BaseConfiguration:366
There's still a .stream().forEach here. Is this deliberate?

TagletWriter:290,etc
Maybe a separate cleanup of tweaking sentences for @param and @return

Start:477/552
I think you should use separate, better-named local variables for the 
start time (nanos)
and elapsed time (millis). "It would be nice" if there was better API to 
print out
elapsed time in a more adaptable format, but as an alternative, "it 
would be nice"
if javadoc was faster and fast-enough that the elapsed time was not 
interesting.

No need for re-review if you just address these issues.

-- Jon

On 02/17/2020 06:40 AM, Pavel Rappo wrote:
> Jon,
>
> I have updated the webrev:
>
>    http://cr.openjdk.java.net/~prappo/8238969/webrev.01/
>
> The main changes are:
>
> 1. Removed the "Consider this while working on JDK-8238966" comments.
> I have also updated the JDK-8238966 issue, adding a link to this thread so the
> discussion and the findings won't be lost.
>
> 2. Downgraded uses of Stream.forEach to Collections' forEach.
> It is both correct and less noisy.
>
> Come to think of it, there's one particular case where Collections' forEach is
> definitely more readable than that of Stream's. It's Map.forEach. Here you get a
> benefit of the key-value pair being already bound to separate variables, rather
> than to Map.Entry. Which is nice because you can give them meaningful names.
>
> 3. As for nanoTime, it's a hiccup from me working on core-libs networking. Even
> though in this particular case time is NOT used to control stuff (e.g. timeout),
> nanoTime is still a correct way of measuring the elapsed time. Being monotonic,
> it is immune to many problems in this area. So, there's value in that change as well.
>
> All the feedback you've provided so far has been considered. Most of the changes
> have been included in that new webrev. All the tests pass in our CI system.
>
> -Pavel
>
>> On 14 Feb 2020, at 21:57, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>
>> Responding to the details in your message.  Comments inline.
>>
>>
>> On 02/13/2020 07:50 AM, Pavel Rappo wrote:
>>> Hello,
>>>
>>> Please review the change for
>>> https://bugs.openjdk.java.net/browse/JDK-8238969
>>> :
>>>
>>>   
>>> http://cr.openjdk.java.net/~prappo/8238969/webrev.00/
>>>
>>>
>>> During the last exploratory debugging session, I collected a number of issues
>>> that I think are worth fixing. These include:
>>>
>>> 1. Indentation, unnecessary (and hopefully non-controversial) parentheses,
>>> local variables' names, typos, method references (where practical), comments, etc.
>>>
>> Generally OK
>>
>>> 2. Usages of the java.util.stream API. This could be thought of as complementary
>>> change to that [1] of by Jonathan Gibbons, though it stemmed from an unrelated
>>> investigation of mine.
>>>
>>> In a nutshell, some sites that use streams are order-sensitive, while others
>>> are not. Where the order is important, I used forEachOrdered() or changed the
>>> thing straight to Iterable.forEach(), eliding the in-between call to stream().
>>> In some cases that allowed to simplify the thing into an atomic (as in "indivisible",
>>> not "concurrent-atomic") operation from Collections API. For example:
>>>
>>>      - jdk/javadoc/internal/doclets/formats/html/LinkFactoryImpl.java:136,138
>>>      - jdk/javadoc/internal/doclets/toolkit/util/Utils.java:3268
>>>
>>> Where the order was immaterial, I emphasized that by ensuring a non-deterministic
>>> semantics was used. For that reason, in some places I inserted an in-between call
>>> to stream(), if there was none previously.
>>>
>>>      I'm still undecided on whether to use streams where the order is immaterial.
>>>      It's not about optimization, but readability. I guess it all depends on the
>>>      context and potential for using intermediate operations. It seems to be a
>>>      no-brainer when no intermediate operations, such as filtering or mapping,
>>>      are involved.
>>>
>>> Sadly, we don't seem to have good tools at hand for testing things like that.
>>> One way this change could be tested is through using "unfriendly" (work-to-rule)
>>> implementations of Collections and Stream APIs. For example, if the order of
>>> iteration is unspecified, or better still, explicitly specified to be non-deterministic,
>>> the implementation should make sure that the order is random. If a List is not
>>> of the RandomAccess flavor it should impose a perceivable penalty for getting
>>> elements by index, a call to Thread.sleep or a busy-loop would do.
>>>
>>> I'm sure such things exist in one form or another. We just don't have them, and
>>> implementing those on our own is a project not for the faint-hearted! Immutable
>>> collections, introduced in JEP 269, are good in this sense, though they are not
>>> enough:
>>>
>>>      The iteration order of set elements/mappings is unspecified and is subject
>>>      to change
>>>
>>> But I digress. The existing battery of tests passes.
>>>
>> I think it is a mis-feature of the API design that Iterable.forEach and Stream.forEach
>> sounds so similar yet have such an important difference in semantics. This makes
>> me want to avoid Stream.forEach entirely. I'm OK with generally keeping code
>> deterministic and restricting use to Iterable.forEach.
>>
>>
>>> 3. Pinpoint uses of modern String APIs (and boy, did they fit in nicely!)
>>>
>>>     - jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java:202
>>>     - jdk/javadoc/internal/tool/Start.java:289,291
>>>
>> Hmmm. it was not clear to me that " ".repeat(4) was a better alternative to "    ".
>> At 18, yes, it's beginning to be more useful.
>>
>>> 4. Marking with in-line comments "Consider this while working on JDK-8238966"
>>>
>>> This is about future work on extracting joining-with-a-separator functionality.
>>> I believe those comments have value, even though the issue they mark is not
>>> addressed in this patch. Some of the sites seem to be a low-hanging fruit, but
>>> the point is to consider them as a whole before proceeding with a fix.
>>>
>> As I noted in the review, I don't think these comments belong in the code.
>> They are interesting work product, however.
>> That being said, there are (at least) 3 subsets here.
>> 1. composing localizable lists in strings, e.g. for error messages
>> 2. composing non-localizable lists in strings (thinking of the search index .js files)
>> 3. composing localizable lists in Content nodes
>>
>>
>>> 5. Removal of the Result.serialVersionUID field
>>>
>>> I couldn't think of any use for that field. My best guess is that it's just a
>>> leftover from the times when this enum was Error.
>>>
>> Probably the result of checking that Serializable items have serialVersionUID at some point.
>>
>>> Notes
>>> =====
>>>
>>> a. jdk/javadoc/internal/doclets/formats/html/AbstractTreeWriter.java:143
>>>
>>> Shouldn't it use equals() instead of `==` in this case? A quick look shows a
>>> surprising number of reference equality checks on javax.lang.model.element.Name
>>> and javax.lang.model.element.Element instances. Why would we need to use
>>> reference equality on types with explicitly defined equals() and hashCode()?
>>>
>> Various aspects here:
>> 	• It is surprising/disappointing that .equals is defined for one impl of Name and not the other, or more accurately, that it is defined differently for both.
>> 	• It is surprising/disappointing that there is no fast-track for `==` in the one case where it is implemented.
>> 	• While it is true that javac Name is defined/used such that `==` is sufficient, there are not such explicit claims on the javax.lang.model super-interface that was retrofitted in JDK 6.  So, unless we change javax.lang.model.Name, it is probably reasonable to make .equals faster and change to using that.  But (separately) I note that javadoc (the tool in particular) is fundamentally dependent on javac and its internals, and so it is not all bad that we make assumptions on that basis.
>> If we follow up, it should be done separately.
>>
>>
>>> b. What do people think of removing unused type members?
>>>
>>> Since jdk.javadoc doesn't seem to be using java.lang.reflect (beyond a couple of
>>> trivial interactions with Doclet SPI), the compile-time checking should be enough
>>> to make sure the removal is safe.
>>>
>>> The are some 100 (hundred) of unused methods across the jdk.javadoc codebase.
>>>
>> I'd want to see the list and proceed cautiously, and separately, perhaps even on a
>> class-by-class basis.
>>
>>> -Pavel
>>>
>>> -------------------------------------------------------------------------------
>>> [1]
>>> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-February/001390.html
>>>
>>>
>>>
>>