Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2280723003:
Avoid making the shared main thread context if we won't use it. (Closed)
|
Created:
4 years, 3 months ago by danakj Modified:
4 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, dshwang, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, jam, Justin Novosad, dglazkov+blink, Rik, darin-cc_chromium.org, blink-reviews, piman+watch_chromium.org, enne (OOO), piman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid making the shared main thread context if we won't use it.
When deciding if HTMLCanvasElement should be accelerated we changed
the code to get the shared main thread context before making the
decision because it's part of the decision. However, for most of the
things we decide on (such as are-we-using-display-list-canvas) we
don't care about the context. We only care once we decide we do want
to use it, then verify it's not using swiftshader.
So, defer getting/creating the shared main thread context until we
know we want to accelerate, then just fail to accelerate if swiftshader.
To make this logic more clear, I split the createImageBufferSurface()
method into createAcceleratedImageBufferSurface() and
createSoftwareImageBufferSurface() which is used if the former
fails/returns null. There is less nesting now so hopefully that's nice.
With this the memory.blink_memory_mobile's TheVerge case goes from
6422Kb of gpu memory back down to 4324Kb.
R=junov@chromium.org, kbr@chromium.org
BUG=640811, 606056
Committed: https://crrev.com/03fe53dc4730038b5d13613fcbad10fc9a204c54
Cr-Commit-Position: refs/heads/master@{#414806}
Patch Set 1 #Patch Set 2 : regress: . #Patch Set 3 : regress: . #Patch Set 4 : regress: rebase #
Total comments: 3
Patch Set 5 : regress: name #
Messages
Total messages: 23 (16 generated)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Description was changed from ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811 ========== to ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811,606056 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by danakj@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...
Description was changed from ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811,606056 ========== to ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811,606056 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by danakj@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 tracking this down Dana. It looks good to me, but Justin or someone else from the Canvas team should review it since it essentially only affects the 2D canvas path (as far as I understand it).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit https://codereview.chromium.org/2280723003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2280723003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:848: if (contextProvider->isSoftwareRendering()) In a follow-up CL It would be nice to be able to make this query without creating the context. Not sure how complicated it would be to do that. I'll leave it up to you to decide whether it is worthwhile. https://codereview.chromium.org/2280723003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/2280723003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.h:234: std::unique_ptr<ImageBufferSurface> createSoftwareImageBufferSurface(const IntSize& deviceSize, OpacityMode, sk_sp<SkColorSpace>); For consistency with surrounding code, please call this "Unaccelerated" instead of "Software"
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/2280723003/#ps80001 (title: "regress: name")
https://codereview.chromium.org/2280723003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/2280723003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.h:234: std::unique_ptr<ImageBufferSurface> createSoftwareImageBufferSurface(const IntSize& deviceSize, OpacityMode, sk_sp<SkColorSpace>); On 2016/08/26 14:11:12, Justin Novosad wrote: > For consistency with surrounding code, please call this "Unaccelerated" instead > of "Software" Done.
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 ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811,606056 ========== to ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811,606056 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811,606056 ========== to ========== Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG=640811,606056 Committed: https://crrev.com/03fe53dc4730038b5d13613fcbad10fc9a204c54 Cr-Commit-Position: refs/heads/master@{#414806} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/03fe53dc4730038b5d13613fcbad10fc9a204c54 Cr-Commit-Position: refs/heads/master@{#414806} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
