Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Performance] Read Bazel profile as a stream #165

Merged
merged 2 commits into from
Dec 21, 2023
Merged

[Performance] Read Bazel profile as a stream #165

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a47724e
[Performance] Read Bazel profile as a stream
saraadams Dec 21, 2023
1a9de97
Pass charset to InputStreamReader
saraadams Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 62 additions & 54 deletions analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelProfile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import com.google.gson.stream.JsonReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
Expand Down Expand Up @@ -77,69 +77,77 @@ public static BazelProfile createFromPath(String path) throws IllegalArgumentExc

public static BazelProfile createFromInputStream(InputStream inputStream)
throws IllegalArgumentException {
JsonObject bazelProfile;
try {
bazelProfile = JsonParser.parseReader(new InputStreamReader(inputStream)).getAsJsonObject();
} catch (IllegalStateException e) {
throw new IllegalArgumentException("Could not parse Bazel profile.", e);
}
return new BazelProfile(bazelProfile);
return new BazelProfile(new JsonReader(new InputStreamReader(inputStream)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When constructing an InputStreamReader, you should always pass in a Charset, such as StandardCharsets.UTF_8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

private final BazelVersion bazelVersion;
private final Map<String, String> otherData = new HashMap<>();
private final Map<ThreadId, ProfileThread> threads = new HashMap<>();

private BazelProfile(JsonObject profile) {
if (!profile.has(TraceEventFormatConstants.SECTION_OTHER_DATA)
|| !profile.has(TraceEventFormatConstants.SECTION_TRACE_EVENTS)) {
throw new IllegalArgumentException(
String.format(
"Invalid profile, JSON file missing \"%s\" and/or \"%s\"",
TraceEventFormatConstants.SECTION_OTHER_DATA,
TraceEventFormatConstants.SECTION_TRACE_EVENTS));
}
private BazelProfile(JsonReader profileReader) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistically, it is not recommended to do significant work in a constructor. It would be better to refactor this code and do the parsing in a static method or a builder class, and then construct the object with just the required fields. While at it, it may also make sense to use immutable maps for the fields.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not this PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

try {
profile
.get(TraceEventFormatConstants.SECTION_OTHER_DATA)
.getAsJsonObject()
.entrySet()
.forEach(entry -> otherData.put(entry.getKey(), entry.getValue().getAsString()));
this.bazelVersion =
BazelVersion.parse(otherData.get(BazelProfileConstants.OTHER_DATA_BAZEL_VERSION));

profile
.get(TraceEventFormatConstants.SECTION_TRACE_EVENTS)
.getAsJsonArray()
.forEach(
element -> {
JsonObject object = element.getAsJsonObject();
int pid;
int tid;
try {
pid = object.get(TraceEventFormatConstants.EVENT_PROCESS_ID).getAsInt();
tid = object.get(TraceEventFormatConstants.EVENT_THREAD_ID).getAsInt();
} catch (Exception e) {
// Skip events that do not have a valid pid or tid.
return;
}
ThreadId threadId = new ThreadId(pid, tid);
ProfileThread profileThread =
threads.compute(
threadId,
(key, t) -> {
if (t == null) {
t = new ProfileThread(threadId);
}
return t;
});
// TODO: Use success response to take action on errant events.
profileThread.addEvent(object);
});
} catch (IllegalStateException e) {
boolean hasOtherData = false;
boolean hasTraceEvents = false;
profileReader.beginObject();
while (profileReader.hasNext()) {
switch (profileReader.nextName()) {
case TraceEventFormatConstants.SECTION_OTHER_DATA:
hasOtherData = true;
profileReader.beginObject();
while (profileReader.hasNext()) {
otherData.put(profileReader.nextName(), profileReader.nextString());
}
profileReader.endObject();
break;
case TraceEventFormatConstants.SECTION_TRACE_EVENTS:
hasTraceEvents = true;
profileReader.beginArray();
while (profileReader.hasNext()) {
var traceEvent = JsonParser.parseReader(profileReader).getAsJsonObject();
int pid;
int tid;
try {
pid = traceEvent.get(TraceEventFormatConstants.EVENT_PROCESS_ID).getAsInt();
tid = traceEvent.get(TraceEventFormatConstants.EVENT_THREAD_ID).getAsInt();
} catch (Exception e) {
// Skip events that do not have a valid pid or tid.
continue;
}
ThreadId threadId = new ThreadId(pid, tid);
ProfileThread profileThread =
threads.compute(
threadId,
(key, t) -> {
if (t == null) {
t = new ProfileThread(threadId);
}
return t;
});
// TODO: Use success response to take action on errant events.
profileThread.addEvent(traceEvent);
}
profileReader.endArray();
break;
default:
// We only care about otherData and traceEvents.
profileReader.skipValue();
}
}
profileReader.endObject();
if (!hasOtherData || !hasTraceEvents) {
throw new IllegalArgumentException(
String.format(
"Invalid profile, JSON file missing \"%s\" and/or \"%s\"",
TraceEventFormatConstants.SECTION_OTHER_DATA,
TraceEventFormatConstants.SECTION_TRACE_EVENTS));
}
} catch (IllegalStateException | IOException e) {
throw new IllegalArgumentException("Could not parse Bazel profile.", e);
}

this.bazelVersion =
BazelVersion.parse(otherData.get(BazelProfileConstants.OTHER_DATA_BAZEL_VERSION));

if (!containsMainThread()) {
throw new IllegalArgumentException(
String.format(
Expand Down