-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Speed up suite html reporter #833
Speed up suite html reporter #833
Conversation
Signed-off-by: Uli Schlachter <psychon@znc.in>
Before this, the SuiteHTMLReporter would write the chronologically list of methods one method at a time. After every method, the target file was opened and the new line was written. This behavior was introduced in commit 17c5221 to fix an OOM error with many tests. This commit instead creates a BufferedWriter for the target file and writes the data directly to the target file, without re-opening it in a loop. For a random, non-representative test suite with about 2800 tests (the reason why I am looking into this), this speeds up the SuiteHTMLReporter from about 6.5 seconds to a quarter of a second. Signed-off-by: Uli Schlachter <psychon@znc.in>
The last commit removed the only user of this function. Signed-off-by: Uli Schlachter <psychon@znc.in>
} finally { | ||
try { | ||
if (bw != null) { | ||
bw.close(); |
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.
TestNG is using Java7, so you can use the try-with-resources statement.
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.
@psychon Do you think you can add a test, extracted from the patch you provided? |
@juherr What kind of test? The patch that I have in my previous comment doesn't have any behavior that can be tested for. As I said, I don't get any OutOfMemoryErrors and yet the above patch changes the test suite's peak memory usage from about 800 MiB to at least 1.5 GiB (I just looked at a system monitor for this). Also I don't know about "too long". Someone might just run this over a slow USB harddisk which makes everything slow. Sorry, I'm no good at this.... |
As the second commit explains, this speeds up a random test suite (the reason that I am working on this at all) from
to
I tested this with the following patch (note: I did not get an OOM error by just using a
StringBuffer
, as proposed in #567, so this doesn't say much; however, theStringBuffer
had a size of 42563696 = 40.6 MiB which is a lot, I guess). I watched the test suite's memory usage intop
and could not see any difference with this PR compared to master or compared to myStringBuffer
patch (always peaks over 1.5 GiB).