Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108163003:
Fix dead-lock issue attempting to decode (Closed)
|
Created:
4 years, 5 months ago by scroggo Modified:
4 years, 5 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, Rik, f(malita), blink-reviews, Stephen Chennney, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@componentBuild Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix dead-lock issue attempting to decode
During serialization, if DecodingImageGenerator::onRefEncodedData()
returns nullptr, this may require the serialization code to decode so
it can have something to then encode and serialize.
This can cause a deadlock if the DecodingImageGenerator is using
discardable memory, which cannot be used from certain threads (where
serialization may be occurring).
Currently, onRefEncodedData() returns nullptr if !m_allDataReceived.
This was to fix crbug.com/568016, but that has been further fixed by
crrev.com/1970773002, which avoids calling onRefEncodedData() to pass
it to the GPU, since even if m_allDataReceived is true, we do not yet
support any GPU texture formats.
This change returns the encoded data even if the data has not all been
received, except if the GrContext argument is not NULL for the future
when the GPU starts calling it again.
This will result in serializing the existing data and avoid the
deadlock.
BUG=607227, 623475
Committed: https://crrev.com/7ba0aa90179d42ac4806386a88d50ed3778ae30b
Cr-Commit-Position: refs/heads/master@{#402963}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add a TODO #Messages
Total messages: 18 (8 generated)
scroggo@chromium.org changed reviewers: + fmalita@chromium.org, reed@google.com, scroggo@chromium.org
The CQ bit was checked by scroggo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for fixing the regression, LGTM. I wonder whether there are other serialization cases which may trigger DM allocations though. Maybe long-term devtools should serialize the SKP on a different thread to be safe. Can you add a note to the bug? nit: not sure if multiple BUG= lines work as intended, I've always used a comma-separated bug list on a single line. https://codereview.chromium.org/2108163003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2108163003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:60: // crbug.com/568016 for details about such a slowdown.) It seems a bit fragile to tweak this implementation for internal Skia semantics. Would it make sense to add a "bool requireCompleteData" param to onRefEncodedData() at some point?
Description was changed from ========== Fix dead-lock issue attempting to decode During serialization, if DecodingImageGenerator::onRefEncodedData() returns nullptr, this may require the serialization code to decode so it can have something to then encode and serialize. This can cause a deadlock if the DecodingImageGenerator is using discardable memory, which cannot be used from certain threads (where serialization may be occurring). Currently, onRefEncodedData() returns nullptr if !m_allDataReceived. This was to fix crbug.com/568016, but that has been further fixed by crrev.com/1970773002, which avoids calling onRefEncodedData() to pass it to the GPU, since even if m_allDataReceived is true, we do not yet support any GPU texture formats. This change returns the encoded data even if the data has not all been received, except if the GrContext argument is not NULL for the future when the GPU starts calling it again. This will result in serializing the existing data and avoid the deadlock. BUG=607227 BUG=623475 ========== to ========== Fix dead-lock issue attempting to decode During serialization, if DecodingImageGenerator::onRefEncodedData() returns nullptr, this may require the serialization code to decode so it can have something to then encode and serialize. This can cause a deadlock if the DecodingImageGenerator is using discardable memory, which cannot be used from certain threads (where serialization may be occurring). Currently, onRefEncodedData() returns nullptr if !m_allDataReceived. This was to fix crbug.com/568016, but that has been further fixed by crrev.com/1970773002, which avoids calling onRefEncodedData() to pass it to the GPU, since even if m_allDataReceived is true, we do not yet support any GPU texture formats. This change returns the encoded data even if the data has not all been received, except if the GrContext argument is not NULL for the future when the GPU starts calling it again. This will result in serializing the existing data and avoid the deadlock. BUG=607227,623475 ==========
https://codereview.chromium.org/2108163003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2108163003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:60: // crbug.com/568016 for details about such a slowdown.) On 2016/06/29 17:38:37, f(malita) wrote: > It seems a bit fragile to tweak this implementation for internal Skia semantics. > Would it make sense to add a "bool requireCompleteData" param to > onRefEncodedData() at some point? agreed, seems fragile. Can you imagine other ways to address this longer term? - explicit flags sent to this api? - diff API reporting if the generator is "complete"? - other?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/29 17:38:38, f(malita) wrote: > Thanks for fixing the regression, LGTM. > > I wonder whether there are other serialization cases which may trigger DM > allocations though. I'm only aware that we use DM allocations for images, but I suppose there could be other cases... > Maybe long-term devtools should serialize the SKP on a > different thread to be safe. It surprised me that this serialization is being done on the IO thread anyway - I would think that could slow down input from the user? > Can you add a note to the bug? Done: https://bugs.chromium.org/p/chromium/issues/detail?id=607227#c15 > > nit: not sure if multiple BUG= lines work as intended, I've always used a > comma-separated bug list on a single line. It does create links to the issues, but I think it won't update the bug, come to think of it. Switched to comma-separated. https://codereview.chromium.org/2108163003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2108163003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:60: // crbug.com/568016 for details about such a slowdown.) On 2016/06/29 17:38:37, f(malita) wrote: > It seems a bit fragile to tweak this implementation for internal Skia semantics. Agreed. Added a TODO. > Would it make sense to add a "bool requireCompleteData" param to > onRefEncodedData() at some point? Possibly. Prior to reed@'s landing of crrev.com/1970773002, we discussed many options for how to do this better, and weren't happy with any of them. For the moment, we decided to remove the call that was inefficient, and bring it back once it actually gets used. https://codereview.chromium.org/2108163003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:60: // crbug.com/568016 for details about such a slowdown.) On 2016/06/29 17:57:45, reed1 wrote: > On 2016/06/29 17:38:37, f(malita) wrote: > > It seems a bit fragile to tweak this implementation for internal Skia > semantics. > > Would it make sense to add a "bool requireCompleteData" param to > > onRefEncodedData() at some point? > > agreed, seems fragile. Can you imagine other ways to address this longer term? > - explicit flags sent to this api? > - diff API reporting if the generator is "complete"? > - other? Tracking with a bug: skbug.com/5485
The CQ bit was checked by scroggo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2108163003/#ps20001 (title: "Add a TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix dead-lock issue attempting to decode During serialization, if DecodingImageGenerator::onRefEncodedData() returns nullptr, this may require the serialization code to decode so it can have something to then encode and serialize. This can cause a deadlock if the DecodingImageGenerator is using discardable memory, which cannot be used from certain threads (where serialization may be occurring). Currently, onRefEncodedData() returns nullptr if !m_allDataReceived. This was to fix crbug.com/568016, but that has been further fixed by crrev.com/1970773002, which avoids calling onRefEncodedData() to pass it to the GPU, since even if m_allDataReceived is true, we do not yet support any GPU texture formats. This change returns the encoded data even if the data has not all been received, except if the GrContext argument is not NULL for the future when the GPU starts calling it again. This will result in serializing the existing data and avoid the deadlock. BUG=607227,623475 ========== to ========== Fix dead-lock issue attempting to decode During serialization, if DecodingImageGenerator::onRefEncodedData() returns nullptr, this may require the serialization code to decode so it can have something to then encode and serialize. This can cause a deadlock if the DecodingImageGenerator is using discardable memory, which cannot be used from certain threads (where serialization may be occurring). Currently, onRefEncodedData() returns nullptr if !m_allDataReceived. This was to fix crbug.com/568016, but that has been further fixed by crrev.com/1970773002, which avoids calling onRefEncodedData() to pass it to the GPU, since even if m_allDataReceived is true, we do not yet support any GPU texture formats. This change returns the encoded data even if the data has not all been received, except if the GrContext argument is not NULL for the future when the GPU starts calling it again. This will result in serializing the existing data and avoid the deadlock. BUG=607227,623475 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix dead-lock issue attempting to decode During serialization, if DecodingImageGenerator::onRefEncodedData() returns nullptr, this may require the serialization code to decode so it can have something to then encode and serialize. This can cause a deadlock if the DecodingImageGenerator is using discardable memory, which cannot be used from certain threads (where serialization may be occurring). Currently, onRefEncodedData() returns nullptr if !m_allDataReceived. This was to fix crbug.com/568016, but that has been further fixed by crrev.com/1970773002, which avoids calling onRefEncodedData() to pass it to the GPU, since even if m_allDataReceived is true, we do not yet support any GPU texture formats. This change returns the encoded data even if the data has not all been received, except if the GrContext argument is not NULL for the future when the GPU starts calling it again. This will result in serializing the existing data and avoid the deadlock. BUG=607227,623475 ========== to ========== Fix dead-lock issue attempting to decode During serialization, if DecodingImageGenerator::onRefEncodedData() returns nullptr, this may require the serialization code to decode so it can have something to then encode and serialize. This can cause a deadlock if the DecodingImageGenerator is using discardable memory, which cannot be used from certain threads (where serialization may be occurring). Currently, onRefEncodedData() returns nullptr if !m_allDataReceived. This was to fix crbug.com/568016, but that has been further fixed by crrev.com/1970773002, which avoids calling onRefEncodedData() to pass it to the GPU, since even if m_allDataReceived is true, we do not yet support any GPU texture formats. This change returns the encoded data even if the data has not all been received, except if the GrContext argument is not NULL for the future when the GPU starts calling it again. This will result in serializing the existing data and avoid the deadlock. BUG=607227,623475 Committed: https://crrev.com/7ba0aa90179d42ac4806386a88d50ed3778ae30b Cr-Commit-Position: refs/heads/master@{#402963} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7ba0aa90179d42ac4806386a88d50ed3778ae30b Cr-Commit-Position: refs/heads/master@{#402963} |
