-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[MNG-7899] Various memory usage improvements 4-1 #1296
Conversation
- BatchModeMavenTransferListener removed - FileSizeFormat moved to top level class - FileSizeFormat.formatProcess() refactored to accept an StringBuilder as argument - FileSizeFormat refactored format() methods
d070939
to
4e61ebc
Compare
double scaledSize = (double) size / unit.bytes(); | ||
|
||
if (unit == ScaleUnit.BYTE) { | ||
return largeFormat.format(size); |
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.
The goal is to avoid those calls.
NumberFormat
has methods that take a StringBuilder
...
In NumberFormat
, I have the following declaration:
public final String format(long number) {
return format(number, new StringBuffer(),
DontCareFieldPosition.INSTANCE).toString();
}
so we should go straight to the method that takes a StringBuffer
to avoid unneeded allocations + toString....
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.
Would have been nice, but the NumberFormat.format() method takes only StringBuffer as parameter, the synchronized StringBuilder. They are not interchangeable and I am not sure we should switch StringBuilder for StringBuffer for that....
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.
What about new Formatter(locale, builder).format("%3.f", size)
? i.e. do not use the NumberFormat
, but Formatter
?
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.
Using a Formatter, the pattern string is passed as parameter to the format() method. Doing this the pattern is recompiled each method call and not reused. This generates a lot of garbage. A more optimised solution would be to avoid using generic Format or Formatter classes and implement a custom logic specific to this particular case. Some math logic to round the numbers as needed. This would reduce even more the memory allocation.
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.
@gnodet you can find the refactored FileSizeFormat
class using Math logic in the last commit
- FileSizeFormat refactored, replace DecimalFormat with Math logic to reduce memory allocation
@gnodet your changes are cleaner. I agree with your contribution, thanks! |
99c6dae
to
ebed0da
Compare
@gnodet I just fixed a format problem that made the build fail. |
BatchModeMavenTransferListener
removedFileSizeFormat
moved to top level classFileSizeFormat.formatProcess()
refactored to accept an StringBuilderas argument
FileSizeFormat
refactoredformat()
methodsTo make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
[ X] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
[ X] In any other case, please file an Apache Individual Contributor License Agreement.