-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
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))); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
Signed-off-by: Sara Adams <[email protected]>
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