github-actions[bot] commented on code in PR #67824:
URL: https://github.com/apache/doris/pull/67824#discussion_r4267461580
##########
build-support/tests/test-incremental-source-marker.sh:
##########
@@ -0,0 +1,173 @@
+#!/bin/bash
Review Comment:
[P2] Make the new self-test executable
This file is committed as mode `100644`, while `build-support/tests/run.sh`
directly executes every `test-*.sh` under `set -e`. Consequently, `bash
build-support/tests/run.sh` stops with `Permission denied` when it reaches this
test, and these cases never run through the aggregate entry point. Commit it as
`100755`, matching the existing self-tests.
##########
fe/fe-core/pom.xml:
##########
@@ -134,6 +134,67 @@ under the License.
</dependency>
</dependencies>
</profile>
+ <!--
+ Fast incremental compilation, for the local edit / compile loop:
`mvn -Pfast-fe ...`.
+
+ maven-compiler-plugin is all-or-nothing by default, so one edited
file recompiles all ~4500
+ sources of fe-core. In its `useIncrementalCompilation=false` mode it
only compiles the
+ sources which are newer than their class files, but that mode alone
is unsafe: it never
+ recompiles the classes which use the changed one.
IncrementalSourceMarker closes that gap by
+ touching the changed sources and every source which mentions a type
they declare, so javac
+ is handed exactly the safe set.
+
+ The set is a conservative superset based on identifier matching: a
source is recompiled
+ whenever it mentions a type declared by a changed source, so nothing
can be left referring to
+ a type which moved or disappeared underneath it. Only a coupling
which is not spelled out in
+ the source at all (reflection, ServiceLoader, resource lookups) is
invisible to it, so keep
+ the default build for a release and in CI, and use this profile for
the local edit loop.
+ -->
+ <profile>
+ <id>fast-fe</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>exec-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>mark-incremental-sources</id>
+ <phase>process-sources</phase>
+ <goals>
+ <goal>exec</goal>
+ </goals>
+ <configuration>
+
<executable>${java.home}/bin/java</executable>
+ <classpathScope>runtime</classpathScope>
+ <arguments>
+ <argument>-cp</argument>
+ <classpath/>
+
<argument>org.apache.doris.buildtools.IncrementalSourceMarker</argument>
+
<argument>--sources=${project.basedir}/src/main/java,${project.build.directory}/generated-sources</argument>
Review Comment:
[P1] Invalidate fe-core when an upstream dependency changes
Only fe-core main/generated sources enter this index, while the new
`useIncrementalCompilation=false` also skips Maven's dependency-change check.
For the supported local workflow with the build cache disabled, run `mvn
-Dmaven.build.cache.enabled=false -Pfast-fe -pl fe-core -am compile`, then edit
`fe-catalog`'s `Column.SHADOW_NAME_PREFIX` and repeat the command: the marker
sees no changed input, so consumers such as `SchemaChangeHandler` retain the
old inlined constant. Upstream API changes can similarly leave incompatible
bytecode. This differs from the existing propagation thread: the changed type
is outside the indexed roots entirely. Track the effective dependency
outputs/classpath and force the necessary recompilation when they change.
##########
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()) {
Review Comment:
[P2] Remove obsolete classes from sources that still exist
Cleanup runs only for missing source paths, and `index.put` has already
overwritten the previous declarations of edited sources. With `-Pfast-fe
-Dmaven.build.cache.enabled=false`, compile `A.java` containing `public class A
{} class Extra {}` and another source using `Extra`, then remove only `Extra`
while keeping `A.java`: the referrer is touched, but `Extra.class` survives in
`target/classes`, so javac can still resolve the deleted type and accept an
invalid source set. The new compiler mode skips Maven's usual previous-output
cleanup. Unlike the existing deleted-source thread, this case never enters
`removeClasses`. Preserve previous output ownership and clean obsolete types
from changed sources too.
--
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]
