From 136223fd0d7bc05495fa40624d7686c643ba4d7d Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Tue, 21 Feb 2023 12:44:08 -0500 Subject: [PATCH] Revert "Revert "Cleanup `RecipeScheduler` and associated logic (#2860)"" (#2870) --- .../java/org/openrewrite/RecipeRunStats.java | 43 +++- .../java/org/openrewrite/RecipeScheduler.java | 215 ++++++++++-------- .../marker/RecipesThatMadeChanges.java | 7 + ...RecipeSchedulerCheckingExpectedCycles.java | 15 +- 4 files changed, 178 insertions(+), 102 deletions(-) diff --git a/rewrite-core/src/main/java/org/openrewrite/RecipeRunStats.java b/rewrite-core/src/main/java/org/openrewrite/RecipeRunStats.java index e37fa421f89..6aa2b57a20c 100644 --- a/rewrite-core/src/main/java/org/openrewrite/RecipeRunStats.java +++ b/rewrite-core/src/main/java/org/openrewrite/RecipeRunStats.java @@ -38,11 +38,11 @@ public class RecipeRunStats { public RecipeRunStats(Recipe recipe) { this.recipe = recipe; if (recipe.getRecipeList().isEmpty()) { - this.called = new ArrayList<>(); + called = new ArrayList<>(); } else { - this.called = new ArrayList<>(recipe.getRecipeList().size()); + called = new ArrayList<>(recipe.getRecipeList().size()); for (Recipe callee : recipe.getRecipeList()) { - called.add(new RecipeRunStats(callee)); + addCalledRecipe(callee); } } } @@ -54,7 +54,17 @@ public RecipeRunStats(Recipe recipe) { @Getter List called; - AtomicInteger calls = new AtomicInteger(); + RecipeRunStats addCalledRecipe(Recipe recipe) { + RecipeRunStats stats = new RecipeRunStats(recipe); + called.add(stats); + return stats; + } + + private AtomicInteger calls = new AtomicInteger(); + + void markCall() { + calls.incrementAndGet(); + } /** * The number of times the recipe ran over all cycles. @@ -63,7 +73,7 @@ public int getCalls() { return calls.get(); } - final AtomicLong cumulative = new AtomicLong(); + private final AtomicLong cumulative = new AtomicLong(); /** * The total time spent across all executions of this recipe. @@ -72,7 +82,7 @@ public Duration getCumulative() { return Duration.ofNanos(cumulative.get()); } - final AtomicLong max = new AtomicLong(); + private final AtomicLong max = new AtomicLong(); /** * The max time spent in any one execution of this recipe. @@ -81,7 +91,16 @@ public Duration getMax() { return Duration.ofNanos(max.get()); } - AtomicLong ownGetVisitor = new AtomicLong(); + /** + * Called when the recipe being visited is completed. + */ + void recipeVisitCompleted(long startTime) { + long totalTime = System.nanoTime() - startTime; + max.compareAndSet(Math.min(max.get(), totalTime), totalTime); + cumulative.addAndGet(totalTime); + } + + private AtomicLong ownGetVisitor = new AtomicLong(); /** * The total time spent in running the visitor returned by {@link Recipe#getVisitor()} @@ -91,7 +110,11 @@ public Duration getOwnGetVisitor() { return Duration.ofNanos(ownGetVisitor.get()); } - AtomicLong ownVisit = new AtomicLong(); + void ownGetVisitorCompleted(long ownGetVisitorStartTime) { + ownGetVisitor.addAndGet(System.nanoTime() - ownGetVisitorStartTime); + } + + private AtomicLong ownVisit = new AtomicLong(); /** * The total time spent in running the visitor returned by {@link Recipe#visit(List, ExecutionContext)}()} @@ -101,6 +124,10 @@ public Duration getOwnVisit() { return Duration.ofNanos(ownVisit.get()); } + void ownVisitCompleted(long ownVisitStartTime) { + ownVisit.addAndGet(System.nanoTime() - ownVisitStartTime); + } + @Incubating(since = "7.29.0") public String printAsMermaidGantt(double scale) { StringBuilder gantt = new StringBuilder("gantt\n"); diff --git a/rewrite-core/src/main/java/org/openrewrite/RecipeScheduler.java b/rewrite-core/src/main/java/org/openrewrite/RecipeScheduler.java index 1ea19f9a34d..bd1d7c814c2 100644 --- a/rewrite-core/src/main/java/org/openrewrite/RecipeScheduler.java +++ b/rewrite-core/src/main/java/org/openrewrite/RecipeScheduler.java @@ -35,25 +35,27 @@ import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.BiFunction; +import java.util.function.UnaryOperator; +import java.util.stream.Collectors; import static java.util.Collections.*; import static java.util.Objects.requireNonNull; import static org.openrewrite.Recipe.PANIC; import static org.openrewrite.RecipeSchedulerUtils.addRecipesThatMadeChanges; import static org.openrewrite.RecipeSchedulerUtils.handleUncaughtException; -import static org.openrewrite.Tree.randomId; +/** + * The scheduler is responsible for executing a {@link Recipe} full lifecycle and + * reporting a {@link RecipeRun} result. + */ public interface RecipeScheduler { - default List mapAsync(List input, BiFunction mapFn) { + default List mapAsync(List input, UnaryOperator mapFn) { @SuppressWarnings("unchecked") CompletableFuture[] futures = new CompletableFuture[input.size()]; int k = 0; - for (int i = 0; i < input.size(); i++) { - T before = input.get(i); - final int index = i; - Callable updateTreeFn = () -> mapFn.apply(before, index); + for (T before : input) { + Callable updateTreeFn = () -> mapFn.apply(before); futures[k++] = schedule(updateTreeFn); } @@ -61,15 +63,18 @@ default List mapAsync(List input, BiFunction mapFn) { return ListUtils.map(input, (j, in) -> futures[j].join()); } - default RecipeRun scheduleRun(Recipe recipe, - List before, - ExecutionContext ctx, - int maxCycles, - int minCycles) { + default RecipeRun scheduleRun( + Recipe recipe, + List before, + ExecutionContext ctx, + int maxCycles, + int minCycles + ) { org.openrewrite.table.RecipeRunStats runStatsTable = new org.openrewrite.table.RecipeRunStats(Recipe.noop()); RecipeRunStats runStats = new RecipeRunStats(recipe); RecipeRun recipeRun = new RecipeRun(runStats, emptyList(), emptyMap()); + // Guards against recipes that run on source files with the same ID Set sourceFileIds = new HashSet<>(); before = ListUtils.map(before, sourceFile -> { if (!sourceFileIds.add(sourceFile.getId())) { @@ -98,7 +103,14 @@ default RecipeRun scheduleRun(Recipe recipe, Stack recipeStack = new Stack<>(); recipeStack.push(recipe); - after = scheduleVisit(runStats, recipeStack, acc, null, ctxWithWatch, recipeThatAddedOrDeletedSourceFile); + after = scheduleVisit( + runStats, + recipeStack, + acc, + ctxWithWatch, + null, + recipeThatAddedOrDeletedSourceFile + ); if (i + 1 >= minCycles && ((after == acc && !ctxWithWatch.hasNewMessages()) || !recipe.causesAnotherCycle())) { break; } @@ -111,11 +123,29 @@ default RecipeRun scheduleRun(Recipe recipe, return recipeRun.withDataTables(ctx.getMessage(ExecutionContext.DATA_TABLES, emptyMap())); } + List results = createAndProcessResults( + before, + after, + ctx, + recipeThatAddedOrDeletedSourceFile + ); + + runStatsTable.record(ctx, recipe, runStats); + return recipeRun + .withResults(results) + .withDataTables(ctx.getMessage(ExecutionContext.DATA_TABLES, emptyMap())); + } + + static List createAndProcessResults( + List before, + List after, + ExecutionContext ctx, + Map> recipeThatAddedOrDeletedSourceFile + ) { Map sourceFileIdentities = new HashMap<>(); for (SourceFile sourceFile : before) { sourceFileIdentities.put(sourceFile.getId(), sourceFile); } - List results = new ArrayList<>(); // added or changed files @@ -129,18 +159,20 @@ default RecipeRun scheduleRun(Recipe recipe, continue; } - results.add(new Result(original, s, s.getMarkers() - .findFirst(RecipesThatMadeChanges.class) - .orElseThrow(() -> new IllegalStateException("SourceFile changed but no recipe reported making a change")) - .getRecipes())); + results.add(new Result( + original, + s, + s.getMarkers() + .findFirst(RecipesThatMadeChanges.class) + .orElseThrow(() -> new IllegalStateException("SourceFile changed but no recipe " + + "reported making a change")) + .getRecipes() + )); } } } - Set afterIds = new HashSet<>(); - for (SourceFile sourceFile : after) { - afterIds.add(sourceFile.getId()); - } + Set afterIds = after.stream().map(SourceFile::getId).collect(Collectors.toSet()); // removed files for (SourceFile s : before) { @@ -149,6 +181,7 @@ default RecipeRun scheduleRun(Recipe recipe, } } + // Process the Result and add to the results table for (Result result : results) { SourcesFileResults resultsTable = new SourcesFileResults(Recipe.noop()); Stack recipeStack = new Stack<>(); @@ -163,28 +196,28 @@ default RecipeRun scheduleRun(Recipe recipe, result.getBefore() == null ? "" : result.getBefore().getSourcePath().toString(), result.getAfter() == null ? "" : result.getAfter().getSourcePath().toString(), recipeThatMadeChange[0] == null ? "" : recipeThatMadeChange[0].getName(), - recipeThatMadeChange[1].getName())); + recipeThatMadeChange[1].getName() + )); for (RecipeDescriptor rd : recipeThatMadeChange[1].getRecipeList()) { recipeStack.push(new RecipeDescriptor[]{recipeThatMadeChange[1], rd}); } } } - - runStatsTable.record(ctx, recipe, runStats); - return recipeRun - .withResults(results) - .withDataTables(ctx.getMessage(ExecutionContext.DATA_TABLES, emptyMap())); + return results; } - default List scheduleVisit(RecipeRunStats runStats, - Stack recipeStack, - List before, - @Nullable Map singleSourceApplicableTestResult, - ExecutionContext ctx, - Map> recipeThatAddedOrDeletedSourceFile) { - runStats.calls.incrementAndGet(); + default List scheduleVisit( + RecipeRunStats runStats, + Stack recipeStack, + List before, + ExecutionContext ctx, + @Nullable Map singleSourceApplicableTestResult, + Map> recipeThatAddedOrDeletedSourceFile + ) { + runStats.markCall(); long startTime = System.nanoTime(); Recipe recipe = recipeStack.peek(); + assert recipe == runStats.getRecipe() : "Recipe stack should always contain the recipe being run"; ctx.putCurrentRecipe(recipe); if (ctx instanceof WatchableExecutionContext) { @@ -213,8 +246,10 @@ default List scheduleVisit(RecipeRunStats runStats, } for (S s : before) { - singleSourceApplicableTestResult.put(s.getId(), - RecipeSchedulerUtils.applicableListTests(s, recipe.getSingleSourceApplicableTests(), ctx)); + singleSourceApplicableTestResult.put( + s.getId(), + RecipeSchedulerUtils.applicableListTests(s, recipe.getSingleSourceApplicableTests(), ctx) + ); } } } @@ -226,21 +261,23 @@ default List scheduleVisit(RecipeRunStats runStats, AtomicBoolean thrownErrorOnTimeout = new AtomicBoolean(false); List after; final Map singleSourceApplicableTestResultRef = singleSourceApplicableTestResult; - boolean hasSingleSourceApplicableTest = singleSourceApplicableTestResult != null - && !singleSourceApplicableTestResult.isEmpty(); + boolean hasSingleSourceApplicableTest = + singleSourceApplicableTestResult != null && + !singleSourceApplicableTestResult.isEmpty(); if (!recipe.validate(ctx).isValid()) { after = before; } else { long getVisitorStartTime = System.nanoTime(); - after = mapAsync(before, (s, index) -> { + after = mapAsync(before, s -> { Timer.Builder timer = Timer.builder("rewrite.recipe.visit").tag("recipe", recipe.getDisplayName()); Timer.Sample sample = Timer.start(); S afterFile = s; try { - if (hasSingleSourceApplicableTest && singleSourceApplicableTestResultRef.containsKey(s.getId()) - && !singleSourceApplicableTestResultRef.get(s.getId())) { + if (hasSingleSourceApplicableTest && + singleSourceApplicableTestResultRef.containsKey(s.getId()) && + !singleSourceApplicableTestResultRef.get(s.getId())) { return s; } @@ -265,7 +302,7 @@ default List scheduleVisit(RecipeRunStats runStats, //noinspection unchecked afterFile = (S) visitor.visitSourceFile(s, ctx); - if (visitor.isAcceptable(s, ctx)) { + if (afterFile != null && visitor.isAcceptable(afterFile, ctx)) { //noinspection unchecked afterFile = (S) visitor.visit(afterFile, ctx); } @@ -277,13 +314,18 @@ default List scheduleVisit(RecipeRunStats runStats, RecipeRunException vt = (RecipeRunException) t; //noinspection unchecked - afterFile = (S) new FindRecipeRunException(vt).visitNonNull(requireNonNull(afterFile), 0); + afterFile = (S) new FindRecipeRunException(vt).visitNonNull( + requireNonNull(afterFile, "afterFile is null"), + 0 + ); } else if (afterFile != null) { // The applicable test threw an exception, but it was not in a visitor. It cannot be associated to any specific line of code, // and instead we add a marker to the top of the source file to record the exception message. afterFile = Markup.error(afterFile, t); } + // Use the original source file to record the error, not the one that may have been modified by the visitor. + // This is so the error is associated with the original source file, and it's original source path. if (s != null) { errorsTable.insertRow(ctx, new SourcesFileErrors.Row( s.getSourcePath().toString(), @@ -304,7 +346,7 @@ default List scheduleVisit(RecipeRunStats runStats, } return afterFile; }); - runStats.ownGetVisitor.addAndGet(System.nanoTime() - getVisitorStartTime); + runStats.ownGetVisitorCompleted(getVisitorStartTime); } // The type of the list is widened at this point, since a source file type may be generated that isn't @@ -317,14 +359,8 @@ default List scheduleVisit(RecipeRunStats runStats, long ownVisitStartTime = System.nanoTime(); if (hasSingleSourceApplicableTest) { - boolean anyFilePassedSingleApplicableTest = false; - for (Boolean b : singleSourceApplicableTestResult.values()) { - if (b) { - anyFilePassedSingleApplicableTest = true; - break; - } - } - if (!anyFilePassedSingleApplicableTest) { + // If no files passed the single source applicable test, skip the recipe + if (singleSourceApplicableTestResult.values().stream().noneMatch(b -> b)) { return after; } } @@ -354,7 +390,7 @@ default List scheduleVisit(RecipeRunStats runStats, }); } - runStats.ownVisit.addAndGet(System.nanoTime() - ownVisitStartTime); + runStats.ownVisitCompleted(ownVisitStartTime); } catch (Throwable t) { return handleUncaughtException(recipeStack, recipeThatAddedOrDeletedSourceFile, before, ctx, recipe, t); } @@ -370,14 +406,10 @@ default List scheduleVisit(RecipeRunStats runStats, // a new source file generated recipeThatAddedOrDeletedSourceFile.put(s.getId(), recipeStack); } else if (s != original) { - List> recipeStackList = new ArrayList<>(1); - recipeStackList.add(recipeStack); - return s.withMarkers(s.getMarkers().computeByType( - new RecipesThatMadeChanges(randomId(), recipeStackList), - (r1, r2) -> { - r1.getRecipes().addAll(r2.getRecipes()); - return r1; - })); + return RecipeSchedulerUtils.addRecipesThatMadeChanges( + recipeStack, + s + ); } return s; }); @@ -410,21 +442,21 @@ default List scheduleVisit(RecipeRunStats runStats, // when doNext is called conditionally inside a recipe visitor if (nextStats == null) { - nextStats = new RecipeRunStats(r); - runStats.getCalled().add(nextStats); + nextStats = runStats.addCalledRecipe(r); } Map newMap = new HashMap<>(newSingleSourceApplicableTestResult); - afterWidened = scheduleVisit(requireNonNull(nextStats), - nextStack, - afterWidened, - newMap, - ctx, recipeThatAddedOrDeletedSourceFile); + afterWidened = scheduleVisit( + nextStats, + nextStack, + afterWidened, + ctx, + newMap, + recipeThatAddedOrDeletedSourceFile + ); } - long totalTime = System.nanoTime() - startTime; - runStats.max.compareAndSet(Math.min(runStats.max.get(), totalTime), totalTime); - runStats.cumulative.addAndGet(totalTime); + runStats.recipeVisitCompleted(startTime); //noinspection unchecked return (List) afterWidened; @@ -434,24 +466,26 @@ default List scheduleVisit(RecipeRunStats runStats, } class RecipeSchedulerUtils { - public static S addRecipesThatMadeChanges(Stack recipeStack, S afterFile) { - List> recipeStackList = new ArrayList<>(1); - recipeStackList.add(recipeStack); - afterFile = afterFile.withMarkers(afterFile.getMarkers().computeByType( - new RecipesThatMadeChanges(randomId(), recipeStackList), + public static S addRecipesThatMadeChanges( + Stack recipeStack, + S afterFile + ) { + return afterFile.withMarkers(afterFile.getMarkers().computeByType( + RecipesThatMadeChanges.create(recipeStack), (r1, r2) -> { r1.getRecipes().addAll(r2.getRecipes()); return r1; })); - return afterFile; } - public static List handleUncaughtException(Stack recipeStack, - Map> recipeThatAddedOrDeletedSourceFile, - List before, - ExecutionContext ctx, - Recipe recipe, - Throwable t) { + public static List handleUncaughtException( + Stack recipeStack, + Map> recipeThatAddedOrDeletedSourceFile, + List before, + ExecutionContext ctx, + Recipe recipe, + Throwable t + ) { ctx.getOnError().accept(t); ctx.putMessage(PANIC, true); @@ -485,13 +519,16 @@ public static List handleUncaughtException(Stack boolean applicableListTests(S s, - List> applicableTests, - ExecutionContext ctx) { + public static boolean applicableListTests( + S s, + List> applicableTests, + ExecutionContext ctx + ) { boolean allMatch = true; for (TreeVisitor applicableTest : applicableTests) { - boolean noChange = applicableTest.visitSourceFile(s, ctx) == s - && applicableTest.visit(s, ctx) == s; + boolean noChange = + applicableTest.visitSourceFile(s, ctx) == s + && applicableTest.visit(s, ctx) == s; if (noChange) { allMatch = false; break; diff --git a/rewrite-core/src/main/java/org/openrewrite/marker/RecipesThatMadeChanges.java b/rewrite-core/src/main/java/org/openrewrite/marker/RecipesThatMadeChanges.java index 46b53cc7dbc..eee82fd5245 100644 --- a/rewrite-core/src/main/java/org/openrewrite/marker/RecipesThatMadeChanges.java +++ b/rewrite-core/src/main/java/org/openrewrite/marker/RecipesThatMadeChanges.java @@ -20,6 +20,7 @@ import lombok.With; import org.openrewrite.Incubating; import org.openrewrite.Recipe; +import org.openrewrite.Tree; import org.openrewrite.config.RecipeDescriptor; import java.util.*; @@ -32,4 +33,10 @@ public class RecipesThatMadeChanges implements Marker { UUID id; Collection> recipes; + + public static RecipesThatMadeChanges create(Stack recipeStack) { + List> recipeStackList = new ArrayList<>(1); + recipeStackList.add(recipeStack); + return new RecipesThatMadeChanges(Tree.randomId(), recipeStackList); + } } diff --git a/rewrite-test/src/main/java/org/openrewrite/test/RecipeSchedulerCheckingExpectedCycles.java b/rewrite-test/src/main/java/org/openrewrite/test/RecipeSchedulerCheckingExpectedCycles.java index b0c4bbe12af..e08fe671273 100644 --- a/rewrite-test/src/main/java/org/openrewrite/test/RecipeSchedulerCheckingExpectedCycles.java +++ b/rewrite-test/src/main/java/org/openrewrite/test/RecipeSchedulerCheckingExpectedCycles.java @@ -43,17 +43,22 @@ public CompletableFuture schedule(Callable fn) { } @Override - public List scheduleVisit(RecipeRunStats runStats, Stack recipeStack, List before, - @Nullable Map singleSourceApplicableTestResult, ExecutionContext ctx, - Map> recipeThatAddedOrDeletedSourceFile) { + public List scheduleVisit( + RecipeRunStats runStats, + Stack recipeStack, + List before, + ExecutionContext ctx, + @Nullable Map singleSourceApplicableTestResult, + Map> recipeThatAddedOrDeletedSourceFile + ) { ctx.putMessage("cyclesThatResultedInChanges", cyclesThatResultedInChanges); - List afterList = delegate.scheduleVisit(runStats, recipeStack, before, singleSourceApplicableTestResult, ctx, recipeThatAddedOrDeletedSourceFile); + List afterList = delegate.scheduleVisit(runStats, recipeStack, before, ctx, singleSourceApplicableTestResult, recipeThatAddedOrDeletedSourceFile); if (afterList != before) { cyclesThatResultedInChanges++; if (cyclesThatResultedInChanges > expectedCyclesThatMakeChanges && !before.isEmpty() && !afterList.isEmpty()) { for (int i = 0; i < before.size(); i++) { - if(!(afterList.get(i) instanceof Quark)) { + if (!(afterList.get(i) instanceof Quark)) { assertThat(afterList.get(i).printAllTrimmed()) .as( "Expected recipe to complete in " + expectedCyclesThatMakeChanges + " cycle" + (expectedCyclesThatMakeChanges == 1 ? "" : "s") + ", " +