Perf: use buffered streams for IO #307

fractalwrench · 2018-05-01T10:29:38Z

FileReader and FileWriter are not buffered, meaning they are slow. This change wraps all IO streams in a buffered reader/writer which in an (unscientific) unit test of the ErrorStore, reduced the write of an error from ~230ms to ~115ms.

This also allows us to specify the charset encoding explicitly, which is a potential (but unproven) cause for File serialisation issues on devices from certain manufacturers.

FileReader and FileWriter are not buffered, meaning they are slow. This change wraps the stream in a buffered reader/writer which in an (unscientific) unit test of the ErrorStore, reduced the write of an error from ~230ms to ~115ms. This also allows us to specify the charset encoding explicitly.

coveralls · 2018-05-01T10:45:32Z

Coverage increased (+0.5%) to 76.914% when pulling 7e57e92 on use-buffered-streams into f76d408 on next.

kattrali

Looks good from a perf standpoint - why is this a potential cause for serialization issues?

fractalwrench · 2018-05-02T08:11:14Z

@kattrali it's very much a long shot but I thought it might be possible that some manufacturers don't use UTF-8 encoding by default, which is mandated by the Android docs. This change makes the use of UTF-8 explicit. However, thinking about it, if this were the cause, we would probably see various other issues as a result.

martin308

A couple of questions about closing

martin308 · 2018-05-08T19:13:11Z

sdk/src/main/java/com/bugsnag/android/DefaultHttpClient.java

-                JsonStream stream = new JsonStream(new OutputStreamWriter(out));
+                Charset charset = Charset.forName("UTF-8");
+                BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(out, charset));
+                JsonStream stream = new JsonStream(writer);


do any of these other objects need to be closed?

martin308 · 2018-05-08T19:50:06Z

