Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1318713005:
Change the way ImageDecodeBench supplies data (Closed)
|
Created:
5 years, 3 months ago by scroggo_chromium Modified:
5 years, 3 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@ImageSource2 Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionChange the way ImageDecodeBench supplies data
By default, pass the data as a single, pre-merged SharedBuffer, to imitate
decoding an image whose encoded data is stored in the Blink MemoryCache.
Add an option (supplied by a command line flag) to supply the data in chunks.
This simulates network traffic. It also allows us to better evaluate the
impact of calling SharedBuffer::data() in every call to decode.
TBR=tkent@chromium.org
BUG=467772
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201836
Patch Set 1 #Patch Set 2 : Add an option to pass data in chunks #
Total comments: 10
Patch Set 3 : Remove unnecessary check #Patch Set 4 : Respond to Noel's comments in patch set 2 #
Total comments: 10
Patch Set 5 : Respond to Noel's comments in patch set 4 #Messages
Total messages: 22 (7 generated)
scroggo@chromium.org changed reviewers: + noel@chromium.org, pkasting@chromium.org
LGTM https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:298: fprintf(stderr, "Should have exited before now."); This should never be reachable, I wouldn't bother with it
https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:298: fprintf(stderr, "Should have exited before now."); On 2015/09/01 22:27:11, Peter Kasting wrote: > This should never be reachable, I wouldn't bother with it Done.
https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:18: FIXME: Consider adding an input data packetization option. Break the input Has this FIXME been addressed? If so, delete it. https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:280: bool allDataReceived = true; blink style: don't compare to 0, prefer if (!chunkSize), here and elsewhere. https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:334: fprintf(stderr, "Usage: %s [--color-correct] file [iterations]\n", name); ... file [iterations] [????] ... you added a new option, also @339. https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:360: long chunkLong = strtol(argv[3], &end, 10); Silly users who provide negative values. Per line @349, I decided to not worry about it, but here you did. Is it worth the code inconsistency? Are negative chunk sizes worth worrying about?
https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:18: FIXME: Consider adding an input data packetization option. Break the input On 2015/09/02 08:39:48, noel gordon wrote: > Has this FIXME been addressed? If so, delete it. Done. https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:280: bool allDataReceived = true; On 2015/09/02 08:39:48, noel gordon wrote: > blink style: don't compare to 0, prefer if (!chunkSize), here and elsewhere. Done. https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:334: fprintf(stderr, "Usage: %s [--color-correct] file [iterations]\n", name); On 2015/09/02 08:39:48, noel gordon wrote: > ... file [iterations] [????] ... you added a new option, also @339. Done. https://codereview.chromium.org/1318713005/diff/20001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:360: long chunkLong = strtol(argv[3], &end, 10); On 2015/09/02 08:39:48, noel gordon wrote: > Silly users who provide negative values. Per line @349, I decided to not worry > about it, but here you did. Is it worth the code inconsistency? Are negative > chunk sizes worth worrying about? Done.
https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:287: RefPtr<SharedBuffer> incrementalData = SharedBuffer::create(); incrementalData -> packetData ? https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:290: const char* segment; segment -> packet https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:296: bool allDataReceived = position == data->size(); Space before this line. https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:335: // Control decode bench iterations. ... iterations and chunkSize. https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:396: // Merge the buffers. This imitates the MemCache case, where the data is These details about Memcache and packet chunkSize are already in the ChangeLog. Adding them here threw me on first reading. The mystery is data->data() and what it does, so I'd focus on that ... something like // Consolidate the SharedBuffer data() segments into one, contiguous block of memory. to set the scene for all that follows?
scroggo@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:287: RefPtr<SharedBuffer> incrementalData = SharedBuffer::create(); On 2015/09/02 23:29:35, noel gordon wrote: > incrementalData -> packetData ? Done. https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:290: const char* segment; On 2015/09/02 23:29:35, noel gordon wrote: > segment -> packet Done. https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:296: bool allDataReceived = position == data->size(); On 2015/09/02 23:29:35, noel gordon wrote: > Space before this line. Done. https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:335: // Control decode bench iterations. On 2015/09/02 23:29:35, noel gordon wrote: > ... iterations and chunkSize. Done. I've also changed "chunkSize" to "packetSize". https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... Source/web/ImageDecodeBench.cpp:396: // Merge the buffers. This imitates the MemCache case, where the data is On 2015/09/02 23:29:35, noel gordon wrote: > These details about Memcache and packet chunkSize are already in the ChangeLog. > Adding them here threw me on first reading. The mystery is data->data() and > what it does, so I'd focus on that ... something like > > // Consolidate the SharedBuffer data() segments into one, contiguous block of > memory. > > to set the scene for all that follows? Done.
On 2015/09/03 22:10:26, scroggo wrote: https://codereview.chromium.org/1318713005/diff/60001/Source/web/ImageDecodeB... > Source/web/ImageDecodeBench.cpp:335: // Control decode bench iterations. > On 2015/09/02 23:29:35, noel gordon wrote: > > ... iterations and chunkSize. > > Done. I've also changed "chunkSize" to "packetSize". Ah yes. Overall, looks great Leon, thanks.
LGTM
The CQ bit was checked by scroggo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1318713005/#ps80001 (title: "Respond to Noel's comments in patch set 4")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1318713005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1318713005/80001
scroggo@chromium.org changed reviewers: + tkent@chromium.org
tkent@, can you give me an owners review please?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1318713005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1318713005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201836
Message was sent while issue was closed.
lgtm |
