From cdb011deff8cd3bb72bf64d9b21890cce27db6c8 Mon Sep 17 00:00:00 2001 From: Felipe Date: Fri, 3 Oct 2025 09:15:06 -0500 Subject: [PATCH 01/10] Reduce complexity of CriticalPathQueuingDurationDataProvider Signed-off-by: Felipe --- .../analyzer/bazelprofile/BazelProfile.java | 5 + .../analyzer/bazelprofile/ProfileThread.java | 10 +- .../dataproviders/remoteexecution/BUILD | 1 + ...iticalPathQueuingDurationDataProvider.java | 264 +++++++++++------- 4 files changed, 179 insertions(+), 101 deletions(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java index 182dd58..b0dec8d 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java @@ -35,6 +35,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.io.Reader; import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.List; @@ -82,6 +83,10 @@ public static BazelProfile createFromInputStream(InputStream inputStream) new JsonReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))); } + public static BazelProfile of(Reader reader) { + return new BazelProfile(new JsonReader(reader)); + } + private final BazelVersion bazelVersion; private final Map otherData = new HashMap<>(); private final Map threads = new HashMap<>(); diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java index 6e46704..635536c 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java @@ -40,6 +40,7 @@ public class ProfileThread { @Nullable private String name; @Nullable private Integer sortIndex; + private boolean sorted; private final List extraMetadata; private final List extraEvents; @@ -193,8 +194,13 @@ public boolean addEvent(JsonObject event) { } public List getCompleteEvents() { - completeEvents.sort(Comparator.comparing((e) -> e.start)); - return ImmutableList.copyOf(completeEvents); + synchronized (this) { + if (!sorted) { + completeEvents.sort(Comparator.comparing((e) -> e.start)); + sorted = true; + } + } + return completeEvents; } public ImmutableMap> getCounts() { diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/BUILD b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/BUILD index 3740608..915cc1f 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/BUILD +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/BUILD @@ -24,6 +24,7 @@ java_library( "//analyzer/java/com/engflow/bazel/invocation/analyzer/time", "//analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat", "//third_party/guava", + "//third_party/jsr305", ], ) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java index 7d38f5b..a4235d8 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java @@ -26,12 +26,14 @@ import com.engflow.bazel.invocation.analyzer.time.Timestamp; import com.engflow.bazel.invocation.analyzer.traceeventformat.CompleteEvent; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; +import com.google.common.collect.HashMultimap; import java.time.Duration; -import java.util.HashSet; +import java.util.HashMap; import java.util.List; -import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.annotation.Nullable; /** * A {@link DataProvider} that supplies the duration spent queuing for remote execution within the @@ -60,110 +62,174 @@ CriticalPathQueuingDuration getCriticalPathQueuingDuration() // the relevant name, and further filtering by time interval. // Given the matching event, find a queuing event with the same tid and pid that fits // within the time interval. - Set criticalPathEventsInThreads = new HashSet<>(); + HashMultimap criticalPathEventsInThreads = HashMultimap.create(); if (bazelProfile.getCriticalPath().isEmpty()) { return new CriticalPathQueuingDuration(EMPTY_REASON); } + /* + * Key: critical action path event name. + * Value: the same critical action path, but in event form. + * Used to efficiently look up critical path events by their parsed name. + */ + HashMap cPathActionNameToSelfEvent = new HashMap<>(); + /* + * Key: critical action path event name. + * Values: all profile 'action processing' events that matched by name with the critical path + * counterpart. + * Used to keep track, for each critical path event, all the candidates that could be used to + * find its real process and thread IDs. + */ + HashMultimap cPathActionNameToEventCandidates = HashMultimap.create(); + /* + * Key: PidTidKey (process id and thread id in record form) of the remote queuing event + * Value: The remote queuing event + * This is used to efficiently match between a critical path event with the remote queuing + * event, by the process and thread IDs. + */ + HashMultimap remoteQueuingEvents = HashMultimap.create(); + + for (var cPathEvent : bazelProfile.getCriticalPath().get().getCompleteEvents()) { + if (Strings.isNullOrEmpty(cPathEvent.name)) { + continue; + } + Matcher m = CRITICAL_PATH_TO_EVENT_NAME.matcher(cPathEvent.name); + if (!m.matches()) { + continue; + } + var eventNameToFind = m.group(1); + cPathActionNameToSelfEvent.put(eventNameToFind, cPathEvent); + } + + // This loop is extremely expensive. Make sure we only perform it once! bazelProfile - .getCriticalPath() - .get() - .getCompleteEvents() + .getThreads() .forEach( - (criticalPathEvent) -> { - Matcher m = CRITICAL_PATH_TO_EVENT_NAME.matcher(criticalPathEvent.name); - if (m.matches()) { - String eventNameToFind = m.group(1); - bazelProfile - .getThreads() - .flatMap((thread) -> thread.getCompleteEvents().stream()) - // Name should match, and event interval should be contained in - // criticalPathEvent interval. - .filter( - (event) -> - BazelProfileConstants.CAT_ACTION_PROCESSING.equals(event.category) - && eventNameToFind.equals(event.name) - // If "action processing" is the first event, the timestamp - // may be slightly out of sync with the critical path event. - && (criticalPathEvent.start.almostEquals(event.start) - || - // It may not be the first event, e.g. - // "action dependency checking" may be reported before - criticalPathEvent.start.compareTo(event.start) > 0) - // Keep this always-true-condition for documentation purposes! - // We have found cases where the end time of the critical path - // event is less than the end time of the processing event. - // This might be a bug / inconsistency in Bazel profile writing. - && (true - || criticalPathEvent.end.almostEquals(event.end) - || criticalPathEvent.end.compareTo(event.end) > 0)) - // We expect to find just one event, but this may not be true for more - // generic action names. Sort all thus far matching events to find the best - // match. - .sorted( - (a, b) -> { - boolean aWithinBounds = - criticalPathEvent.end.almostEquals(a.end) - || criticalPathEvent.end.compareTo(a.end) > 0; - boolean bWithinBounds = - criticalPathEvent.end.almostEquals(b.end) - || criticalPathEvent.end.compareTo(b.end) > 0; - if (aWithinBounds && bWithinBounds) { - // Both events within bounds, prefer the longer one. - return b.duration.compareTo(a.duration); - } - // If one of the events is within the bounds, prefer it. - if (aWithinBounds) { - return -1; - } - if (bWithinBounds) { - return 1; - } - // Neither event within bounds, prefer the one that extends the bounds - // least. - return a.end.compareTo(b.end); - }) - .limit(1) - .forEach( - e -> { - // As we could not check the end boundary above, adjust the duration here, - // so that we can ensure queuing events do not exceed the boundaries of - // the critical path entry. - Timestamp end = - Timestamp.ofMicros( - Math.min(e.end.getMicros(), criticalPathEvent.end.getMicros())); - criticalPathEventsInThreads.add( - new CompleteEvent( - e.name, - e.category, - e.start, - TimeUtil.getDurationBetween(e.start, end), - e.threadId, - e.processId, - e.args)); - }); + thread -> { + for (var event : thread.getCompleteEvents()) { + if (Strings.isNullOrEmpty(event.category)) { + continue; + } + switch (event.category) { + case BazelProfileConstants.CAT_ACTION_PROCESSING -> { + var cPathEvent = cPathActionNameToSelfEvent.get(event.name); + if (cPathEvent == null) { + continue; + } + // Found an event that matches a critical path event by name. Add it to the + // critical + // path event's list of candidates for later. + cPathActionNameToEventCandidates.put(event.name, event); + } + case BazelProfileConstants.CAT_REMOTE_EXECUTION_QUEUING_TIME -> { + // We'll need to iterate through these later to sum the queuing time. Keep track + // of this + // to avoid having to iterate through the full profile again. + remoteQueuingEvents.put(new PidTidKey(event.processId, event.threadId), event); + } + default -> {} + } } }); - Duration duration = - bazelProfile - .getThreads() - .flatMap((thread) -> thread.getCompleteEvents().stream()) - // Restrict to queuing events. - .filter( - (event) -> - BazelProfileConstants.CAT_REMOTE_EXECUTION_QUEUING_TIME.equals(event.category)) - // Restrict to events that are contained in one of the critical path events. - .filter( - (event) -> - criticalPathEventsInThreads.stream() - .anyMatch( - (cpEvent) -> - cpEvent.threadId == event.threadId - && cpEvent.processId == event.processId - && (cpEvent.start.compareTo(event.start) <= 0) - && (event.end.almostEquals(cpEvent.end) - || (event.end.compareTo(cpEvent.end) <= 0)))) - .map((event) -> event.duration) - .reduce(Duration.ZERO, Duration::plus); + + // For each critical path event, loop through its list of candidates that matched by name. + // This is used to get the correct thread ID and process ID that will later be matched up to + // the remote queuing events, to calculate critical path queuing. + for (var cPathEventName : cPathActionNameToEventCandidates.keySet()) { + var cPathEvent = cPathActionNameToSelfEvent.get(cPathEventName); + if (cPathEvent == null) { + continue; + } + + @Nullable CompleteEvent found = null; + var foundWithinBounds = false; + for (CompleteEvent event : cPathActionNameToEventCandidates.get(cPathEventName)) { + // If "action processing" is the first event, the timestamp + // may be slightly out of sync with the critical path event. + // + // It may not be the first event, e.g. + // "action dependency checking" may be reported before + if (!cPathEvent.start.almostEquals(event.start) + && cPathEvent.start.compareTo(event.start) <= 0) { + continue; + } + // We have found cases where the end time of the critical path event is less than the end + // time of the processing event. This might be a bug / inconsistency in Bazel profile + // writing. + if (!cPathEvent.end.almostEquals(event.end) && cPathEvent.end.compareTo(event.end) <= 0) { + continue; + } + + if (found == null) { + found = event; + foundWithinBounds = + cPathEvent.end.almostEquals(found.end) || cPathEvent.end.compareTo(found.end) > 0; + continue; + } + + // We expect to find just one event, but this may not be true for more generic action + // names. Sort all thus far matching events to find the best match. + var eventWithinBounds = + cPathEvent.end.almostEquals(event.end) || cPathEvent.end.compareTo(event.end) > 0; + + if (foundWithinBounds && eventWithinBounds) { + // Both events within bounds, prefer the longer one. + if (event.duration.compareTo(found.duration) > 0) { + found = event; + foundWithinBounds = true; + } + continue; + } + // If one of the events is within the bounds, prefer it. + if (eventWithinBounds) { + found = event; + foundWithinBounds = true; + continue; + } + // Neither event within bounds, prefer the one that extends the bounds + // least. + if (found.end.compareTo(event.end) < 0) { + found = event; + foundWithinBounds = false; + } + } + if (found != null) { + // As we could not check the end boundary above, adjust the duration here, + // so that we can ensure queuing events do not exceed the boundaries of + // the critical path entry. + Timestamp end = + Timestamp.ofMicros(Math.min(found.end.getMicros(), cPathEvent.end.getMicros())); + criticalPathEventsInThreads.put( + new PidTidKey(found.processId, found.threadId), + new CompleteEvent( + found.name, + found.category, + found.start, + TimeUtil.getDurationBetween(found.start, end), + found.threadId, + found.processId, + found.args)); + } + } + + Duration duration = Duration.ZERO; + for (var remoteQueuingEventEntry : remoteQueuingEvents.entries()) { + var remoteQueuingEvent = remoteQueuingEventEntry.getValue(); + boolean found = false; + for (var criticalPathEventInThread : + criticalPathEventsInThreads.get(remoteQueuingEventEntry.getKey())) { + if (criticalPathEventInThread.start.compareTo(remoteQueuingEvent.start) <= 0 + && (remoteQueuingEvent.end.almostEquals(criticalPathEventInThread.end) + || (remoteQueuingEvent.end.compareTo(criticalPathEventInThread.end) <= 0))) { + found = true; + break; + } + } + if (found) { + duration = duration.plus(remoteQueuingEvent.duration); + } + } return new CriticalPathQueuingDuration(duration); } + + private record PidTidKey(int pid, int tid) {} } From 5aac8c1c1333271d5b975370418e7be596e62ea9 Mon Sep 17 00:00:00 2001 From: Felipe Date: Fri, 3 Oct 2025 09:46:48 -0500 Subject: [PATCH 02/10] Throw an ISE on attempting to modify a bazel profile thread after being sorted Signed-off-by: Felipe --- .../analyzer/bazelprofile/ProfileThread.java | 6 ++ .../invocation/analyzer/bazelprofile/BUILD | 1 + .../bazelprofile/BazelProfileTest.java | 58 +++++++++++++++++++ 3 files changed, 65 insertions(+) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java index 635536c..42927a3 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java @@ -112,8 +112,14 @@ public Integer getSortIndex() { /** * Parses a {@link JsonObject} as a tracing event and adds it to this thread. Returns {@code true} * if parsing and adding the event was successful and {@code false} otherwise. + * + * @throws IllegalStateException if the thread has already been sorted, and as such cannot be + * modified anymore. */ public boolean addEvent(JsonObject event) { + if (sorted) { + throw new IllegalStateException("Cannot add event, Bazel profile thread has been sorted!"); + } try { switch (event.get(TraceEventFormatConstants.EVENT_PHASE).getAsString()) { case TraceEventFormatConstants.PHASE_COMPLETE: // Complete events diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BUILD b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BUILD index 7aec094..5a09004 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BUILD +++ b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BUILD @@ -14,6 +14,7 @@ java_test( "//analyzer/java/com/engflow/bazel/invocation/analyzer/time", "//analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat", "//analyzer/javatests/com/engflow/bazel/invocation/analyzer:test_base", + "//third_party/gson", "//third_party/guava", "//third_party/junit", "//third_party/truth", diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java index 6c2ccb4..7647326 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java +++ b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java @@ -40,6 +40,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; +import com.google.gson.JsonObject; import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; import java.util.stream.IntStream; @@ -349,6 +350,63 @@ public void getActionCountsMixedUsesNew() assertThat(profile.getActionCounts().get().size()).isEqualTo(201); } + @Test + public void addEventShouldThrowOnSortedProfileThread() throws Exception { + var name = "CPP"; + var want = + new ProfileThread( + new ThreadId(1, 1), + BazelProfileConstants.THREAD_CRITICAL_PATH, + 0, + ImmutableList.of(), + ImmutableList.of(), + Lists.newArrayList( + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(11), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()), + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(21), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()), + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(31), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()), + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(50), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of())), + ImmutableMap.of(), + ImmutableMap.of()); + + // Getting the completeEvents from the thread will trigger them to be sorted. + var sorted = want.getCompleteEvents(); + // At this point, any further attempts at modifying the thread should throw an ISE, as this + // would break the sorting that was previously done. + var exception = + assertThrows(IllegalStateException.class, () -> want.addEvent(new JsonObject())); + assertThat(exception) + .hasMessageThat() + .isEqualTo("Cannot add event, Bazel profile thread has been sorted!"); + } + @Test public void isMainThreadShouldReturnFalseOnUnnamedProfileThread() { ProfileThread thread = new ProfileThread(new ThreadId(0, 0)); From ae847891d349afa45acae5b523162d6d2e8964c9 Mon Sep 17 00:00:00 2001 From: Felipe Date: Fri, 3 Oct 2025 09:48:57 -0500 Subject: [PATCH 03/10] Change variable name Signed-off-by: Felipe --- .../invocation/analyzer/bazelprofile/BazelProfileTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java index 7647326..7d753f4 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java +++ b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java @@ -353,7 +353,7 @@ public void getActionCountsMixedUsesNew() @Test public void addEventShouldThrowOnSortedProfileThread() throws Exception { var name = "CPP"; - var want = + var thread = new ProfileThread( new ThreadId(1, 1), BazelProfileConstants.THREAD_CRITICAL_PATH, @@ -397,11 +397,11 @@ public void addEventShouldThrowOnSortedProfileThread() throws Exception { ImmutableMap.of()); // Getting the completeEvents from the thread will trigger them to be sorted. - var sorted = want.getCompleteEvents(); + var sorted = thread.getCompleteEvents(); // At this point, any further attempts at modifying the thread should throw an ISE, as this // would break the sorting that was previously done. var exception = - assertThrows(IllegalStateException.class, () -> want.addEvent(new JsonObject())); + assertThrows(IllegalStateException.class, () -> thread.addEvent(new JsonObject())); assertThat(exception) .hasMessageThat() .isEqualTo("Cannot add event, Bazel profile thread has been sorted!"); From 986867126a1751db72b23a88d0432f47557d59b5 Mon Sep 17 00:00:00 2001 From: Felipe Date: Fri, 3 Oct 2025 11:06:00 -0500 Subject: [PATCH 04/10] Use builder pattern to avoid re-sorting and re-copying of bazel thread data Signed-off-by: Felipe --- .../analyzer/bazelprofile/BazelProfile.java | 15 +- .../analyzer/bazelprofile/ProfileThread.java | 379 +++++++++++------- .../bazelprofile/BazelProfileTest.java | 226 ++++------- 3 files changed, 324 insertions(+), 296 deletions(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java index b0dec8d..1ff7337 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java @@ -42,6 +42,7 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.atomic.AtomicLong; +import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.zip.GZIPInputStream; import java.util.zip.ZipException; @@ -89,10 +90,11 @@ public static BazelProfile of(Reader reader) { private final BazelVersion bazelVersion; private final Map otherData = new HashMap<>(); - private final Map threads = new HashMap<>(); + private final Map threads; private BazelProfile(JsonReader profileReader) { try { + Map threadBuilders = new HashMap<>(); boolean hasOtherData = false; boolean hasTraceEvents = false; profileReader.beginObject(); @@ -121,17 +123,17 @@ private BazelProfile(JsonReader profileReader) { continue; } ThreadId threadId = new ThreadId(pid, tid); - ProfileThread profileThread = - threads.compute( + ProfileThread.Builder profileThreadBuilder = + threadBuilders.compute( threadId, (key, t) -> { if (t == null) { - t = new ProfileThread(threadId); + t = new ProfileThread.Builder().setThreadId(key); } return t; }); // TODO: Use success response to take action on errant events. - profileThread.addEvent(traceEvent); + profileThreadBuilder.addEvent(traceEvent); } profileReader.endArray(); break; @@ -148,6 +150,9 @@ private BazelProfile(JsonReader profileReader) { TraceEventFormatConstants.SECTION_OTHER_DATA, TraceEventFormatConstants.SECTION_TRACE_EVENTS)); } + threads = + threadBuilders.entrySet().stream() + .collect(Collectors.toConcurrentMap(Map.Entry::getKey, e -> e.getValue().build())); } catch (IllegalStateException | IOException e) { throw new IllegalArgumentException("Could not parse Bazel profile.", e); } diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java index 42927a3..b95849d 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java @@ -40,25 +40,12 @@ public class ProfileThread { @Nullable private String name; @Nullable private Integer sortIndex; - private boolean sorted; - - private final List extraMetadata; - private final List extraEvents; - private final List completeEvents; - private final Map> counts; - private final Map> instants; - - public ProfileThread(ThreadId threadId) { - this( - threadId, - null, - null, - new ArrayList<>(), - new ArrayList<>(), - new ArrayList<>(), - new HashMap<>(), - new HashMap<>()); - } + + private final ImmutableList extraMetadata; + private final ImmutableList extraEvents; + private final ImmutableList completeEvents; + private final ImmutableMap> counts; + private final ImmutableMap> instants; @Override public String toString() { @@ -76,23 +63,23 @@ public String toString() { threadId, name, sortIndex, extraMetadata, extraEvents, completeEvents, counts, instants); } - public ProfileThread( + private ProfileThread( ThreadId threadId, @Nullable String name, @Nullable Integer sortIndex, - @Nullable List extraMetadata, - @Nullable List extraEvents, - @Nullable List completeEvents, - @Nullable Map> counts, - @Nullable Map> instants) { + @Nullable ImmutableList extraMetadata, + @Nullable ImmutableList extraEvents, + @Nullable ImmutableList completeEvents, + @Nullable ImmutableMap> counts, + @Nullable ImmutableMap> instants) { this.threadId = Preconditions.checkNotNull(threadId); this.name = name; this.sortIndex = sortIndex; - this.extraMetadata = extraMetadata == null ? new ArrayList<>() : extraMetadata; - this.extraEvents = extraEvents == null ? new ArrayList<>() : extraEvents; - this.completeEvents = completeEvents == null ? new ArrayList<>() : completeEvents; - this.counts = counts == null ? new HashMap<>() : counts; - this.instants = instants == null ? new HashMap<>() : instants; + this.extraMetadata = extraMetadata == null ? ImmutableList.of() : extraMetadata; + this.extraEvents = extraEvents == null ? ImmutableList.of() : extraEvents; + this.completeEvents = completeEvents == null ? ImmutableList.of() : completeEvents; + this.counts = counts == null ? ImmutableMap.of() : counts; + this.instants = instants == null ? ImmutableMap.of() : instants; } public ThreadId getThreadId() { @@ -109,135 +96,20 @@ public Integer getSortIndex() { return sortIndex; } - /** - * Parses a {@link JsonObject} as a tracing event and adds it to this thread. Returns {@code true} - * if parsing and adding the event was successful and {@code false} otherwise. - * - * @throws IllegalStateException if the thread has already been sorted, and as such cannot be - * modified anymore. - */ - public boolean addEvent(JsonObject event) { - if (sorted) { - throw new IllegalStateException("Cannot add event, Bazel profile thread has been sorted!"); - } - try { - switch (event.get(TraceEventFormatConstants.EVENT_PHASE).getAsString()) { - case TraceEventFormatConstants.PHASE_COMPLETE: // Complete events - { - completeEvents.add(CompleteEvent.fromJson(event)); - break; - } - - case "I": // Deprecated, fall-through - case TraceEventFormatConstants.PHASE_INSTANT: // Instant events - { - InstantEvent instantEvent = InstantEvent.fromJson(event); - - List instantList = - instants.compute( - instantEvent.getCategory(), - (key, c) -> { - if (c == null) { - c = new ArrayList<>(); - } - return c; - }); - - instantList.add(instantEvent); - break; - } - - case TraceEventFormatConstants.PHASE_COUNTER: // Counter events - { - CounterEvent counterEvent = CounterEvent.fromJson(event); - - List countList = - counts.compute( - counterEvent.getName(), - (key, c) -> { - if (c == null) { - c = new ArrayList<>(); - } - return c; - }); - - countList.add(counterEvent); - break; - } - - case TraceEventFormatConstants.PHASE_METADATA: // Metadata events - { - String eventName = event.get(TraceEventFormatConstants.EVENT_NAME).getAsString(); - if (TraceEventFormatConstants.METADATA_THREAD_NAME.equals(eventName)) { - this.name = - event - .get(TraceEventFormatConstants.EVENT_ARGUMENTS) - .getAsJsonObject() - .get("name") - .getAsString(); - } else if (TraceEventFormatConstants.METADATA_THREAD_SORT_INDEX.equals(eventName)) { - this.sortIndex = - Integer.parseInt( - event - .get(TraceEventFormatConstants.EVENT_ARGUMENTS) - .getAsJsonObject() - .get("sort_index") - .getAsString()); - } else { - extraMetadata.add(event); - } - break; - } - - default: - extraEvents.add(event); - } - - return true; - } catch (Exception ex) { - return false; - } - } - public List getCompleteEvents() { - synchronized (this) { - if (!sorted) { - completeEvents.sort(Comparator.comparing((e) -> e.start)); - sorted = true; - } - } return completeEvents; } public ImmutableMap> getCounts() { - return ImmutableMap.copyOf( - counts.entrySet().stream() - .collect( - Collectors.toMap( - Map.Entry::getKey, - e -> { - List entries = e.getValue(); - entries.sort(Comparator.comparing(CounterEvent::getTimestamp)); - return ImmutableList.copyOf(entries); - }))); + return counts; } public ImmutableMap> getInstants() { - return ImmutableMap.copyOf( - instants.entrySet().stream() - .collect( - Collectors.toMap( - Map.Entry::getKey, - e -> { - List entries = e.getValue(); - entries.sort(Comparator.comparing(InstantEvent::getTimestamp)); - return ImmutableList.copyOf(entries); - }))); + return instants; } public ImmutableList getExtraEvents() { - extraEvents.sort(Comparator.comparingLong(e -> e.get("ts").getAsLong())); - return ImmutableList.copyOf(extraEvents); + return extraEvents; } @Override @@ -270,4 +142,215 @@ public int hashCode() { return Objects.hashCode( threadId, name, sortIndex, extraMetadata, extraEvents, completeEvents, counts, instants); } + + public static class Builder { + + private ThreadId threadId; + + @Nullable private String name; + @Nullable private Integer sortIndex; + + private List extraMetadata = new ArrayList<>(); + private List extraEvents = new ArrayList<>(); + private List completeEvents = new ArrayList<>(); + private Map> counts = new HashMap<>(); + private Map> instants = new HashMap<>(); + + /** + * Parses a {@link JsonObject} as a tracing event and adds it to this thread. Returns {@code + * true} if parsing and adding the event was successful and {@code false} otherwise. + */ + public boolean addEvent(JsonObject event) { + try { + switch (event.get(TraceEventFormatConstants.EVENT_PHASE).getAsString()) { + case TraceEventFormatConstants.PHASE_COMPLETE: // Complete events + { + completeEvents.add(CompleteEvent.fromJson(event)); + break; + } + + case "I": // Deprecated, fall-through + case TraceEventFormatConstants.PHASE_INSTANT: // Instant events + { + InstantEvent instantEvent = InstantEvent.fromJson(event); + + List instantList = + instants.compute( + instantEvent.getCategory(), + (key, c) -> { + if (c == null) { + c = new ArrayList<>(); + } + return c; + }); + + instantList.add(instantEvent); + break; + } + + case TraceEventFormatConstants.PHASE_COUNTER: // Counter events + { + CounterEvent counterEvent = CounterEvent.fromJson(event); + + List countList = + counts.compute( + counterEvent.getName(), + (key, c) -> { + if (c == null) { + c = new ArrayList<>(); + } + return c; + }); + + countList.add(counterEvent); + break; + } + + case TraceEventFormatConstants.PHASE_METADATA: // Metadata events + { + String eventName = event.get(TraceEventFormatConstants.EVENT_NAME).getAsString(); + if (TraceEventFormatConstants.METADATA_THREAD_NAME.equals(eventName)) { + this.name = + event + .get(TraceEventFormatConstants.EVENT_ARGUMENTS) + .getAsJsonObject() + .get("name") + .getAsString(); + } else if (TraceEventFormatConstants.METADATA_THREAD_SORT_INDEX.equals(eventName)) { + this.sortIndex = + Integer.parseInt( + event + .get(TraceEventFormatConstants.EVENT_ARGUMENTS) + .getAsJsonObject() + .get("sort_index") + .getAsString()); + } else { + extraMetadata.add(event); + } + break; + } + + default: + extraEvents.add(event); + } + + return true; + } catch (Exception ex) { + return false; + } + } + + public ThreadId getThreadId() { + return threadId; + } + + public Builder setThreadId(ThreadId threadId) { + this.threadId = threadId; + return this; + } + + @Nullable + public String getName() { + return name; + } + + public Builder setName(@Nullable String name) { + this.name = name; + return this; + } + + @Nullable + public Integer getSortIndex() { + return sortIndex; + } + + public Builder setSortIndex(@Nullable Integer sortIndex) { + this.sortIndex = sortIndex; + return this; + } + + public List getExtraMetadata() { + return extraMetadata; + } + + public Builder setExtraMetadata(List extraMetadata) { + this.extraMetadata = extraMetadata; + return this; + } + + public List getExtraEvents() { + return extraEvents; + } + + public Builder setExtraEvents(List extraEvents) { + this.extraEvents = extraEvents; + return this; + } + + public List getCompleteEvents() { + return completeEvents; + } + + public Builder setCompleteEvents(List completeEvents) { + this.completeEvents = completeEvents; + return this; + } + + public Map> getCounts() { + return counts; + } + + public Builder setCounts(Map> counts) { + this.counts = counts; + return this; + } + + public Map> getInstants() { + return instants; + } + + public Builder setInstants(Map> instants) { + this.instants = instants; + return this; + } + + public ProfileThread build() { + var extraEventsSorted = + ImmutableList.sortedCopyOf( + Comparator.comparingLong(e -> e.get("ts").getAsLong()), this.extraEvents); + var completeEventSorted = + ImmutableList.sortedCopyOf(Comparator.comparing((e) -> e.start), this.completeEvents); + var countsSorted = + ImmutableMap.copyOf( + counts.entrySet().stream() + .collect( + Collectors.toMap( + Map.Entry::getKey, + e -> { + List entries = e.getValue(); + entries.sort(Comparator.comparing(CounterEvent::getTimestamp)); + return ImmutableList.copyOf(entries); + }))); + var instantsSorted = + ImmutableMap.copyOf( + instants.entrySet().stream() + .collect( + Collectors.toMap( + Map.Entry::getKey, + e -> { + List entries = e.getValue(); + entries.sort(Comparator.comparing(InstantEvent::getTimestamp)); + return ImmutableList.copyOf(entries); + }))); + return new ProfileThread( + this.threadId, + this.name, + this.sortIndex, + ImmutableList.copyOf(this.extraMetadata), + extraEventsSorted, + completeEventSorted, + countsSorted, + instantsSorted); + } + } } diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java index 7d753f4..00c3ae6 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java +++ b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfileTest.java @@ -37,12 +37,10 @@ import com.engflow.bazel.invocation.analyzer.time.Timestamp; import com.engflow.bazel.invocation.analyzer.traceeventformat.CompleteEvent; import com.engflow.bazel.invocation.analyzer.traceeventformat.TraceEventFormatConstants; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Lists; -import com.google.gson.JsonObject; import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; +import java.util.List; import java.util.stream.IntStream; import java.util.stream.Stream; import org.junit.Test; @@ -143,47 +141,45 @@ public void shouldReturnDataProviderForBazelProfile() throws Exception { public void parseCriticalPath() throws Exception { var name = "CPP"; var want = - new ProfileThread( - new ThreadId(1, 1), - BazelProfileConstants.THREAD_CRITICAL_PATH, - 0, - ImmutableList.of(), - ImmutableList.of(), - Lists.newArrayList( - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(11), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of()), - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(21), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of()), - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(31), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of()), - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(50), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of())), - ImmutableMap.of(), - ImmutableMap.of()); + new ProfileThread.Builder() + .setThreadId(new ThreadId(1, 1)) + .setName(BazelProfileConstants.THREAD_CRITICAL_PATH) + .setSortIndex(0) + .setCompleteEvents( + List.of( + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(11), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()), + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(21), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()), + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(31), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()), + new CompleteEvent( + name, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(50), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()))) + .build(); var profile = useProfile( @@ -350,148 +346,92 @@ public void getActionCountsMixedUsesNew() assertThat(profile.getActionCounts().get().size()).isEqualTo(201); } - @Test - public void addEventShouldThrowOnSortedProfileThread() throws Exception { - var name = "CPP"; - var thread = - new ProfileThread( - new ThreadId(1, 1), - BazelProfileConstants.THREAD_CRITICAL_PATH, - 0, - ImmutableList.of(), - ImmutableList.of(), - Lists.newArrayList( - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(11), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of()), - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(21), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of()), - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(31), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of()), - new CompleteEvent( - name, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(50), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of())), - ImmutableMap.of(), - ImmutableMap.of()); - - // Getting the completeEvents from the thread will trigger them to be sorted. - var sorted = thread.getCompleteEvents(); - // At this point, any further attempts at modifying the thread should throw an ISE, as this - // would break the sorting that was previously done. - var exception = - assertThrows(IllegalStateException.class, () -> thread.addEvent(new JsonObject())); - assertThat(exception) - .hasMessageThat() - .isEqualTo("Cannot add event, Bazel profile thread has been sorted!"); - } - @Test public void isMainThreadShouldReturnFalseOnUnnamedProfileThread() { - ProfileThread thread = new ProfileThread(new ThreadId(0, 0)); + ProfileThread thread = new ProfileThread.Builder().setThreadId(new ThreadId(0, 0)).build(); assertThat(BazelProfile.isMainThread(thread)).isFalse(); } @Test public void isMainThreadShouldReturnFalseOnOtherProfileThread() { ProfileThread thread = - new ProfileThread( - new ThreadId(0, 0), "skyframe-evaluator-1", null, null, null, null, null, null); + new ProfileThread.Builder() + .setThreadId(new ThreadId(0, 0)) + .setName("skyframe-evaluator-1") + .build(); assertThat(BazelProfile.isMainThread(thread)).isFalse(); } @Test public void isMainThreadShouldReturnTrueOnNewName() { ProfileThread thread = - new ProfileThread(new ThreadId(0, 0), "Main Thread", null, null, null, null, null, null); + new ProfileThread.Builder().setThreadId(new ThreadId(0, 0)).setName("Main Thread").build(); assertThat(BazelProfile.isMainThread(thread)).isTrue(); } @Test public void isMainThreadShouldReturnTrueOnOldName() { ProfileThread thread = - new ProfileThread(new ThreadId(0, 0), "grpc-command-3", null, null, null, null, null, null); + new ProfileThread.Builder() + .setThreadId(new ThreadId(0, 0)) + .setName("grpc-command-3") + .build(); assertThat(BazelProfile.isMainThread(thread)).isTrue(); } @Test public void isGarbageCollectorThreadShouldReturnFalseOnEmptyProfileThread() { - ProfileThread thread = new ProfileThread(new ThreadId(0, 0)); + ProfileThread thread = new ProfileThread.Builder().setThreadId(new ThreadId(0, 0)).build(); assertThat(BazelProfile.isGarbageCollectorThread(thread)).isFalse(); } @Test public void isGarbageCollectorShouldReturnFalseOnOtherProfileThread() { ProfileThread thread = - new ProfileThread( - new ThreadId(0, 0), "skyframe-evaluator-1", null, null, null, null, null, null); + new ProfileThread.Builder() + .setThreadId(new ThreadId(0, 0)) + .setName("skyframe-evaluator-1") + .build(); assertThat(BazelProfile.isGarbageCollectorThread(thread)).isFalse(); } @Test public void isGarbageCollectorShouldReturnFalseOnGCThreadWithoutGCEvents() { ProfileThread thread = - new ProfileThread( - new ThreadId(0, 0), - "Garbage Collector", - null, - null, - null, - Lists.newArrayList( - new CompleteEvent( - null, - BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, - Timestamp.ofMicros(11), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of())), - null, - null); + new ProfileThread.Builder() + .setThreadId(new ThreadId(0, 0)) + .setName("Garbage Collector") + .setCompleteEvents( + List.of( + new CompleteEvent( + null, + BazelProfileConstants.CAT_CRITICAL_PATH_COMPONENT, + Timestamp.ofMicros(11), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()))) + .build(); assertThat(BazelProfile.isGarbageCollectorThread(thread)).isFalse(); } @Test public void isGarbageCollectorShouldReturnTrueOnThreadWithGC() { ProfileThread thread = - new ProfileThread( - new ThreadId(0, 0), - "Foo Thread", - null, - null, - null, - Lists.newArrayList( - new CompleteEvent( - null, - BazelProfileConstants.CAT_GARBAGE_COLLECTION, - Timestamp.ofMicros(11), - TimeUtil.getDurationForMicros(10), - 1, - 1, - ImmutableMap.of())), - null, - null); + new ProfileThread.Builder() + .setThreadId(new ThreadId(0, 0)) + .setName("Foo Thread") + .setCompleteEvents( + List.of( + new CompleteEvent( + null, + BazelProfileConstants.CAT_GARBAGE_COLLECTION, + Timestamp.ofMicros(11), + TimeUtil.getDurationForMicros(10), + 1, + 1, + ImmutableMap.of()))) + .build(); assertThat(BazelProfile.isGarbageCollectorThread(thread)).isTrue(); } From 89291c08c87e891d7541f6fcdbd2b185bc1582aa Mon Sep 17 00:00:00 2001 From: Felipe Date: Fri, 3 Oct 2025 11:17:36 -0500 Subject: [PATCH 05/10] Remove unnecessary concurrentMap usage (confused this with a HashMap) Signed-off-by: Felipe --- .../bazel/invocation/analyzer/bazelprofile/BazelProfile.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java index 1ff7337..1120161 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java @@ -152,7 +152,7 @@ private BazelProfile(JsonReader profileReader) { } threads = threadBuilders.entrySet().stream() - .collect(Collectors.toConcurrentMap(Map.Entry::getKey, e -> e.getValue().build())); + .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().build())); } catch (IllegalStateException | IOException e) { throw new IllegalArgumentException("Could not parse Bazel profile.", e); } From 66ff45524b38691b542812bd7f0da2267e9816af Mon Sep 17 00:00:00 2001 From: Felipe Date: Fri, 3 Oct 2025 11:24:06 -0500 Subject: [PATCH 06/10] Fix accidentally mutable (although private) fields Signed-off-by: Felipe --- .../bazel/invocation/analyzer/bazelprofile/ProfileThread.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java index b95849d..488dc3a 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java @@ -38,8 +38,8 @@ public class ProfileThread { private final ThreadId threadId; - @Nullable private String name; - @Nullable private Integer sortIndex; + @Nullable private final String name; + @Nullable private final Integer sortIndex; private final ImmutableList extraMetadata; private final ImmutableList extraEvents; From 06a4e6abd1f58089d461c49bfe7bbcc101f2fb20 Mon Sep 17 00:00:00 2001 From: Felipe Date: Thu, 9 Oct 2025 19:48:40 -0500 Subject: [PATCH 07/10] Code review fixes Signed-off-by: Felipe --- .../remoteexecution/CriticalPathQueuingDurationDataProvider.java | 1 - 1 file changed, 1 deletion(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java index a4235d8..119280f 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java @@ -175,7 +175,6 @@ CriticalPathQueuingDuration getCriticalPathQueuingDuration() // Both events within bounds, prefer the longer one. if (event.duration.compareTo(found.duration) > 0) { found = event; - foundWithinBounds = true; } continue; } From 4f6fab5a2175feaee7ee12dac41e9fe6d83980c9 Mon Sep 17 00:00:00 2001 From: Felipe Date: Thu, 9 Oct 2025 19:53:23 -0500 Subject: [PATCH 08/10] Revert to always-false-condition for documentation purposes. Signed-off-by: Felipe --- .../CriticalPathQueuingDurationDataProvider.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java index 119280f..2a8dbd8 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java @@ -152,10 +152,11 @@ CriticalPathQueuingDuration getCriticalPathQueuingDuration() && cPathEvent.start.compareTo(event.start) <= 0) { continue; } + // Keep this always-false-condition for documentation purposes! // We have found cases where the end time of the critical path event is less than the end // time of the processing event. This might be a bug / inconsistency in Bazel profile // writing. - if (!cPathEvent.end.almostEquals(event.end) && cPathEvent.end.compareTo(event.end) <= 0) { + if (false && (!cPathEvent.end.almostEquals(event.end) && cPathEvent.end.compareTo(event.end) <= 0)) { continue; } From 4acc23844af7ee0347df793b09827c32b58962a4 Mon Sep 17 00:00:00 2001 From: Felipe Date: Thu, 9 Oct 2025 22:08:59 -0500 Subject: [PATCH 09/10] Linting fixes Signed-off-by: Felipe --- .../CriticalPathQueuingDurationDataProvider.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java index 2a8dbd8..63ddc1a 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java @@ -156,7 +156,9 @@ CriticalPathQueuingDuration getCriticalPathQueuingDuration() // We have found cases where the end time of the critical path event is less than the end // time of the processing event. This might be a bug / inconsistency in Bazel profile // writing. - if (false && (!cPathEvent.end.almostEquals(event.end) && cPathEvent.end.compareTo(event.end) <= 0)) { + if (false + && (!cPathEvent.end.almostEquals(event.end) + && cPathEvent.end.compareTo(event.end) <= 0)) { continue; } From 5258cc306164518744dcc36944eb188fda64aac0 Mon Sep 17 00:00:00 2001 From: Felipe Date: Thu, 9 Oct 2025 23:02:36 -0500 Subject: [PATCH 10/10] Fix test Signed-off-by: Felipe --- .../CriticalPathQueuingDurationDataProvider.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java index 63ddc1a..8a2a47e 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java @@ -189,9 +189,8 @@ CriticalPathQueuingDuration getCriticalPathQueuingDuration() } // Neither event within bounds, prefer the one that extends the bounds // least. - if (found.end.compareTo(event.end) < 0) { + if (!foundWithinBounds && found.end.compareTo(event.end) < 0) { found = event; - foundWithinBounds = false; } } if (found != null) {