Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
972213002:
[Cronet] Implement batch read in CronetInputStream (Closed)
Description[Cronet] Implement batch read in CronetInputStream
Also added a test to test that batch read works when
getMoreData() is called multiple times (i.e when the data is
large).
There are existing test cases for batch read (testInputStreamReadMoreBytesThanAvailable,
testInputStreamReadExactBytesAvailable, and
testInputStreamReadLessBytesThanAvailable)
BUG=398997
Committed: https://crrev.com/56443c070aaa3c4b1f8e0c05406ef966bf32b726
Cr-Commit-Position: refs/heads/master@{#320815}
Patch Set 1 #Patch Set 2 : Added test #
Total comments: 17
Patch Set 3 : Addressed Matt's comments #
Total comments: 4
Patch Set 4 : Updated tests #
Total comments: 2
Patch Set 5 : Added test #Patch Set 6 : Fix findbug warning #
Messages
Total messages: 25 (10 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org, pauljensen@chromium.org
Per discussion with Paul, we should implement batch read for CronetInputStream to avoid function call overhead. Hopefully this CL is more straight-forward to review compared to my other one :) PTAL. Thanks! I am gonna fix my other CL (the upload wrapper) after bug triage. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/cronet_url_request.cc:289: return ConvertUTF8ToJavaString(env, "").Release(); This might not seem very relevant, but it is needed for the test to not crash (See: crbug.com/463720 the mock data job does not give back headers). So I though I'd add the null check in this CL. I'll separate it out into a new CL, if you do not like it.
https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/cronet_url_request.cc:289: return ConvertUTF8ToJavaString(env, "").Release(); On 2015/03/04 18:10:53, xunjieli wrote: > This might not seem very relevant, but it is needed for the test to not crash > (See: crbug.com/463720 the mock data job does not give back headers). So I > though I'd add the null check in this CL. I'll separate it out into a new CL, if > you do not like it. Should we add a test just for this? https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:44: public int read(byte[] buffer, int byteOffset, int byteCount) throws IOException { Maybe: if (byteCount == 0) return 0; https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:44: public int read(byte[] buffer, int byteOffset, int byteCount) throws IOException { Should our argument names match the official spec? http://docs.oracle.com/javase/7/docs/api/java/io/InputStream.html#read(byte[]... https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:48: } Suggest turning this block into a private method, since it also appears in read(). GetMoreDataIfNeeded, or EnsureHasData, or somesuch. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:53: return mBuffer.position() - oldPosition; Is there any case where this is different from the Math.min result? If not, seems like you should just replace oldPosition with that. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:504: int numBytesToRead = (int) (Math.random() * 1000); Rather than random, can we just read 0 bytes the first time, 1 the next, 2 after that, etc? Makes for more reproducible results, though I suppose we could use a random seed or something, but that just seems more complicated than necessary. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:505: bytesRead += in.read(actualOutput, bytesRead, numBytesToRead); assertLE(bytesRead, numBytesToRead);? (Know it's not actually called LE, just not sure what it is called) https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:509: assertEquals(-1, in.read(actualOutput, 0, 123)); assertEquals(0, in.read(actualOutput, 0, 0))? Specs at least seem to indicate that should always happen, even after we're done.
Thanks for the review! https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... File components/cronet/android/cronet_url_request.cc (right): https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/cronet_url_request.cc:289: return ConvertUTF8ToJavaString(env, "").Release(); On 2015/03/05 19:24:46, mmenke wrote: > On 2015/03/04 18:10:53, xunjieli wrote: > > This might not seem very relevant, but it is needed for the test to not crash > > (See: crbug.com/463720 the mock data job does not give back headers). So I > > though I'd add the null check in this CL. I'll separate it out into a new CL, > if > > you do not like it. > > Should we add a test just for this? Talked to Matt offline. The mock job triggered this null case. Not very easy to trigger otherwise. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:44: public int read(byte[] buffer, int byteOffset, int byteCount) throws IOException { On 2015/03/05 19:24:46, mmenke wrote: > Maybe: > > if (byteCount == 0) > return 0; Done. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:44: public int read(byte[] buffer, int byteOffset, int byteCount) throws IOException { On 2015/03/05 19:24:46, mmenke wrote: > Should our argument names match the official spec? > http://docs.oracle.com/javase/7/docs/api/java/io/InputStream.html#read(byte[]... I am following the Android specs at http://developer.android.com/reference/java/io/InputStream.html https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:48: } On 2015/03/05 19:24:46, mmenke wrote: > Suggest turning this block into a private method, since it also appears in > read(). GetMoreDataIfNeeded, or EnsureHasData, or somesuch. Done. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:53: return mBuffer.position() - oldPosition; On 2015/03/05 19:24:46, mmenke wrote: > Is there any case where this is different from the Math.min result? If not, > seems like you should just replace oldPosition with that. Done. Thanks! https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:505: bytesRead += in.read(actualOutput, bytesRead, numBytesToRead); On 2015/03/05 19:24:46, mmenke wrote: > assertLE(bytesRead, numBytesToRead);? (Know it's not actually called LE, just > not sure what it is called) Hmm.. seems there's no LE or GE. So I used assertTrue. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:509: assertEquals(-1, in.read(actualOutput, 0, 123)); On 2015/03/05 19:24:46, mmenke wrote: > assertEquals(0, in.read(actualOutput, 0, 0))? Specs at least seem to indicate > that should always happen, even after we're done. Done. https://codereview.chromium.org/972213002/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:509: assertEquals(-1, in.read(actualOutput, 0, 123)); On 2015/03/05 19:24:46, mmenke wrote: > assertEquals(0, in.read(actualOutput, 0, 0))? Specs at least seem to indicate > that should always happen, even after we're done. Done.
LGTM https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:66: // Requests more data from CronetHttpURLConnection. nit: Request? (-s) Inline code comments generally use the imperative. https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:500: byte[] actualOutput = new byte[dataLength * repeatCount]; Suggest another test where we only have X body bytes, and try to use reads of Y > X (to a Y + X size buffer). The guarantees that we handle the case we try to read beyond the end of a buffer, while this tests the case we read exactly to the end of the buffer.
https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:500: byte[] actualOutput = new byte[dataLength * repeatCount]; On 2015/03/05 21:42:21, mmenke wrote: > Suggest another test where we only have X body bytes, and try to use reads of Y > > X (to a Y + X size buffer). The guarantees that we handle the case we try to > read beyond the end of a buffer, while this tests the case we read exactly to > the end of the buffer. Is this the same as testInputStreamReadMoreBytesThanAvailable (the test above this one), where we try to read more than what's in the buffer?
Still LGTM https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/972213002/diff/120001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java:500: byte[] actualOutput = new byte[dataLength * repeatCount]; On 2015/03/06 15:21:23, xunjieli wrote: > On 2015/03/05 21:42:21, mmenke wrote: > > Suggest another test where we only have X body bytes, and try to use reads of > Y > > > X (to a Y + X size buffer). The guarantees that we handle the case we try > to > > read beyond the end of a buffer, while this tests the case we read exactly to > > the end of the buffer. > > Is this the same as testInputStreamReadMoreBytesThanAvailable (the test above > this one), where we try to read more than what's in the buffer? Oops. Yea, it is.
lgtm
https://codereview.chromium.org/972213002/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/972213002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:42: throw new IndexOutOfBoundsException(); need to have tests for this condition.
Thanks for the review! I have added a test case to test the boundary condition. https://codereview.chromium.org/972213002/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/972213002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:42: throw new IndexOutOfBoundsException(); On 2015/03/11 17:53:12, mef wrote: > need to have tests for this condition. Done.
lgtm
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/972213002/#ps160001 (title: "Added test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972213002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, mef@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/972213002/#ps180001 (title: "Fix findbug warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972213002/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/56443c070aaa3c4b1f8e0c05406ef966bf32b726 Cr-Commit-Position: refs/heads/master@{#320815} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
