Skip to content

Commit 9a4c671

Browse files
committed
Reduce complexity of CriticalPathQueuingDurationDataProvider
1 parent 3fa9602 commit 9a4c671

4 files changed

Lines changed: 105 additions & 82 deletions

File tree

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,9 @@
3535
import java.io.IOException;
3636
import java.io.InputStream;
3737
import java.io.InputStreamReader;
38+
import java.io.Reader;
3839
import java.nio.charset.StandardCharsets;
40+
import java.util.Collection;
3941
import java.util.HashMap;
4042
import java.util.List;
4143
import java.util.Map;
@@ -82,6 +84,10 @@ public static BazelProfile createFromInputStream(InputStream inputStream)
8284
new JsonReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)));
8385
}
8486

87+
public static BazelProfile of(Reader reader) {
88+
return new BazelProfile(new JsonReader(reader));
89+
}
90+
8591
private final BazelVersion bazelVersion;
8692
private final Map<String, String> otherData = new HashMap<>();
8793
private final Map<ThreadId, ProfileThread> threads = new HashMap<>();
@@ -227,6 +233,10 @@ public Stream<ProfileThread> getThreads() {
227233
return threads.values().stream();
228234
}
229235

236+
public Collection<ProfileThread> getThreadList() {
237+
return threads.values();
238+
}
239+
230240
public Optional<ProfileThread> getCriticalPath() {
231241
return threads.values().stream().filter(BazelProfile::isCriticalPathThread).findAny();
232242
}

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ public class ProfileThread {
4040

4141
@Nullable private String name;
4242
@Nullable private Integer sortIndex;
43+
private boolean sorted;
4344

4445
private final List<JsonObject> extraMetadata;
4546
private final List<JsonObject> extraEvents;
@@ -193,8 +194,13 @@ public boolean addEvent(JsonObject event) {
193194
}
194195

195196
public List<CompleteEvent> getCompleteEvents() {
196-
completeEvents.sort(Comparator.comparing((e) -> e.start));
197-
return ImmutableList.copyOf(completeEvents);
197+
synchronized (this) {
198+
if (!sorted) {
199+
completeEvents.sort(Comparator.comparing((e) -> e.start));
200+
sorted = true;
201+
}
202+
}
203+
return completeEvents;
198204
}
199205

200206
public ImmutableMap<String, ImmutableList<CounterEvent>> getCounts() {

analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ java_library(
2424
"//analyzer/java/com/engflow/bazel/invocation/analyzer/time",
2525
"//analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat",
2626
"//third_party/guava",
27+
"//third_party/maven/com.google.code.findbugs/jsr305",
2728
],
2829
)
2930

analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution/CriticalPathQueuingDurationDataProvider.java

Lines changed: 86 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import java.util.Set;
3333
import java.util.regex.Matcher;
3434
import java.util.regex.Pattern;
35+
import javax.annotation.Nullable;
3536

3637
/**
3738
* A {@link DataProvider} that supplies the duration spent queuing for remote execution within the
@@ -64,85 +65,90 @@ CriticalPathQueuingDuration getCriticalPathQueuingDuration()
6465
if (bazelProfile.getCriticalPath().isEmpty()) {
6566
return new CriticalPathQueuingDuration(EMPTY_REASON);
6667
}
67-
bazelProfile
68-
.getCriticalPath()
69-
.get()
70-
.getCompleteEvents()
71-
.forEach(
72-
(criticalPathEvent) -> {
73-
Matcher m = CRITICAL_PATH_TO_EVENT_NAME.matcher(criticalPathEvent.name);
74-
if (m.matches()) {
75-
String eventNameToFind = m.group(1);
76-
bazelProfile
77-
.getThreads()
78-
.flatMap((thread) -> thread.getCompleteEvents().stream())
79-
// Name should match, and event interval should be contained in
80-
// criticalPathEvent interval.
81-
.filter(
82-
(event) ->
83-
BazelProfileConstants.CAT_ACTION_PROCESSING.equals(event.category)
84-
&& eventNameToFind.equals(event.name)
85-
// If "action processing" is the first event, the timestamp
86-
// may be slightly out of sync with the critical path event.
87-
&& (criticalPathEvent.start.almostEquals(event.start)
88-
||
89-
// It may not be the first event, e.g.
90-
// "action dependency checking" may be reported before
91-
criticalPathEvent.start.compareTo(event.start) > 0)
92-
// Keep this always-true-condition for documentation purposes!
93-
// We have found cases where the end time of the critical path
94-
// event is less than the end time of the processing event.
95-
// This might be a bug / inconsistency in Bazel profile writing.
96-
&& (true
97-
|| criticalPathEvent.end.almostEquals(event.end)
98-
|| criticalPathEvent.end.compareTo(event.end) > 0))
99-
// We expect to find just one event, but this may not be true for more
100-
// generic action names. Sort all thus far matching events to find the best
101-
// match.
102-
.sorted(
103-
(a, b) -> {
104-
boolean aWithinBounds =
105-
criticalPathEvent.end.almostEquals(a.end)
106-
|| criticalPathEvent.end.compareTo(a.end) > 0;
107-
boolean bWithinBounds =
108-
criticalPathEvent.end.almostEquals(b.end)
109-
|| criticalPathEvent.end.compareTo(b.end) > 0;
110-
if (aWithinBounds && bWithinBounds) {
111-
// Both events within bounds, prefer the longer one.
112-
return b.duration.compareTo(a.duration);
113-
}
114-
// If one of the events is within the bounds, prefer it.
115-
if (aWithinBounds) {
116-
return -1;
117-
}
118-
if (bWithinBounds) {
119-
return 1;
120-
}
121-
// Neither event within bounds, prefer the one that extends the bounds
122-
// least.
123-
return a.end.compareTo(b.end);
124-
})
125-
.limit(1)
126-
.forEach(
127-
e -> {
128-
// As we could not check the end boundary above, adjust the duration here,
129-
// so that we can ensure queuing events do not exceed the boundaries of
130-
// the critical path entry.
131-
Timestamp end =
132-
Timestamp.ofMicros(
133-
Math.min(e.end.getMicros(), criticalPathEvent.end.getMicros()));
134-
criticalPathEventsInThreads.add(
135-
new CompleteEvent(
136-
e.name,
137-
e.category,
138-
e.start,
139-
TimeUtil.getDurationBetween(e.start, end),
140-
e.threadId,
141-
e.processId,
142-
e.args));
143-
});
144-
}
145-
});
68+
for (var cPathEvent : bazelProfile.getCriticalPath().get().getCompleteEvents()) {
69+
Matcher m = CRITICAL_PATH_TO_EVENT_NAME.matcher(cPathEvent.name);
70+
if (!m.matches()) {
71+
continue;
72+
}
73+
var eventNameToFind = m.group(1);
74+
@Nullable CompleteEvent found = null;
75+
var foundWithinBounds = false;
76+
for (var thread : bazelProfile.getThreadList()) {
77+
for (var event : thread.getCompleteEvents()) {
78+
// Name should match, and event interval should be contained in cPathEvent interval.
79+
if (!BazelProfileConstants.CAT_ACTION_PROCESSING.equals(event.category)) {
80+
continue;
81+
}
82+
if (!eventNameToFind.equals(event.name)) {
83+
continue;
84+
}
85+
// If "action processing" is the first event, the timestamp
86+
// may be slightly out of sync with the critical path event.
87+
//
88+
// It may not be the first event, e.g.
89+
// "action dependency checking" may be reported before
90+
if (!cPathEvent.start.almostEquals(event.start)
91+
&& cPathEvent.start.compareTo(event.start) <= 0) {
92+
continue;
93+
}
94+
// We have found cases where the end time of the critical path event is less than the end
95+
// time of the processing event. This might be a bug / inconsistency in Bazel profile
96+
// writing.
97+
if (!cPathEvent.end.almostEquals(event.end) && cPathEvent.end.compareTo(event.end) <= 0) {
98+
continue;
99+
}
100+
101+
if (found == null) {
102+
found = event;
103+
foundWithinBounds =
104+
cPathEvent.end.almostEquals(found.end) || cPathEvent.end.compareTo(found.end) > 0;
105+
continue;
106+
}
107+
108+
// We expect to find just one event, but this may not be true for more generic action
109+
// names. Sort all thus far matching events to find the best match.
110+
var eventWithinBounds =
111+
cPathEvent.end.almostEquals(event.end) || cPathEvent.end.compareTo(event.end) > 0;
112+
113+
if (foundWithinBounds && eventWithinBounds) {
114+
// Both events within bounds, prefer the longer one.
115+
if (event.duration.compareTo(found.duration) > 0) {
116+
found = event;
117+
foundWithinBounds = true;
118+
}
119+
continue;
120+
}
121+
// If one of the events is within the bounds, prefer it.
122+
if (eventWithinBounds) {
123+
found = event;
124+
foundWithinBounds = true;
125+
continue;
126+
}
127+
// Neither event within bounds, prefer the one that extends the bounds
128+
// least.
129+
if (found.end.compareTo(event.end) < 0) {
130+
found = event;
131+
foundWithinBounds = false;
132+
}
133+
}
134+
}
135+
if (found != null) {
136+
// As we could not check the end boundary above, adjust the duration here,
137+
// so that we can ensure queuing events do not exceed the boundaries of
138+
// the critical path entry.
139+
Timestamp end =
140+
Timestamp.ofMicros(Math.min(found.end.getMicros(), cPathEvent.end.getMicros()));
141+
criticalPathEventsInThreads.add(
142+
new CompleteEvent(
143+
found.name,
144+
found.category,
145+
found.start,
146+
TimeUtil.getDurationBetween(found.start, end),
147+
found.threadId,
148+
found.processId,
149+
found.args));
150+
}
151+
}
146152
Duration duration =
147153
bazelProfile
148154
.getThreads()
@@ -161,7 +167,7 @@ CriticalPathQueuingDuration getCriticalPathQueuingDuration()
161167
&& cpEvent.processId == event.processId
162168
&& (cpEvent.start.compareTo(event.start) <= 0)
163169
&& (event.end.almostEquals(cpEvent.end)
164-
|| (event.end.compareTo(cpEvent.end) <= 0))))
170+
|| (event.end.compareTo(cpEvent.end) <= 0))))
165171
.map((event) -> event.duration)
166172
.reduce(Duration.ZERO, Duration::plus);
167173
return new CriticalPathQueuingDuration(duration);

0 commit comments

Comments
 (0)