Fixes for SkRWBuffer

tests/DataRefTest.cpp

Index: tests/DataRefTest.cpp
diff --git a/tests/DataRefTest.cpp b/tests/DataRefTest.cpp
index 03b80fb2af4683ffe97bd8ce24e68f2340a35cf7..9acb59ababb2de0af0cf08a0f3bb83d6639c4e2e 100644
--- a/tests/DataRefTest.cpp
+++ b/tests/DataRefTest.cpp
@@ -245,7 +245,7 @@ static void check_abcs(skiatest::Reporter* reporter, const char buffer[], size_t
     }
 }
 
-// stream should contains an integral number of copies of gABC.
+// stream should contain an integral number of copies of gABC.
 static void check_alphabet_stream(skiatest::Reporter* reporter, SkStream* stream) {
     REPORTER_ASSERT(reporter, stream->hasLength());
     size_t size = stream->getLength();
@@ -284,6 +284,8 @@ static void check_alphabet_buffer(skiatest::Reporter* reporter, const SkROBuffer
     check_abcs(reporter, storage.get(), size);
 }
 
+#include "SkTaskGroup.h"
+
 DEF_TEST(RWBuffer, reporter) {
     // Knowing that the default capacity is 4096, choose N large enough so we force it to use
     // multiple buffers internally.
@@ -292,6 +294,10 @@ DEF_TEST(RWBuffer, reporter) {
     SkStream* streams[N];
 
     {
+        // Declaring this SkTaskGroup inside this block ensures that all tasks will

[mtklein, 2016/04/08 17:13:49]

You may just want to call .wait() instead of having to write out this long note?

[scroggo, 2016/04/11 16:24:42]

Done.

+        // finish before testing again, down below. (This is necessary because the
+        // streams need to be rewound to be read again.)
+        SkTaskGroup tasks;
         SkRWBuffer buffer;
         for (int i = 0; i < N; ++i) {
             if (0 == (i & 1)) {
@@ -301,12 +307,24 @@ DEF_TEST(RWBuffer, reporter) {
             }
             readers[i] = buffer.newRBufferSnapshot();
             streams[i] = buffer.newStreamSnapshot();
+            REPORTER_ASSERT(reporter, readers[i]->size() == buffer.size());
+            REPORTER_ASSERT(reporter, streams[i]->getLength() == buffer.size());
+
+            tasks.add([reporter, i, readers, streams] {

[scroggo, 2016/04/08 16:15:29]

DM will put this whole test in a task, and it will spawn its own tasks. I'm not
sure whether the subtasks will be running at the same time as we are appending
to the SkRWBuffer. If not, it defeats the point of this test, although it was
useful for me in developing (when there weren't lots of tests/gms competing for
threads). Should there be a way to run a multithreaded test in DM that is the
only thing running?

[mtklein, 2016/04/08 17:13:49]

The tasks this tests spawns can run in parallel with each other, with other
tests, with GMs, whatever.  We usually try to start tasks in stack order to help
memory locality, but it's not guaranteed, and obviously we cannot guarantee the
order that tasks finish.  So they really may be running in parallel to anything
else.

If you have a set of tasks that cannot run at the same time safely, they all
need to be marked as serial.  DM will then run them serially on the main thread.
 Other tasks will still be running around them on different threads.

Today the only way to signal that a unit test should serialize is by writing it
as GPU unit test and not passing --gpu_threading.  Do not piggy back on this
unless your code also becomes thread safe when Ganesh is.  But you can look at
DM.cpp to see how it decides to put these test in gSerialTests.

The simplest way to make sure two tests cannot have threading issues is to write
them as one test.

[scroggo, 2016/04/11 16:24:42]

I have a different issue. It is perfectly safe to run this test (and its
subtasks) in parallel with other tests. That said, this test itself is only
interesting if its main thread (which is calling SkRWBuffer::append()) runs in
parallel to its subtasks (which read from the SkROBuffer), so we test any racing
between them. If tasks might be run in stack order, that seems ideal - even if
it doesn't always happen it may help us catch any bugs that pop up when it does.

If we want to feel better about these tasks running in parallel with the main
thread, I think we want to run a task group *before* we kick off any parallel
work. gSerialTests starts after, meaning that it runs at the same time as the
parallel stuff.

+                REPORTER_ASSERT(reporter, (i + 1) * 26U == readers[i]->size());
+                REPORTER_ASSERT(reporter, streams[i]->getLength() == readers[i]->size());
+                check_alphabet_buffer(reporter, readers[i]);
+                check_alphabet_stream(reporter, streams[i]);
+                REPORTER_ASSERT(reporter, streams[i]->rewind());
+            });
         }
         REPORTER_ASSERT(reporter, N*26 == buffer.size());
     }
 
+    // Test again, to verify that the snapshots work even after the SkRWBuffer is gone.
     for (int i = 0; i < N; ++i) {
         REPORTER_ASSERT(reporter, (i + 1) * 26U == readers[i]->size());
+        REPORTER_ASSERT(reporter, streams[i]->getLength() == readers[i]->size());
         check_alphabet_buffer(reporter, readers[i]);
         check_alphabet_stream(reporter, streams[i]);
         readers[i]->unref();
@@ -329,9 +347,27 @@ DEF_TEST(RWBuffer_size, r) {
     REPORTER_ASSERT(r, 0 == iter.size());
 }
 
-// Tests that it is safe to destruct an SkRWBuffer without appending
-// anything to it.
+// Tests that operations (including the destructor) are safe on an SkRWBuffer
+// without any data appended.
 DEF_TEST(RWBuffer_noAppend, r) {
     SkRWBuffer buffer;
     REPORTER_ASSERT(r, 0 == buffer.size());
+
+    sk_sp<SkROBuffer> roBuffer = sk_sp<SkROBuffer>(buffer.newRBufferSnapshot());
+    REPORTER_ASSERT(r, roBuffer);
+    if (roBuffer) {
+        REPORTER_ASSERT(r, roBuffer->size() == 0);
+        SkROBuffer::Iter iter(roBuffer.get());
+        REPORTER_ASSERT(r, iter.size() == 0);
+        REPORTER_ASSERT(r, !iter.data());
+        REPORTER_ASSERT(r, !iter.next());
+    }
+
+    SkAutoTDelete<SkStream> stream(buffer.newStreamSnapshot());
+    REPORTER_ASSERT(r, stream);
+    if (stream) {
+        REPORTER_ASSERT(r, stream->hasLength());
+        REPORTER_ASSERT(r, stream->getLength() == 0);
+        REPORTER_ASSERT(r, stream->skip(10) == 0);
+    }
 }