RFR [15] 8239243: Create index structures only if required

[Jonathan Gibbons]

In a number of places, you use a variable or field named "indexbuilder" 
that would be better written in all situations as "indexBuilder".  No 
need for re-review to fix that.

The work is good, for what it does, but it triggers a discussion for a 
potential followup.   What follows is a discussion for future work, not 
necessarily part of this review.

"IndexBuilder" is "old" and now is only half the story. It's the A-Z 
index, and is unrelated to the interactive search index. There's a bunch 
of fields in HtmlConfiguration that could arguably be moved/merged into 
an imporved abstraction of IndexBuilder that is able to manage the A-Z 
index as well as the interactive index. Either that, or, IndexBuilder is 
renamed to AZIndexBuilder, and we create a new sibling structure for the 
interactive search.    For the fields in question, look at this list 
from HtmlConfiguration:

protected SortedSet<SearchIndexItem>memberSearchIndex;
protected SortedSet<SearchIndexItem>moduleSearchIndex;
protected SortedSet<SearchIndexItem>packageSearchIndex;
protected SortedSet<SearchIndexItem>tagSearchIndex;
protected SortedSet<SearchIndexItem>typeSearchIndex;
protected Map<Character,List<SearchIndexItem>>tagSearchIndexMap =new HashMap<>();
protected Set<Character>tagSearchIndexKeys;

These fields, and probably much of the code to manage them should be 
moved out of HtmlConfiguration into either IndexBuilder or a some new 
abstraction to manage these index objects.  This would be 
philosophically similar to the recent cleanup to move the collections of 
fields for options out into distinct abstractions.

-- Jon

On 02/18/2020 07:13 AM, Pavel Rappo wrote:
> Hi Hannes,
>
> I've updated the webrev:
>
>    http://cr.openjdk.java.net/~prappo/8239243/webrev.01/
>
> Thanks!
>
>> On 18 Feb 2020, at 09:52, Hannes Wallnöfer <hannes.wallnoefer at oracle.com> wrote:
>>
>> Hi Pavel,
>>
>> +1 for the conditional index building, and the IndexBuilder cleanup is a major improvement in every respect.
>>
>> Nitpicking here, but two naming suggestions:
>>
>> - „index“ can be both verb and noun, so renaming the „index“ method to „indexElements“ might make its meaning a bit clearer and puts it in line with the other index* methods
>>
>> - since firstCharacter(String) doesn’t always return the first character maybe something like „indexCharacter“ or „keyCharacter“ would be more fitting?
>>
>> Hannes
>>
>>
>>> Am 17.02.2020 um 16:35 schrieb Pavel Rappo <pavel.rappo at oracle.com>:
>>>
>>> Hello,
>>>
>>> Please review the change for https://bugs.openjdk.java.net/browse/JDK-8239243:
>>>
>>> http://cr.openjdk.java.net/~prappo/8239243/webrev.00/
>>>
>>> I noticed that the index structure, used in Index Page an others, is created
>>> unconditionally (i.e. regardless of the presence of the "-noindex" command-line
>>> option). Since populating that index doesn't seem to have any side-effects, this
>>> will unnecessarily consume resources of documentation generation process when no
>>> index is required. Thus, moving it under the relevant if-statement should be
>>> safe and the right thing to do. I've also cleaned up the IndexBuilder class a bit.
>>>
>>> In case you wonder, when generating the API docs for the JDK 15, it takes about
>>> 1 sec. (one second) to create that structure. The number of instances of Element
>>> in that Map<Character, SortedSet<Element>> is roughly 50,000 (fifty thousand).
>>>
>>> -Pavel
>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20200218/5768d96e/attachment.htm>