Reduce complexity of CriticalPathQueuingDurationDataProvider #194

afrueda97 · 2025-10-03T13:58:26Z

This provider currently performs several full loops over the Bazel profiles. This results in it not scaling well performance and memory consumption-wise.

Also at the profile level, some expensive copying and re-sorting of bazel profile data is performed each time it is requested by the providers.

Allocations:

CPU:

This PR aims to reduce the amount of loops over the data in CriticalPathQueuingDurationDataProvider to just one, enabling it to scale better.
Also, cleans the expensive copying and re-sorting of bazel profile data each time it is requested, by switching thread building to a Builder pattern, which sorts the entries internally as needed before returning the immutable sorted result.

Thanks to @iamricard for doing a lot of the initial work and investigation on this (the branch was opened in another repository so I didn't know how to cleanly credit him in the commits I moved over to here)

laszlocsomor · 2025-10-03T14:12:36Z

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java

  public List<CompleteEvent> getCompleteEvents() {
-    completeEvents.sort(Comparator.comparing((e) -> e.start));
-    return ImmutableList.copyOf(completeEvents);
+    synchronized (this) {


It looks like completeEvents is actually mutable, and mutated in ProfileThread.addEvent. So if getCompleteEvents is called, then addEvent, then getCompleteEvents again, the second call will return a possibly unsorted list.

Is it possible to make the object immutable?

If not, is there any guarantee of call order we could enforce? For example, if addEvent is never expected to be called after getCompleteEvents, then getCompleteEvents should "freeze" the object (set some flag), no longer allowing mutation.

Yeah, although addEvent is only currently called when creating the BazelProfile

bazel_invocation_analyzer/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java

Lines 133 to 134 in cdb011d

// TODO: Use success response to take action on errant events.

profileThread.addEvent(traceEvent);

It definitely wouldn't hurt to, for example, use an immutable list here to ensure we don't break the ordering afterwards

Can we make it so the code guarantees correct calling order? For example add a ProfileThread.Builder class with a mutable completeEvents that you can call addEvent on, then the final ProfileThread would have an ImmutableList as completeEvents (and it'd be sorted).

Added a check for this that throws an ISE if such modifications are attempted in 5aac8c1.

(also, a test for this)

You're right, we were also doing a lot of expensive copying and re-sorting in other places which I missed. Switching to a builder pattern cleaned all of those up, should be done in 9868671

PTAL

Signed-off-by: Felipe <afrueda97@outlook.com>

afrueda97 · 2025-10-03T14:16:09Z

Apologies for the force-push, I had accidentally pushed an old version of this work

…ng sorted Signed-off-by: Felipe <afrueda97@outlook.com>

Signed-off-by: Felipe <afrueda97@outlook.com>

…d data Signed-off-by: Felipe <afrueda97@outlook.com>

laszlocsomor · 2025-10-03T16:15:44Z

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java

+   *     modified anymore.
   */
  public boolean addEvent(JsonObject event) {
+    if (sorted) {


Are instances of ProfileThread expected to be accessed concurrently?

Since getCompleteEvents synchronizes on this to access sorted, we must do the same here. Or make sorted and AtomicBoolean.

If access to some members must be synchronized, then it's likely that we should make all member access thread-safe.

Ah, I think GitHub linked you to the previous commit, this code is gone now in favor of the builder pattern you suggested

(which, being fully immutable after built, this class should also now be safe to access concurrently)

Signed-off-by: Felipe <afrueda97@outlook.com>

afrueda97 · 2025-10-07T14:21:30Z

Friendly ping

iamricard · 2025-10-07T14:26:25Z

Sorry I'll only be able to get to this review towards the end of the week 😞

laszlocsomor

everything other than CriticalPathQueuingDurationDataProvider.java‎ looks good; I didn't review that file because I'm not sure it's related?

laszlocsomor · 2025-10-07T14:48:35Z

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java

 import java.util.stream.Collectors;
 import javax.annotation.Nullable;

 public class ProfileThread {


optional, for a follow-up: use @AutoValue and @AutoBuilder to remove some of the boilerplate (getters, equals/hashCode/toString)

laszlocsomor · 2025-10-07T15:48:06Z

...vocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java

are the changes in this file relevant to this PR?
like the new variables such as cPathActionNameToSelfEvent and related logic

They are, unfortunately GitHub does a great job of not showing the differences.

Essentially the changes are intended to:

Reduce how many loops are performed over bazelProfile.getCriticalPath().get().getCompleteEvents() and .getThreads().flatMap((thread) -> thread.getCompleteEvents().stream()). It should be happening just once for each of these lists now.

"Unroll" complicated stream-based code into simpler for loops, to make it easier to get a sense of its performance and scalability

Use more efficient data structures for lookups, which are the HashMaps and HashMultimaps at the top.

laszlocsomor

LGTM % comments

...vocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java

laszlocsomor · 2025-10-09T11:27:19Z

...vocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java

+        // 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;
+        }


This came from L96 on the left, and that check was only there for "documentation purposes".
Now we'll actually do the check. Is that intentional?

You're right. I've reverted this to an equivalent of how it originally was (always-false check now).

...vocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java

Signed-off-by: Felipe <afrueda97@outlook.com>

afrueda97 requested a review from iamricard October 3, 2025 13:58

afrueda97 requested review from WillEngFlow, laszlocsomor and saraadams as code owners October 3, 2025 13:58

afrueda97 force-pushed the felipe-reduce-complexity-queuing-provider branch from 3852557 to 3bb8c61 Compare October 3, 2025 14:04

laszlocsomor reviewed Oct 3, 2025

View reviewed changes

Reduce complexity of CriticalPathQueuingDurationDataProvider

cdb011d

Signed-off-by: Felipe <afrueda97@outlook.com>

afrueda97 force-pushed the felipe-reduce-complexity-queuing-provider branch from 3bb8c61 to cdb011d Compare October 3, 2025 14:15

afrueda97 added 3 commits October 3, 2025 09:46

Throw an ISE on attempting to modify a bazel profile thread after bei…

5aac8c1

…ng sorted Signed-off-by: Felipe <afrueda97@outlook.com>

Change variable name

ae84789

Signed-off-by: Felipe <afrueda97@outlook.com>

Use builder pattern to avoid re-sorting and re-copying of bazel threa…

9868671

…d data Signed-off-by: Felipe <afrueda97@outlook.com>

afrueda97 requested a review from laszlocsomor October 3, 2025 16:07

laszlocsomor reviewed Oct 3, 2025

View reviewed changes

afrueda97 added 2 commits October 3, 2025 11:17

Remove unnecessary concurrentMap usage (confused this with a HashMap)

89291c0

Signed-off-by: Felipe <afrueda97@outlook.com>

Fix accidentally mutable (although private) fields

66ff455

Signed-off-by: Felipe <afrueda97@outlook.com>

laszlocsomor reviewed Oct 7, 2025

View reviewed changes

laszlocsomor approved these changes Oct 9, 2025

View reviewed changes

afrueda97 force-pushed the felipe-reduce-complexity-queuing-provider branch from 85bbc81 to 66ff455 Compare October 10, 2025 00:44

afrueda97 added 2 commits October 9, 2025 19:48

Code review fixes

06a4e6a

Signed-off-by: Felipe <afrueda97@outlook.com>

Revert to always-false-condition for documentation purposes.

4f6fab5

Signed-off-by: Felipe <afrueda97@outlook.com>

afrueda97 force-pushed the felipe-reduce-complexity-queuing-provider branch from 059a6db to 4f6fab5 Compare October 10, 2025 01:24

afrueda97 added 2 commits October 9, 2025 22:08

Linting fixes

4acc238

Signed-off-by: Felipe <afrueda97@outlook.com>

Fix test

5258cc3

Signed-off-by: Felipe <afrueda97@outlook.com>

afrueda97 merged commit 5847f5b into main Oct 10, 2025
3 checks passed

afrueda97 deleted the felipe-reduce-complexity-queuing-provider branch October 10, 2025 04:13

	// TODO: Use success response to take action on errant events.
	profileThread.addEvent(traceEvent);

Reduce complexity of CriticalPathQueuingDurationDataProvider #194

Reduce complexity of CriticalPathQueuingDurationDataProvider #194

Uh oh!

Conversation

afrueda97 commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laszlocsomor Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afrueda97 commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afrueda97 Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afrueda97 commented Oct 7, 2025

Uh oh!

iamricard commented Oct 7, 2025

Uh oh!

laszlocsomor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afrueda97 Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laszlocsomor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

afrueda97 commented Oct 3, 2025 •

edited

Loading

laszlocsomor Oct 3, 2025 •

edited

Loading

afrueda97 Oct 3, 2025 •

edited

Loading

afrueda97 Oct 7, 2025 •

edited

Loading