don't report GPU image size #35473
don't report GPU image size #35473
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The rationale: pain itself:
|
| @@ -64,9 +64,10 @@ SkISize DlDeferredImageGPU::dimensions() const { | |||
|
|
|||
| // |DlImage| | |||
| size_t DlDeferredImageGPU::GetApproximateByteSize() const { | |||
There was a problem hiding this comment.
I think we should just do this in image.cc (CanvasImage::GetAllocationSize instead of here.
There's a feature request somewhere to expose the size of an image through to dart:ui. This method should be left as-is to support that. Alternatively, we should just delete these methods from DlImage.
There's already a hacky work around in CanvasImage::GetAllocationSize to try to avoid tripping up the VM when you allocate a really large image. I'm becoming more convinced that we should just stop telling the VM about image sizes.
@rmacnak-google or @jason-simmons might have some better ideas.
|
I used to investigate memory leaks of image objects through dart external memory reported by dev tools. Fixing memory issues can be more difficult if we don't tell dart the memory size of the binding object |
lib/ui/painting/image.cc
Outdated
| // We don't actually want Dart's GC to use the size of this object to make GC | ||
| // decisions, as it is generally both created and disposed in the framework. | ||
| // This is similar to why we do not report the sizes of engine layers. | ||
| return sizeof(this); |
There was a problem hiding this comment.
I guess this is wrong.. it should be sizeof(*this).
There was a problem hiding this comment.
Otherwise we're reporting the size of a pointer rather than the size of the CanvasImage class.
I'd like to hear a bit more about this. I think we need a better solution in general for detecting leaked graphics objects (such as pictures and images). But there are many things that can retain images even if there's no corresponding dart:ui object specific to it. |
|
I'm on board with this, adding @jason-simmons for a sanity check here. |
jonahwilliams
added
the
autosubmit
We don't actually want Dart's GC to use the size of this object to make GC decisions, as it is generally both created and disposed in the framework. This is similar to why we do not report the sizes of engine layers.
FYI @iskakaushik , if you use the zoom page transition as an example you'll have to revert this change after we land it.