sdk/src/main/java/com/bugsnag/android/JsonStream.java

        try {
-            input = new FileReader(file);
+            FileInputStream fis = new FileInputStream(file);
+            input = new BufferedReader(new InputStreamReader(fis, "UTF-8"));


do any of these objects also need closing?

I believe calling BufferedReader#close also closes the InputStreamReader, which in turn closes the FileInputStream, which means that everything is automatically closed.

martin308 · 2018-05-08T19:50:29Z

sdk/src/main/java/com/bugsnag/android/FileStore.java

@@ -69,7 +72,8 @@ String write(@NonNull T streamable) {

        Writer out = null;
        try {
-            out = new FileWriter(filename);
+            FileOutputStream fos = new FileOutputStream(filename);
+            out = new BufferedWriter(new OutputStreamWriter(fos, "UTF-8"));


do any of these objects also need closing?

Addressed by other inline comment on JsonStream

martin308 · 2018-05-08T19:51:04Z

sdk/src/main/java/com/bugsnag/android/FileStore.java

@@ -69,7 +72,8 @@ String write(@NonNull T streamable) {

        Writer out = null;
        try {
-            out = new FileWriter(filename);
+            FileOutputStream fos = new FileOutputStream(filename);
+            out = new BufferedWriter(new OutputStreamWriter(fos, "UTF-8"));

            JsonStream stream = new JsonStream(out);
            stream.value(streamable);


not related to this PR but should we be closing this stream on the next line in the finally block?

Yes, that would make more sense than closing two separate streams. I've updated the PR so that we do this.

Close JsonStream rather than Writer, as this also closes the writer.

fractalwrench · 2018-05-09T09:55:40Z

@martin308 thanks for the feedback! I've responded to the comments inline, and have also made a slight modification so that we close the JsonStream in a finally block.

fractalwrench · 2018-05-10T07:59:12Z

Copied from #313 (addressing flaky tests) for extra context:

Thanks for the review comments. I'm going to close this PR for now as I've noticed that #307 drastically reduces the flakiness of the tests on Travis (builds 3 times without incident), and I have another hypothesis about what may have caused the flakiness.

I managed to intermittently reproduce the behaviour on an old emulator (API 16), and the log output suggests that other tests are constructing Client objects, which triggers an asynchronous flush of Files. I believe this leads to a race where the File is flushed before ConcurrentSkipListSet finishes calculating the size of Files, leading to an output of 0, where we would expect N. The performance improvements in #307 halve the time it takes to write a File, which could mean that ConcurrentSkipListSet now nearly always wins the race.

Passing output:

05-09 12:22:35.064 5313-5325/com.bugsnag.android.test I/TestRunner: started: testCancelQueuedFiles(com.bugsnag.android.ErrorStoreTest)
05-09 12:22:35.064 5313-5313/com.bugsnag.android.test I/MonitoringInstrumentation: Activities that are still in CREATED to STOPPED: 0
05-09 12:22:35.074 5313-5325/com.bugsnag.android.test I/Bugsnag: Saved unsent payload to disk (/data/data/com.bugsnag.android.test/cache/bugsnag-errors/1525868555077_d2da4d31-71ac-4fbc-a1d6-15ccdb6e07cd_startupcrash.json)
05-09 12:22:35.074 5313-5325/com.bugsnag.android.test I/TestRunner: finished: testCancelQueuedFiles(com.bugsnag.android.ErrorStoreTest)

Failing output:

05-09 12:23:27.134 5605-5617/com.bugsnag.android.test I/TestRunner: started: testCancelQueuedFiles(com.bugsnag.android.ErrorStoreTest)
05-09 12:23:27.134 5605-5605/com.bugsnag.android.test I/MonitoringInstrumentation: Activities that are still in CREATED to STOPPED: 0
05-09 12:23:27.144 5605-5622/com.bugsnag.android.test I/Bugsnag: Sending 1 saved error(s) to Bugsnag
    Deleting sent error file 1525868607149_b1034799-9497-46f1-8072-d1c4b4cdb04f_startupcrash.json
05-09 12:23:27.144 5605-5617/com.bugsnag.android.test I/Bugsnag: Saved unsent payload to disk (/data/data/com.bugsnag.android.test/cache/bugsnag-errors/1525868607149_b1034799-9497-46f1-8072-d1c4b4cdb04f_startupcrash.json)
05-09 12:23:27.144 5605-5617/com.bugsnag.android.test I/TestRunner: failed: testCancelQueuedFiles(com.bugsnag.android.ErrorStoreTest)

I think this is more of an issue with invalid assumptions made within our tests rather than anything sinister in the notifier. I'll create a ticket to investigate further which tests exactly are initialising Clients and causing this behaviour.

Cawllec · 2018-05-10T09:54:55Z

sdk/src/main/java/com/bugsnag/android/DefaultHttpClient.java

            } finally {
-                IOUtils.closeQuietly(out);
+                IOUtils.closeQuietly(stream);


Will closing the JsonStream close the OutputStream as well? There's no explicit closing of the OutputStream.

Yes - also answered in previous comments in a bit more depth.

Specifically: https://github.com/bugsnag/bugsnag-android/blob/master/sdk/src/main/java/com/bugsnag/android/JsonWriter.java#L423

Ok, is this verifiable or testable at all? It seems like something that could change across implementations/versions.

I've added a test to check that JsonWriter closes its underlying stream - beyond that we'd just be testing the behaviour of the Java standard library.

Cawllec · 2018-05-10T09:56:38Z

sdk/src/main/java/com/bugsnag/android/FileStore.java

            Logger.info(String.format("Saved unsent payload to disk (%s) ", filename));
            return filename;
        } catch (Exception exception) {
            Logger.warn(String.format("Couldn't save unsent payload to disk (%s) ",
                filename), exception);
        } finally {
-            IOUtils.closeQuietly(out);
+            IOUtils.closeQuietly(stream);


Same Q as above.

Support Unity shared object files in upload task

$fractalwrench$

$@fractalwrench$ fractalwrench requested a review from a team May 1, 2018 10:30

$@fractalwrench$ fractalwrench changed the base branch from master to next May 1, 2018 10:33

$@fractalwrench$ fractalwrench changed the base branch from next to master May 1, 2018 10:34

$@fractalwrench$

Merge branch 'master' into use-buffered-streams

986f93d

kattrali reviewed May 2, 2018

View reviewed changes

$@fractalwrench$

Merge branch 'master' into use-buffered-streams

e00ff45

$@fractalwrench$ fractalwrench changed the base branch from master to next May 2, 2018 12:45

$@fractalwrench$ fractalwrench changed the base branch from next to master May 2, 2018 12:46

$@fractalwrench$ fractalwrench changed the base branch from master to next May 2, 2018 13:17

martin308 reviewed May 8, 2018

View reviewed changes

fractalwrench added 2 commits May 9, 2018 10:35

$@fractalwrench$

refactor: close jsonstream

4969511

Close JsonStream rather than Writer, as this also closes the writer.

$@fractalwrench$

Merge branch 'next' into use-buffered-streams

8564c42

$@fractalwrench$ fractalwrench mentioned this pull request May 9, 2018

Address flaky FileStore tests #313

Closed

Cawllec reviewed May 10, 2018

View reviewed changes

$@fractalwrench$

test: add test to ensure that jsonwriter closes underlying stream

0c11014

Cawllec approved these changes May 10, 2018

View reviewed changes

$@fractalwrench$

style: pass checkstyle import checks

7e57e92

$@fractalwrench$ fractalwrench merged commit 9ebd22d into next May 10, 2018

$@fractalwrench$ fractalwrench deleted the use-buffered-streams branch May 10, 2018 12:43

lemnik pushed a commit that referenced this pull request Jun 2, 2021

$@fractalwrench$

Merge pull request #307 from bugsnag/PLAT-4966/unity-upload-task

800f85f

Support Unity shared object files in upload task

rich-bugsnag pushed a commit that referenced this pull request Sep 3, 2021

Tests: Add desktop section to testing guide (#307)

b73bd59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf: use buffered streams for IO #307

Perf: use buffered streams for IO #307

$@fractalwrench$ fractalwrench commented May 1, 2018

coveralls commented May 1, 2018 •

edited

Loading

kattrali left a comment

fractalwrench commented May 2, 2018

martin308 left a comment

martin308 May 8, 2018

martin308 May 8, 2018

$@fractalwrench$ fractalwrench May 9, 2018

martin308 May 8, 2018

$@fractalwrench$ fractalwrench May 9, 2018

martin308 May 8, 2018

$@fractalwrench$ fractalwrench May 9, 2018

fractalwrench commented May 9, 2018

fractalwrench commented May 10, 2018 •

edited

Loading

Cawllec May 10, 2018

$@fractalwrench$ fractalwrench May 10, 2018

$@fractalwrench$ fractalwrench May 10, 2018

Cawllec May 10, 2018

$@fractalwrench$ fractalwrench May 10, 2018

Cawllec May 10, 2018

Perf: use buffered streams for IO #307

Perf: use buffered streams for IO #307

Conversation

fractalwrench commented May 1, 2018

coveralls commented May 1, 2018 • edited Loading

kattrali left a comment

Choose a reason for hiding this comment

fractalwrench commented May 2, 2018

martin308 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented May 9, 2018

fractalwrench commented May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

$@fractalwrench$ fractalwrench commented May 1, 2018

coveralls commented May 1, 2018 •

edited

Loading

fractalwrench commented May 10, 2018 •

edited

Loading