Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1681553003:
Optionally run RAW images serially (Closed)
DescriptionOptionally run RAW images serially
RAW images use a lot of memory. Add a new FLAG to run one at a time so
we have less risk of running out of memory.
Isolate RAW images to their own thread on particular devices where our
images cause OOM errors.
Locally, this drops the max memory use from 3945 MB to 1664 MB (running only --image --images <RAW images we test>)
BUG=skia:4912
BUG=skia:4878
BUG=b/27035849
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1681553003
Committed: https://skia.googlesource.com/skia/+/3ac66e98f8701b8105a4c6ae85de61a9e8e5343a
Patch Set 1 #
Total comments: 10
Patch Set 2 : enclave for AndroidCodecSrc; compute in constructor #Patch Set 3 : Rebase #
Total comments: 3
Patch Set 4 : Change to serial() (from crrev.com/1675423002) #Patch Set 5 : Increment actualExt #
Total comments: 10
Patch Set 6 : Remove ifdefs #
Messages
Total messages: 30 (11 generated)
Description was changed from ========== Optionally run RAW images in their own enclave RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 ========== to ========== Optionally run RAW images in their own enclave RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
scroggo@google.com changed reviewers: + msarett@google.com, mtklein@google.com
I'm running some trybots to get an idea of whether this helps. It will be slower though, as we discussed. https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.h#newcode25 dm/DMSrcSink.h:25: const char* SkStringExtension(const SkString&); I needed this in more than one place, and both of those places are in DM, so I put it here. I could put it in SkString or SkOSFile (in SkOSPath class) if we think more clients will want it.
1gtm the approach. Deferring to Mike on the specifics. https://codereview.chromium.org/1681553003/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1681553003/diff/1/dm/DM.cpp#newcode1172 dm/DM.cpp:1172: switch(i) { Do we need to handle kRAW_Enclave here? Is it handled by the default? By using the default, you don't need to check SK_CODEC_DECODES_RAW? https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.cpp#newcode617 dm/DMSrcSink.cpp:617: int CodecSrc::enclave() const { I think AndroidCodecSrc should also override enclave()?
https://codereview.chromium.org/1681553003/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1681553003/diff/1/dm/DM.cpp#newcode1172 dm/DM.cpp:1172: switch(i) { On 2016/02/08 19:35:37, msarett wrote: > Do we need to handle kRAW_Enclave here? Is it handled by the default? By using > the default, you don't need to check SK_CODEC_DECODES_RAW? Yes, it is handled by the default, so no need to check SK_CODEC_DECODES_RAW https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.cpp#newcode617 dm/DMSrcSink.cpp:617: int CodecSrc::enclave() const { On 2016/02/08 19:35:37, msarett wrote: > I think AndroidCodecSrc should also override enclave()? Yep. That's at least partially responsible for my trybot failures... And I also decided to move this work into the constructor, since this method will be called once for each sink. https://codereview.chromium.org/1681553003/diff/40001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1681553003/diff/40001/dm/DM.cpp#newcode1162 dm/DM.cpp:1162: // Use the enclave of the Src. Actually, this is problematic. We'll end up running GPU in different threads now. Back to the drawing board... (IIUC, the enclave still runs in parallel to other enclaves - it just prevents Sinks/Srcs in the same enclave from running simultaneously.) (FWIW, crrev.com/1677133002 makes this safe in the specific case - we won't run RAW on GPU anyway, but it's still generally unsafe.) We could switch this around, and only honor the Src's enclave if the Sink is kAnyThread. RAW decodes are thread safe, so if they occasionally run simultaneously it *may* be okay; we're just trying to cut down on memory use. I'm open to ideas. (And the simplest option, just to turn the bots green, is to just disable on 5 and 9, like we do for Player)
https://codereview.chromium.org/1681553003/diff/40001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1681553003/diff/40001/dm/DM.cpp#newcode1162 dm/DM.cpp:1162: // Use the enclave of the Src. On 2016/02/08 20:08:12, scroggo wrote: > Actually, this is problematic. We'll end up running GPU in different threads > now. Back to the drawing board... (IIUC, the enclave still runs in parallel to > other enclaves - it just prevents Sinks/Srcs in the same enclave from running > simultaneously.) > > (FWIW, crrev.com/1677133002 makes this safe in the specific case - we won't run > RAW on GPU anyway, but it's still generally unsafe.) > > We could switch this around, and only honor the Src's enclave if the Sink is > kAnyThread. RAW decodes are thread safe, so if they occasionally run > simultaneously it *may* be okay; we're just trying to cut down on memory use. > This seems reasonable to me, but I'm not sure I understand completely. The Sink's enclave can be kAny or kGPU? And in the kGPU case, we run single-threaded? Where the Src enclave can be kAny or kRaw? And in the kRAW case, we run single-threaded? So why won't this work all the time? > I'm open to ideas. (And the simplest option, just to turn the bots green, is to > just disable on 5 and 9, like we do for Player)
Description was changed from ========== Optionally run RAW images in their own enclave RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optionally run RAW images serially RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by scroggo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1681553003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1681553003/80001
A good reference point that I should probably note in my commit message (will once it finishes...): Running in parallel, The max memory use is 3945 MB. Running with --noRAW_threading I am close to 1600 right now, with close to 4000 pending tasks. I'm hoping it will remain lower... https://codereview.chromium.org/1681553003/diff/40001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1681553003/diff/40001/dm/DM.cpp#newcode1162 dm/DM.cpp:1162: // Use the enclave of the Src. On 2016/02/08 20:16:19, msarett wrote: > On 2016/02/08 20:08:12, scroggo wrote: > > Actually, this is problematic. We'll end up running GPU in different threads > > now. Back to the drawing board... (IIUC, the enclave still runs in parallel to > > other enclaves - it just prevents Sinks/Srcs in the same enclave from running > > simultaneously.) > > > > (FWIW, crrev.com/1677133002 makes this safe in the specific case - we won't > run > > RAW on GPU anyway, but it's still generally unsafe.) > > > > We could switch this around, and only honor the Src's enclave if the Sink is > > kAnyThread. RAW decodes are thread safe, so if they occasionally run > > simultaneously it *may* be okay; we're just trying to cut down on memory use. > > > > This seems reasonable to me, but I'm not sure I understand completely. The > Sink's enclave can be kAny or kGPU? And in the kGPU case, we run > single-threaded? Where the Src enclave can be kAny or kRaw? And in the kRAW > case, we run single-threaded? > > So why won't this work all the time? We went a different direction (Mike submitted crrev.com/1675423002, which changes the way we decide to run things in parallel), but in case you're still curious: The (old) way to do things is to run an enclave in its own thread, and everything else in multiple threads. The key takeaway is that the enclaves run in parallel. (The new way is fairly similar - serial run on the main thread, and everything else can run in other threads.) The Sink can be kAny or kGPU. The Src can be kAny or kRAW. If the Src is kRAW, we will always run it in the kRAW enclave, even if the Sink is kGPU. So some GPU Sinks will run in the GPU enclave, and others will run in the RAW enclave, so they may run at the same time. > > > I'm open to ideas. (And the simplest option, just to turn the bots green, is > to > > just disable on 5 and 9, like we do for Player) >
Description was changed from ========== Optionally run RAW images serially RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optionally run RAW images serially RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. Locally, this drops the max memory use from 3945 MB to 1664 MB (running only --image --images <RAW images we test>) BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
On 2016/02/08 21:09:30, scroggo wrote: > A good reference point that I should probably note in my commit message (will > once it finishes...): > > Running in parallel, The max memory use is 3945 MB. Running with > --noRAW_threading I am close to 1600 right now, with close to 4000 pending > tasks. I'm hoping it will remain lower... Yep. Updated the commit message.
https://codereview.chromium.org/1681553003/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1681553003/diff/1/dm/DM.cpp#newcode1160 dm/DM.cpp:1160: tasks.push_back(Task(gSrcs[i], gSinks[j])); This reads weirdly asymmetric now. Let's really rewrite it rather than grafting onto it: for (int i = 0; i < gSrcs.count(); i++) { Task task(gSrcs[i], gSinks[j]) if (task.src->enclave() != kAnyThread_Enclave) { enclaves[task.src->enclave()].push_back(task); } else { enclaves[task.sink->enclave()].push_back(task); } } Though I think this logic breaks if we start testing RAW against GPU. The sink enclave should trump there. https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.cpp#newcode618 dm/DMSrcSink.cpp:618: #ifdef SK_CODEC_DECODES_RAW Why #ifdef? Doesn't the actual logic you wrote still hold true? https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.h#newcode25 dm/DMSrcSink.h:25: const char* SkStringExtension(const SkString&); On 2016/02/08 18:57:52, scroggo wrote: > I needed this in more than one place, and both of those places are in DM, so I > put it here. I could put it in SkString or SkOSFile (in SkOSPath class) if we > think more clients will want it. Better call this FileExtension() or FileNameExtension()? It's not part of Skia, and SkStrings don't have an extension. I still think this is probably simpler and clearer inline: const char* ext = strrchr(path.c_str(), '.'); if (ext++) { ... use ext ... } https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.h#newcode67 dm/DMSrcSink.h:67: #ifdef SK_CODEC_DECODES_RAW Why the #ifdef? https://codereview.chromium.org/1681553003/diff/1/dm/DMSrcSink.h#newcode74 dm/DMSrcSink.h:74: // All Srcs must be thread safe. Better update / remove this. https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcode41 dm/DMSrcSink.cpp:41: #ifdef SK_CODEC_DECODES_RAW Doesn't need an ifdef? https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:245: #ifdef SK_CODEC_DECODES_RAW Doesn't need an ifdef? https://codereview.chromium.org/1681553003/diff/80001/tools/dm_flags.py File tools/dm_flags.py (left): https://codereview.chromium.org/1681553003/diff/80001/tools/dm_flags.py#oldco... tools/dm_flags.py:158: r = ["arw", "cr2", "dng", "nef", "nrw", "orf", "raf", "rw2", "pef", "srw", Is this still used?
Oops, ignore comments on old patchsets :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1681553003/diff/80001/tools/dm_flags.py File tools/dm_flags.py (left): https://codereview.chromium.org/1681553003/diff/80001/tools/dm_flags.py#oldco... tools/dm_flags.py:158: r = ["arw", "cr2", "dng", "nef", "nrw", "orf", "raf", "rw2", "pef", "srw", On 2016/02/08 21:26:55, mtklein wrote: > Is this still used? Yes. Though I should check if we still need to blacklist these images on the GPU?
https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcode41 dm/DMSrcSink.cpp:41: #ifdef SK_CODEC_DECODES_RAW On 2016/02/08 21:26:55, mtklein wrote: > Doesn't need an ifdef? Not needed, but I think it makes sense. If it's not defined, this FLAG doesn't make a difference. So it seems unnecessary to include it as an option. But I don't feel strongly on that one. (We do not use the guard around gpu_threading, which is similar.) https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:245: #ifdef SK_CODEC_DECODES_RAW On 2016/02/08 21:26:55, mtklein wrote: > Doesn't need an ifdef? This ifdef isn't strictly necessary, but it skips wasted work (checking every file extension) when the ultimate outcome is going to be return false always.
https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcode41 dm/DMSrcSink.cpp:41: #ifdef SK_CODEC_DECODES_RAW On 2016/02/08 21:31:30, scroggo wrote: > On 2016/02/08 21:26:55, mtklein wrote: > > Doesn't need an ifdef? > > Not needed, but I think it makes sense. If it's not defined, this FLAG doesn't > make a difference. So it seems unnecessary to include it as an option. But I > don't feel strongly on that one. (We do not use the guard around gpu_threading, > which is similar.) Please no ifdefs that don't strictly matter. https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:245: #ifdef SK_CODEC_DECODES_RAW On 2016/02/08 21:31:30, scroggo wrote: > On 2016/02/08 21:26:55, mtklein wrote: > > Doesn't need an ifdef? > > This ifdef isn't strictly necessary, but it skips wasted work (checking every > file extension) when the ultimate outcome is going to be return false always. Ditto. (We're not allowed to pretend we're optimizing code unless we actually measure it.)
https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcode41 dm/DMSrcSink.cpp:41: #ifdef SK_CODEC_DECODES_RAW On 2016/02/08 21:45:47, mtklein wrote: > On 2016/02/08 21:31:30, scroggo wrote: > > On 2016/02/08 21:26:55, mtklein wrote: > > > Doesn't need an ifdef? > > > > Not needed, but I think it makes sense. If it's not defined, this FLAG doesn't > > make a difference. So it seems unnecessary to include it as an option. But I > > don't feel strongly on that one. (We do not use the guard around > gpu_threading, > > which is similar.) > > Please no ifdefs that don't strictly matter. Done. https://codereview.chromium.org/1681553003/diff/80001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:245: #ifdef SK_CODEC_DECODES_RAW On 2016/02/08 21:45:47, mtklein wrote: > On 2016/02/08 21:31:30, scroggo wrote: > > On 2016/02/08 21:26:55, mtklein wrote: > > > Doesn't need an ifdef? > > > > This ifdef isn't strictly necessary, but it skips wasted work (checking every > > file extension) when the ultimate outcome is going to be return false always. > > Ditto. > > (We're not allowed to pretend we're optimizing code unless we actually measure > it.) Done.
lgtm
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1681553003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1681553003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: No JSON object could be decoded
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1681553003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1681553003/100001
Message was sent while issue was closed.
Description was changed from ========== Optionally run RAW images serially RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. Locally, this drops the max memory use from 3945 MB to 1664 MB (running only --image --images <RAW images we test>) BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optionally run RAW images serially RAW images use a lot of memory. Add a new FLAG to run one at a time so we have less risk of running out of memory. Isolate RAW images to their own thread on particular devices where our images cause OOM errors. Locally, this drops the max memory use from 3945 MB to 1664 MB (running only --image --images <RAW images we test>) BUG=skia:4912 BUG=skia:4878 BUG=b/27035849 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/3ac66e98f8701b8105a4c6ae85de61a9e8e5343a ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/3ac66e98f8701b8105a4c6ae85de61a9e8e5343a |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
