Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1644893003:
Avoid too small reads to bufferMoreData() (Closed)
DescriptionAvoid too small reads to bufferMoreData()
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1644893003
Committed: https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb
Committed: https://skia.googlesource.com/skia/+/22000d1f8e2c554aec2f50a5a4cad0a5d04ca41c
Patch Set 1 #
Total comments: 9
Patch Set 2 : Change the buffer size from 256 to 8192 #Patch Set 3 : Address more comments #Patch Set 4 : Update commit message #Messages
Total messages: 33 (9 generated)
Description was changed from ========== Avoid too small reads to bufferMoreData() BUG=skia: ========== to ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
yujieqin@google.com changed reviewers: + msarett@google.com, scroggo@google.com
https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:270: // Try to read at least 256 bytes to avoid to many small reads. FYI: In Snapseed we usually read 8192, same applies for the default value of the BufferedInputStream.java.
https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:270: // Try to read at least 256 bytes to avoid to many small reads. On 2016/01/28 14:20:25, adaubert wrote: > FYI: In Snapseed we usually read 8192, same applies for the default value of the > BufferedInputStream.java. I don't have preference here. msarett@ & scroggo@, which size do you prefer for Skia?
https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:270: // Try to read at least 256 bytes to avoid to many small reads. On 2016/01/28 14:27:44, yujieqin wrote: > On 2016/01/28 14:20:25, adaubert wrote: > > FYI: In Snapseed we usually read 8192, same applies for the default value of > the > > BufferedInputStream.java. > > I don't have preference here. msarett@ & scroggo@, which size do you prefer for > Skia? I guess how much we need really depends on the image... The case I mentioned in https://codereview.chromium.org/1520403003 calls bufferMoreData over 200 times to increase from 108kb to 112kb. 8192 covers this (plus some extra) and 256 seems a little small here. I would lean toward the larger size. I don't think an extra 8 kb will make much difference considering it is much smaller than the stream size. But I would defer your judgment on what is the best balance for RAW.
https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:270: // Try to read at least 256 bytes to avoid to many small reads. On 2016/01/28 16:23:44, msarett wrote: > On 2016/01/28 14:27:44, yujieqin wrote: > > On 2016/01/28 14:20:25, adaubert wrote: > > > FYI: In Snapseed we usually read 8192, same applies for the default value of > > the > > > BufferedInputStream.java. > > > > I don't have preference here. msarett@ & scroggo@, which size do you prefer > for > > Skia? > > I guess how much we need really depends on the image... The case I mentioned in > https://codereview.chromium.org/1520403003 calls bufferMoreData over 200 times > to increase from 108kb to 112kb. 8192 covers this (plus some extra) and 256 > seems a little small here. > > I would lean toward the larger size. I don't think an extra 8 kb will make much > difference considering it is much smaller than the stream size. But I would > defer your judgment on what is the best balance for RAW. As Matt suggested, the ideal number will depend on the data. Let's assume that the common case is a valid image, meaning we'll eventually read the whole stream. So the larger size we use here, the fewer times we make these small copies. On the other hand, if we read the whole stream early on (when does that typically happen?), this doesn't really matter since we will already have the whole thing buffered. The only danger here would be if ended up buffering too much during an attempt to get the size, resulting in overflowing the Java buffer, so the caller can no longer rewind to do a full decode. Note that it's only a problem if reading the exact amount requested did *not* overflow the Java buffer. In general, though, this only matters if it helps performance, so it'd be nice to know what difference it makes. https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:271: const size_t minSizeToRead = 256; nit: This is effectively a constant, which we would typically write as kMinSizeToRead https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:272: const size_t sizeMustRead = newSize - fStreamBuffer.bytesWritten(); nit: These names don't distinguish themselves very well to me. Maybe this should be named "sizeRequested"?
https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:270: // Try to read at least 256 bytes to avoid to many small reads. On 2016/01/28 16:49:58, scroggo wrote: > On 2016/01/28 16:23:44, msarett wrote: > > On 2016/01/28 14:27:44, yujieqin wrote: > > > On 2016/01/28 14:20:25, adaubert wrote: > > > > FYI: In Snapseed we usually read 8192, same applies for the default value > of > > > the > > > > BufferedInputStream.java. > > > > > > I don't have preference here. msarett@ & scroggo@, which size do you prefer > > for > > > Skia? > > > > I guess how much we need really depends on the image... The case I mentioned > in > > https://codereview.chromium.org/1520403003 calls bufferMoreData over 200 times > > to increase from 108kb to 112kb. 8192 covers this (plus some extra) and 256 > > seems a little small here. > > > > I would lean toward the larger size. I don't think an extra 8 kb will make > much > > difference considering it is much smaller than the stream size. But I would > > defer your judgment on what is the best balance for RAW. > > As Matt suggested, the ideal number will depend on the data. Let's assume that > the common case is a valid image, meaning we'll eventually read the whole > stream. So the larger size we use here, the fewer times we make these small > copies. On the other hand, if we read the whole stream early on (when does that > typically happen?), this doesn't really matter since we will already have the > whole thing buffered. > In the case that Piex can find a preview, I think we may not need to read the entire stream? > The only danger here would be if ended up buffering too much during an attempt > to get the size, resulting in overflowing the Java buffer, so the caller can no > longer rewind to do a full decode. Note that it's only a problem if reading the > exact amount requested did *not* overflow the Java buffer. > > In general, though, this only matters if it helps performance, so it'd be nice > to know what difference it makes. +1 for knowing the performance impact (if any). Though I would support this change even if we can't see a performance impact. If you want to use Skia's CodecBench, you can run: ninja -C out/Release out/Release/nanobench --images <path to image or images> --match <some part of the filename(s)> Or even better, performance tests on Android: BUILDTYPE=Release ./platform_tools/android/bin/android_ninja [-d device_id] nanobench ./platform_tools/android/bin/android_run_skia --release nanobench -images <> --match <>
On 2016/01/28 17:00:35, msarett wrote: > https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp > File src/codec/SkRawCodec.cpp (right): > > https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... > src/codec/SkRawCodec.cpp:270: // Try to read at least 256 bytes to avoid to many > small reads. > On 2016/01/28 16:49:58, scroggo wrote: > > On 2016/01/28 16:23:44, msarett wrote: > > > On 2016/01/28 14:27:44, yujieqin wrote: > > > > On 2016/01/28 14:20:25, adaubert wrote: > > > > > FYI: In Snapseed we usually read 8192, same applies for the default > value > > of > > > > the > > > > > BufferedInputStream.java. > > > > > > > > I don't have preference here. msarett@ & scroggo@, which size do you > prefer > > > for > > > > Skia? > > > > > > I guess how much we need really depends on the image... The case I > mentioned > > in > > > https://codereview.chromium.org/1520403003 calls bufferMoreData over 200 > times > > > to increase from 108kb to 112kb. 8192 covers this (plus some extra) and 256 > > > seems a little small here. > > > > > > I would lean toward the larger size. I don't think an extra 8 kb will make > > much > > > difference considering it is much smaller than the stream size. But I would > > > defer your judgment on what is the best balance for RAW. > > > > As Matt suggested, the ideal number will depend on the data. Let's assume that > > the common case is a valid image, meaning we'll eventually read the whole > > stream. So the larger size we use here, the fewer times we make these small > > copies. On the other hand, if we read the whole stream early on (when does > that > > typically happen?), this doesn't really matter since we will already have the > > whole thing buffered. > > > > In the case that Piex can find a preview, I think we may not need to read the > entire stream? > > > The only danger here would be if ended up buffering too much during an attempt > > to get the size, resulting in overflowing the Java buffer, so the caller can > no > > longer rewind to do a full decode. Note that it's only a problem if reading > the > > exact amount requested did *not* overflow the Java buffer. > > > > In general, though, this only matters if it helps performance, so it'd be nice > > to know what difference it makes. > > +1 for knowing the performance impact (if any). Though I would support this > change even if we can't see a performance impact. > > If you want to use Skia's CodecBench, you can run: > ninja -C out/Release > out/Release/nanobench --images <path to image or images> --match <some part of > the filename(s)> > > Or even better, performance tests on Android: > BUILDTYPE=Release ./platform_tools/android/bin/android_ninja [-d device_id] > nanobench > ./platform_tools/android/bin/android_run_skia --release nanobench -images <> > --match <> Changed the size from 256 to 8192. I don't see performance differences from the nanobench on my workstation. And I can not run the test Android, not sure how to offer the testing RAW image to the device, keep getting "Could not open the file. Did you forget to set the resourcePath?". But as said, I also did not expect significant performance difference.
"And I can not run the test Android, not sure how to offer the testing RAW image to the device, keep getting "Could not open the file. Did you forget to set the resourcePath?". But as said, I also did not expect significant performance difference." Sorry I can provide more instructions here. To put local images on a device, you can run: adb push <local folder of images> /data/local/tmp/<folder>/ And then run with: --images /data/local/tmp/<folder> The warnings regarding the resourcePath can be ignored. Regarding the CL, I think you've missed a few of Leon's comments. Otherwise this looks good to me.
https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:271: const size_t minSizeToRead = 256; On 2016/01/28 16:49:57, scroggo wrote: > nit: This is effectively a constant, which we would typically write as > > kMinSizeToRead Done. https://codereview.chromium.org/1644893003/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:272: const size_t sizeMustRead = newSize - fStreamBuffer.bytesWritten(); On 2016/01/28 16:49:58, scroggo wrote: > nit: These names don't distinguish themselves very well to me. Maybe this should > be named "sizeRequested"? Done.
On 2016/02/01 15:57:58, msarett wrote: > "And I > can not run the test Android, not sure how to offer the testing RAW image to the > device, keep getting "Could not open the file. Did you forget to set the > resourcePath?". But as said, I also did not expect significant performance > difference." > > Sorry I can provide more instructions here. To put local > images on a device, you can run: > adb push <local folder of images> /data/local/tmp/<folder>/ > > And then run with: > --images /data/local/tmp/<folder> > > The warnings regarding the resourcePath can be ignored. > > > Regarding the CL, I think you've missed a few of Leon's > comments. Otherwise this looks good to me. The comments are addressed now. :) And thanks for the tips, the stuff runs for me now on the Android. But since it only runs 1 loop, the results are quite flaky.
lgtm I've seen similar flakiness on Android. I always base my observations on the "minimum" value. Often, I run multiple times and take the minimum of the "minimums". Every run does the same thing, so I think it's safe to assume that the minimum is the best representation of the true runtime (when unaffected by whatever else is going on).
On 2016/02/01 17:01:07, msarett wrote: > lgtm > > I've seen similar flakiness on Android. I always base my > observations on the "minimum" value. > > Often, I run multiple times and take the minimum of the > "minimums". > > Every run does the same thing, so I think it's safe to > assume that the minimum is the best representation of > the true runtime (when unaffected by whatever else is > going on). If only looking at minimum, we got 463ms (original) --> 329ms (size 256) --> 223ms (size 8192) for Codec_N32 using one of the difficult image, which requires about 70+ reads originally. :)
That's great!!! Please feel free to add some of those stats to the commit message.
The CQ bit was checked by yujieqin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com Link to the patchset: https://codereview.chromium.org/1644893003/#ps60001 (title: "Update commit message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1644893003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1644893003/60001
Message was sent while issue was closed.
Description was changed from ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb
Message was sent while issue was closed.
On 2016/02/02 at 09:21:44, commit-bot wrote: > Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb I'm going to revert this CL as it crashed both the Nexus 5's. https://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Ne... https://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Ne...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1656303002/ by jcgregorio@google.com. The reason for reverting is: Crashed both Nexus 5s: https://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Ne... https://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Ne....
Message was sent while issue was closed.
On 2016/02/02 13:32:51, jcgregorio wrote: > On 2016/02/02 at 09:21:44, commit-bot wrote: > > Committed patchset #4 (id:60001) as > https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb > > I'm going to revert this CL as it crashed both the Nexus 5's. > > > https://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Ne... > > https://build.chromium.org/p/client.skia.android/builders/Perf-Android-GCC-Ne... By looking at the error, I saw: "Running Codec_art.jpg_N32 nonrendering error: device not found - waiting for device -" Maybe it is not about current CL?
Message was sent while issue was closed.
Description was changed from ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb ========== to ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb ==========
I had the same thought. Yujie, you can select "Edit Issue" and click the checkbox to reopen the issue. From there, you can run the Nexus 5 Perf trybot. If that runs fine, I think it is reasonable to reland this.
Looks like the trybots are already running :).
On 2016/02/02 13:45:04, msarett wrote: > Looks like the trybots are already running :). But the bots are still "gray", I think they are just not be started...
On 2016/02/02 13:47:22, yujieqin wrote: > On 2016/02/02 13:45:04, msarett wrote: > > Looks like the trybots are already running :). > > But the bots are still "gray", I think they are just not be started... The trybot passed. I will try to commit the CL again.
The CQ bit was checked by yujieqin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1644893003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1644893003/60001
Message was sent while issue was closed.
Description was changed from ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb ========== to ========== Avoid too small reads to bufferMoreData() BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/fd918e8c739065fa467cf6614181d3a2c5dcadcb Committed: https://skia.googlesource.com/skia/+/22000d1f8e2c554aec2f50a5a4cad0a5d04ca41c ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/22000d1f8e2c554aec2f50a5a4cad0a5d04ca41c |
