RFR: 8344148: Add an explicit compiler phase for warning generation

[Maurizio Cimadamore]

On Wed, 13 Nov 2024 22:25:36 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

> Please review this patch which does some minor refactoring to the compiler:
> * Create a new `WARN` phase which can be a dedicated home for (new) lint/warning logic
> * Create a new `WarningAnalyzer` singleton whose job is to invoke such lint/warning logic
> * Move `ThisEscapeAnalyzer` out of `Flow` (where it doesn't belong) and into `WarningAnalyzer`
> * Refactor `ThisEscapeAnalyzer` to be a context singleton like all other such classes
> 
> See [JDK-8344148](https://bugs.openjdk.org/browse/JDK-8344148) for details.

> > We are faced with a dilemma: we could fold all the various linter analyses in a single tree scanner -- or we could have one tree scanner per linter (or somewhere in between).
> 
> It's an interesting design question. Performance testing is always authoritative of course, but intuitively I would guess the relative cost of doing separate scans would be low because the average `JCTree` probably (?) more-or-less fits into cache, and the efficiency of having a separate scan per "finder" depends mainly on how well the average `JCTree` fits into cache, because that's the memory that is being repeatedly accessed by separate scans that wouldn't be repeatedly accessed in a combined scan.
> 
> Moreover, the current `JCTree` should always be "hot" during compilation... it's accessed all over the place... so if it fits in cache, then separate scans are going to be fast, and if it doesn't fit in cache, then things are _already_ going to be slow, and having a single combined scan probably wouldn't increase performance by much.
> 
> In any case, what would a combined scanner look like? `TreeScanner`'s are "proactive" and initiate subtree scanning themselves, so you couldn't combine them unless e.g. you changed the semantics of `scan()` to just add the subtree nodes to a queue, which might make life harder. That idea would look something like this:
> 
> ```java
> /** Applies multiple tree scanners to a tree in a single pass.
>  */
> public class CompositeTreeScanner extends TreeScanner {
> 
>     private final LinkedHashMap<JCTree, List<MemberTreeScanner>> queue = new LinkedHashMap<>();
> 
>     public void scan(JCTree tree, List<MemberTreeScanner> members) {
>         members.forEach(member -> member.parent = this);
>         queue.put(tree, members);
>         while (!queue.isEmpty())
>             dequeueAndScan();
>     }
> 
>     private void dequeueAndScan() {
>         var i = queue.entrySet().iterator();
>         var entry = i.next();
>         i.remove();
>         entry.getValue().forEach(entry.getKey()::accept);
>     }
> 
>     /** One of multiple scanners applied by a {@link CompositeTreeScanner}.
>      *  <b>Warning</b>: Calls to {@link #scan} are executed asynchronously.
>      */
>     public static class MemberTreeScanner extends TreeScanner {
> 
>         private CompositeTreeScanner parent;
> 
>         @Override
>         public void scan(JCTree tree) {
>             parent.queue.computeIfAbsent(tree, t -> new ArrayList<>()).add(this);
>         }
>     }
> }
> ```
> 
> OTOH you could easily combine `Visitor`'s, which are "reactive", with the downside being that a visitor will scan all of the tree nodes, even those that none of this individual finders wishes to visit.

I think it would be worth starting with a simple design (e.g. one visitor per analysis first), but then try to implement a certain number of them (say 4-5) and see whether e.g. compiling something big (like the JDK) suffers from the number of extra passes. In the past we have been a bit schizophrenic on this topic: sometimes we always apply a pass (e.g. TransTypes), but starting with lambdas we started checking if a pass had anything to do (see LambdaToMethod and TransPatterns). I don't think any evidence was ever gathered to make such passed optional, but I think this is a question we should settle once and for all before aiming for a particular design.

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

PR Comment: https://git.openjdk.org/jdk/pull/22088#issuecomment-2477241252