RFR 8220792: JavacFileManager.list() performance

[Ron Shapiro]

Thanks for the comments!

I'm not able to see JDK-8150461 in JBS or in the mercurial history, but I
do like your suggestion on adding a method on Container instead of the
instanceof.

I think the tracking you discuss at the end may be challenging given that
JRTImageContainer and DirectoryContainer don't create an index right away.
I'm not sure exactly how to achieve what you're suggesting without some
heavy lifting.

As for the importing of Main.Option - I needed to do that to have the
jshell tests pass, since they continually update the classpath. I'm open to
other suggestions as I wasn't 100% sure how that works. I've updated the
other setLocation*() methods to also clear the caches as you mentioned.
Thanks for pointing that out.

> class PathAndContainer implements compareTo but not equals and hashCode
Good point. I'll add those for consistency.

> It's not clear to me why you maintain the list of PathAndContainer as a
sorted list, and hence why you need to use java.util.List instead of
javac's List.
There are some tests that ensure that the classpath that javac sees is
equivalent to the classpath the JVM sees vis-a-vis ordering. Without
sorting, javac was seeing a different order, and I presume that is because
the JRTImageContainer and DirectoryContainers could appear anywhere.
Sorting solved that issue. Additionally, I was seeing javac's List.append()
calls take up lots of time in profiling, but using ArrayList made that
disappear.

I have an updated webrev here:
http://cr.openjdk.java.net/~ronsh/8220792/webrev.01/

On Tue, Mar 26, 2019 at 3:52 PM Jonathan Gibbons <
jonathan.gibbons at oracle.com> wrote:

>
>
> On 03/18/2019 11:14 AM, Ron Shapiro wrote:
>
> Hi,
>
> Please review this change to improve the performance of
> JavacFileManager.list().
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8220792
> webrev: http://cr.openjdk.java.net/~ronsh/8220792/webrev.00/
>
> Thanks,
> Ron
>
>
> Ron,
>
> Look at these lines:
>
>  984         // First collect all of the containers that don't maintain their own index on 985         // RelativeDirectory. These need to always be included for all mappings 986         java.util.List<PathAndContainer> nonIndexingContainers = new ArrayList<>(); 987         for (PathAndContainer pathAndContainer : allPathsAndContainers) { 988             if (!(pathAndContainer.container instanceof ArchiveContainer)) { 989                 nonIndexingContainers.add(pathAndContainer); 990             } 991         }
>
> The "instanceof" on line 988 does not generally match the comment about
> "all of the
> containers that don't maintain their own index".
>
> Digging into the Mercurial histroy a bit, I see that the package index was
> added as a
> performance update in JDK-8150461. What if we were to do similar fixes on
> other
> subtypes of Container?  Your code would be more future-proof if the
> instanceof was
> replaced by a new method on the Container interface, indicating if the
> container
> maintained its own map.
>
> That being said, it would be even better if the "master index" was
> automatically updated
> whenever any directory was found in any container. That way, the master
> index would lazily
> learn about the contents of the JRTImageContainer and DirectoryContainer
> as well.
>
> -- Jon
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190327/20287bd3/attachment.html>