github-actions[bot] commented on code in PR #67824: URL: https://github.com/apache/doris/pull/67824#discussion_r4267734045
########## fe/fe-core/src/main/java/org/apache/doris/buildtools/IncrementalSourceMarker.java: ########## @@ -0,0 +1,703 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.buildtools; + +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; +import java.io.DataInputStream; +import java.io.DataOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.FileTime; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.stream.Stream; + +/** + * Computes the set of sources which must be recompiled after a local edit and marks them for the + * compiler plugin, so that {@code mvn compile} stops rebuilding the whole fe-core module. + * + * <p>Background: {@code maven-compiler-plugin} is all-or-nothing by default (one stale source + * recompiles every source of the module), and its {@code useIncrementalCompilation=false} mode, + * which does compile only the stale sources, is unsafe on its own because it never recompiles the + * classes which <em>use</em> a changed class. + * + * <p>This tool closes that gap. For every scanned source file it caches, keyed by the content hash: + * <ul> + * <li>the identifiers the file mentions, which over-approximates the types it depends on,</li> + * <li>the type names the file declares.</li> + * </ul> + * A run then computes + * <pre> + * changed = files whose content changed, plus files whose set of declared names changed + * (a type was added, removed, renamed or moved) + * affected = changed, closed over "mentions a type name declared by an affected file" + * </pre> + * and updates the last modification time of every affected file which belongs to the main sources. + * The compiler plugin then compiles exactly that set. Because every file which mentions a changed + * type is touched, no class is left referring to a type which was changed or deleted underneath it; + * the set is a conservative superset, so the only cost of an imprecise name match is a few extra + * files being recompiled. + * + * <p>The compiler plugin still owns the compilation, its annotation processing and its classpath. + * This tool only decides which sources are stale. + */ +public class IncrementalSourceMarker { + /** Bump it whenever the cached facts change, then the index is rebuilt from scratch. */ + private static final int FORMAT_VERSION = 2; + + /** The java keywords which introduce a type name. {@code interface} also covers {@code @interface}. */ + private static final Set<String> DECLARATION_KEYWORDS = new HashSet<>( + Arrays.asList("class", "interface", "enum", "record")); + + /** The keywords which introduce the supertypes of a type, i.e. what it inherits from. */ + private static final Set<String> SUPERTYPE_KEYWORDS = new HashSet<>( + Arrays.asList("extends", "implements")); + + /** + * The reserved java keywords, which can never be a type name. They are dropped from the mentioned + * identifiers to keep the index small and to avoid matching the keywords that would otherwise + * appear in every file. Contextual keywords such as {@code record} or {@code var} are deliberately + * not listed, they can be type names. + */ + private static final Set<String> RESERVED_KEYWORDS = new HashSet<>(Arrays.asList( + "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", "class", "const", + "continue", "default", "do", "double", "else", "enum", "extends", "final", "finally", "float", + "for", "goto", "if", "implements", "import", "instanceof", "int", "interface", "long", "native", + "new", "package", "private", "protected", "public", "return", "short", "static", "strictfp", + "super", "switch", "synchronized", "this", "throw", "throws", "transient", "try", "void", + "volatile", "while", "true", "false", "null", "_")); + + /** entry point, see the {@code fast-fe} profile of fe-core/pom.xml. */ + public static void main(String[] args) throws Exception { + long startNanos = System.nanoTime(); + Map<String, String> options = parseOptions(args); + List<Path> sourceRoots = canonicalRoots(requireOption(options, "sources")); + // Every path is derived from these canonical roots, so that a checkout reached through a + // symlink (mktemp gives /var/... on macOS, whose canonical form is /private/var/...) yields + // the same absolute paths here and in the index of the next run. + Path touchRoot = canonical(Paths.get(requireOption(options, "touch-root"))); + Path cacheFile = Paths.get(requireOption(options, "cache")); + Path classesDir = options.containsKey("classes") + ? canonical(Paths.get(options.get("classes"))) : null; + + List<Path> sourceFiles = findJavaFiles(sourceRoots); + SourceIndex index = SourceIndex.load(cacheFile); + + Map<String, Facts> factsByPath = new LinkedHashMap<>(); + List<Facts> derivedSources = new ArrayList<>(); + Map<String, List<String>> declaredBy = new TreeMap<>(); + Map<String, List<String>> referrersBy = new TreeMap<>(); + Map<String, List<String>> subtypesBySuperType = new TreeMap<>(); + Set<String> changed = new TreeSet<>(); + int scanned = 0; + for (Path sourceFile : sourceFiles) { + String path = sourceFile.toString(); + byte[] contentHash = contentHash(sourceFile); + Facts facts = index.get(path, contentHash); + if (facts == null) { + facts = scan(sourceFile, contentHash); + index.put(path, facts); + changed.add(path); + scanned++; + } + factsByPath.put(path, facts); + if (!sourceFile.startsWith(touchRoot)) { + derivedSources.add(facts); + } + for (String declaredName : facts.declaredNames) { + declaredBy.computeIfAbsent(declaredName, name -> new ArrayList<>()).add(path); + } + for (String identifier : facts.identifiers) { + referrersBy.computeIfAbsent(identifier, name -> new ArrayList<>()).add(path); + } + // A class inherits the members of its supertypes, so a change to a supertype reaches the + // files which name one of its subtypes even though they never name the supertype itself + // (B extends A, C uses B.VALUE: changing A.VALUE must recompile C as well, its class file + // holds the inlined constant). + for (String superTypeName : facts.superTypeNames) { + subtypesBySuperType.computeIfAbsent(superTypeName, name -> new ArrayList<>()) + .addAll(facts.declaredNames); + } + } + + // A file which disappeared, or a type which is not declared by the same files as before + // (added, removed, renamed, moved to another package), invalidates the files which mention + // that name exactly like a content change does. A removed file itself has nothing left to + // compile, only its classes have to go away, and its type names are already covered by the + // names whose declaring files changed. + Set<String> affected = new TreeSet<>(); + for (String staleName : index.replaceDeclaredNames(declaredBy)) { + addReferrers(affected, referrersBy, staleName); + } + for (Map.Entry<String, Facts> removed : index.removeMissing(sourceFiles).entrySet()) { + removeClasses(Paths.get(removed.getKey()), touchRoot, classesDir, removed.getValue().topLevelNames); + } + affected.addAll(changed); + + // Propagate over the types a changed file declares, plus every subtype of them: a source + // which is recompiled without being edited keeps its own declarations, but its effective api + // still changes when one of its supertypes changes. + // + // The other exception is a source derived from another one, such as the Immutables output of + // an annotated class: it is regenerated when its input changes, so its api may change too, and + // the files which use it need a recompilation as well. + Set<String> seedNames = new TreeSet<>(); + for (String path : changed) { Review Comment: [P1] Propagate changes through recompiled constant aliases After a successful `-Pfast-fe` build with `-Dmaven.build.cache.enabled=false`, changing only `FeConstants.INTERNAL_DB_NAME` marks `StatisticConstants`, whose `FULL_QUALIFIED_PARTITION_STATS_TBL_NAME` is a compile-time expression using that value. It never seeds `StatisticConstants` for another reference hop, so `BaseAnalysisTask` (which uses the composed constant at lines 269/574/587 but never names `FeConstants`) remains untouched. Maven's per-file compilation keeps its old class, including SQL with the old database name. An unchanged source can therefore expose a different constant when recompiled even without an `extends`/`implements` edge: `A.VALUE -> B.COPIED = A.VALUE -> C` still leaves C stale. This differs from the fixed inherited-member thread and the upstream-module thread: the declarations are all indexed main sources, and constant-expression reexports need propagation beyond the new subtype closure. Propagate affected declarations transitively, or use ABI/constant fingerprints to prove that propagation can stop. This follows the [Java constant-inlining rules](https://docs.oracle.com/javase/specs/jls/se17/html/jls-13.html#jls-13.4.9) and [compiler 3.10.1 per-file source selection](https://github.com/apache/maven-compiler-plugin/blob/maven-compiler-plugin-3.10.1/src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java#L893-L927); no build was run in this review. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
