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.

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

Conversation

saraadams
Copy link
Collaborator

Instead of reading the Bazel profile all at once, use a stream to reduce memory consumption.

Benchmark with a Bazel profile of unzipped size 357MB
Using YourKit as a profiler and checking the shallow size at the end of the BazelProfile constructor:

Before: 3.6 GB
After: 349 MB

Contributes to #163

Instead of reading the Bazel profile all at once, use a stream
to reduce memory consumption.

Benchmark with a Bazel profile of unzipped size 357MB
Using YourKit as a profiler and checking the shallow size at the end
of the `BazelProfile` constructor:

Before: 3.6 GB
After: 349 MB

Contributes to #163

Signed-off-by: Sara Adams <[email protected]>
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.

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.

@saraadams saraadams merged commit ef5c4d5 into main Dec 21, 2023
3 checks passed
@saraadams saraadams deleted the sara-read-bazel-profile-streamed branch December 21, 2023 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants