RFR: JDK-8255059: Regressions >5% in all Javadoc benchmarks in 16-b19

[Hannes Wallnöfer]

I have narrowed down the performance regression to the modularity check introduced in JDK-8240169. Since this check is not necessary if we know that the element list's modularity matches that of the library we can omit the modularity check when linking to platform libraries using our own element lists.

Unfortunately, while recent element lists match the modular JDK libraries, the ones for JDK 9 and 10 do not. The patch therefore adds module tags to the lists for these two versions. For JDK 10, this is a relatively simple change because the packages were already ordered by module. For JDK 9 the change unfortunately requires changing the order of packages. I've written a small utility program to convert the list and have double-checked its content matches the old list.

Performance should be very close to where it was before the regression. I haven't run the benchmarks on the final version because I currently have some background tasks running, but I will do so eventually before integration.

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

Commit messages:
 - JDK-8255059: Regressions >5% in all Javadoc benchmarks in 16-b19

Changes: https://git.openjdk.java.net/jdk/pull/2221/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2221&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255059
  Stats: 787 lines in 4 files changed: 447 ins; 331 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2221.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2221/head:pull/2221

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

[Hannes Wallnöfer]

On Mon, 25 Jan 2021 14:03:22 GMT, Hannes Wallnöfer <[hidden email]> wrote:

> I have narrowed down the performance regression to the modularity check introduced in JDK-8240169. Since this check is not necessary if we know that the element list's modularity matches that of the library we can omit the modularity check when linking to platform libraries using our own element lists.
>
> Unfortunately, while recent element lists match the modular JDK libraries, the ones for JDK 9 and 10 do not. The patch therefore adds module tags to the lists for these two versions. For JDK 10, this is a relatively simple change because the packages were already ordered by module. For JDK 9 the change unfortunately requires changing the order of packages. I've written a small utility program to convert the list and have double-checked its content matches the old list.
>
> Performance should be very close to where it was before the regression. I haven't run the benchmarks on the final version because I currently have some background tasks running, but I will do so eventually before integration.

This pull request has been closed without being integrated.

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

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

[Hannes Wallnöfer]

> I have narrowed down the performance regression to the modularity check introduced in JDK-8240169. Since this check is not necessary if we know that the element list's modularity matches that of the library we can omit the modularity check when linking to platform libraries using our own element lists.
>
> Unfortunately, while recent element lists match the modular JDK libraries, the ones for JDK 9 and 10 do not. The patch therefore adds module tags to the lists for these two versions. For JDK 10, this is a relatively simple change because the packages were already ordered by module. For JDK 9 the change unfortunately requires changing the order of packages. I've written a small utility program to convert the list and have double-checked its content matches the old list.
>
> Performance should be very close to where it was before the regression. I haven't run the benchmarks on the final version because I currently have some background tasks running, but I will do so eventually before integration.

Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision:

  Fix incorrect condition for JDK 9 and 10

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2221/files
  - new: https://git.openjdk.java.net/jdk/pull/2221/files/ac66c131..f6a82453

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2221&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2221&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2221.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2221/head:pull/2221

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

[Hannes Wallnöfer]

On Mon, 25 Jan 2021 14:25:59 GMT, Hannes Wallnöfer <[hidden email]> wrote:

>> I have narrowed down the performance regression to the modularity check introduced in JDK-8240169. Since this check is not necessary if we know that the element list's modularity matches that of the library we can omit the modularity check when linking to platform libraries using our own element lists.
>>
>> Unfortunately, while recent element lists match the modular JDK libraries, the ones for JDK 9 and 10 do not. The patch therefore adds module tags to the lists for these two versions. For JDK 10, this is a relatively simple change because the packages were already ordered by module. For JDK 9 the change unfortunately requires changing the order of packages. I've written a small utility program to convert the list and have double-checked its content matches the old list.
>>
>> Performance should be very close to where it was before the regression. I haven't run the benchmarks on the final version because I currently have some background tasks running, but I will do so eventually before integration.
>
> Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix incorrect condition for JDK 9 and 10

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java line 531:

> 529:                         DocPath pkgPath = DocPath.create(elemname.replace('.', '/'));
> 530:                         // Although being modular, JDKs 9 and 10 do not use module names in javadoc URL paths.
> 531:                         if (moduleName != null && platformVersion != 9 && platformVersion != 10) {

I know first version was fine, blame it on a lack of coffee :)

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

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

[Jonathan Gibbons-2]

On Mon, 25 Jan 2021 14:25:59 GMT, Hannes Wallnöfer <[hidden email]> wrote:

>> I have narrowed down the performance regression to the modularity check introduced in JDK-8240169. Since this check is not necessary if we know that the element list's modularity matches that of the library we can omit the modularity check when linking to platform libraries using our own element lists.
>>
>> Unfortunately, while recent element lists match the modular JDK libraries, the ones for JDK 9 and 10 do not. The patch therefore adds module tags to the lists for these two versions. For JDK 10, this is a relatively simple change because the packages were already ordered by module. For JDK 9 the change unfortunately requires changing the order of packages. I've written a small utility program to convert the list and have double-checked its content matches the old list.
>>
>> Performance should be very close to where it was before the regression. I haven't run the benchmarks on the final version because I currently have some background tasks running, but I will do so eventually before integration.
>
> Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix incorrect condition for JDK 9 and 10

OK, I see why the old code might be slow (checking all those package names), so I understand why this is a good fix.

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

Marked as reviewed by jjg (Reviewer).

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

[Hannes Wallnöfer]

On Mon, 25 Jan 2021 14:03:22 GMT, Hannes Wallnöfer <[hidden email]> wrote:

> I have narrowed down the performance regression to the modularity check introduced in JDK-8240169. Since this check is not necessary if we know that the element list's modularity matches that of the library we can omit the modularity check when linking to platform libraries using our own element lists.
>
> Unfortunately, while recent element lists match the modular JDK libraries, the ones for JDK 9 and 10 do not. The patch therefore adds module tags to the lists for these two versions. For JDK 10, this is a relatively simple change because the packages were already ordered by module. For JDK 9 the change unfortunately requires changing the order of packages. I've written a small utility program to convert the list and have double-checked its content matches the old list.
>
> Performance should be very close to where it was before the regression. I haven't run the benchmarks on the final version because I currently have some background tasks running, but I will do so eventually before integration.

This pull request has now been integrated.

Changeset: 0779adde
Author:    Hannes Wallnöfer <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/0779adde
Stats:     787 lines in 4 files changed: 447 ins; 331 del; 9 mod

8255059: Regressions >5% in all Javadoc benchmarks in 16-b19

Reviewed-by: jjg

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

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