RFR 8220792: JavacFileManager.list() performance

[Jonathan Gibbons]

Responses inline, but there is one additional comment that occurred to 
me after my responses yesterday.

I guess I'm mildly concerned about the double caching caused by the 
proposed new cache in front of the cache built inside ArchiveContainer. 
I guess the point is that the ArchiveContainer should be stable (unless 
the jar file is mutated) but the locations are more easily mutable, 
either via options or setLocation* methods.

---

I'll read and comment on the new webrev separately.

-- Jon


On 3/27/19 9:06 AM, Ron Shapiro wrote:
> 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.
Mea culpa; transposition typo; I meant JDK-8150641, 
https://bugs.openjdk.java.net/browse/JDK-8150641 which was the work done 
by Jan to add the caching for ArchiveContainer.
>
> 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.

There were two ideas in my head when I was writing the note.

1. If it is worth indexing ArchiveContainer when it is opened, would it 
be worth indexing other containers as well?

2. Given that the answer to #1 may be "maybe, maybe not", the thought 
was that after you list any package for the first time, you know what 
containers the package is found in, so it (theoretically) ought to be 
easy to update a method like JavacFileManager.list so that it updates 
the main index for that package.  That being said, I guess the contents 
of a DirectoryContainer are "easily mutable", e.g. by annotation 
processing and so we should not prematurely cache info about its 
contents. But JRTImageContainer should not be mutable, and so may be 
worth indexing.


>
> 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.
Look for and use "javacFileManagerOptions", which gives all the file 
manager options. If any option is modified, I think it appropriate to 
clear the cache.
>
> > 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 agree with preserving the order expected by tests ;-)

I don't have an immediate explanation for the ordering issue.  If you 
need mutable lists, with the ability to append any time, then yes, use 
ArrayList. If you just need to build immutable lists, use ListBuffer, 
which has a fast append, and then call .toList()

>
> 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 <mailto: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/
>>     <http://cr.openjdk.java.net/%7Eronsh/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/18ef55d0/attachment-0001.html>