You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
~40% regression in complex_layout_scroll_perf__memory #36079
This commit caused a similar regression in the "flutter_gallery__image_cache_memory diff-median" and the "flutter_gallery__image_cache_memory diff-min" benchmarks
In the devicelab logs complex_layout_scroll_perf__memory shows a consistent pattern after the regression started. The "Native Heap" metric from Android's dumpsys meminfo increases by about 3MB during each run of the scroll test.
Prior to the regression, that value would only increase by a couple hundred KB.
However, I've been unable to reproduce this when I run complex_layout_scroll_perf__memory on my desktop.
The difference in native heap usage might be related to the image decompression thread pool introduced in flutter-team-archive/engine@ad582b5
Apparently complex_layout_scroll_perf__memory is timing sensitive. I can reproduce the difference in native heap usage by running the benchmark controller on an old MacBook or by inserting a delay in MemoryTest.useMemory before the call to recordStart(). Maybe that gives the device time to do a GC before the scroll test starts?
Anyway, I can confirm that the difference in behavior started with flutter-team-archive/engine@ad582b5. However, part of the difference seems to be caused by lower native heap consumption at the start of the test. Possibly the new image decoding approach shifts some memory usage into the time interval measured by the test? (versus the old approach doing equivalent work prior to the test's initial data collection)
Another change introduced in flutter-team-archive/engine@ad582b5 is that the SingleFrameCodec is holding on to the encoded image data buffer (descriptor_.data) after decoding the frame.
This is likely what accounts for most of the observed difference in native heap usage.
This issue was caused by the removal of a previous hack in the way the compressed host allocation used to be discarded. This hack was getting preventing users from querying the image size from the codec without incurring the cost of a 1:1 decompression (before attempting texture decompression at a reduced size).
I have highlighted the following section from the commit message of the patch that caused this regression.
... creating a codec with a single image frame immediately kicks of
decompression and upload of that frame image (even if the frame was never
request from the codec). This hack was added because we wanted to get rid of
the compressed image allocation ASAP. The expectation was codecs would only be
created with the sole purpose of getting the decompressed image bytes.
However, for applications that only create codecs to get image sizes (but
never actually decompress the same), we would end up replacing the compressed
image allocation with a larger allocation (device resident no less) for no
obvious use...
In the benchmark, for reasons that are not fully apparent ATM, the codec was being kept around after the frame was requested. Now, just like the multi-frame codec, the single-frame codec will also hold onto the host allocation so that it will only decompress to the device allocation lazily when the next frame is requested (or not at all in the resizing case). The reasoning being that the compressed host allocation is many times smaller than the decompressed device allocation. So, when a device allocation is not necessary, a lot of work can be avoided.
After chatting with Jason, I think we can bring back the old behavior by discarding the image descriptors compressed image data in the callback that returns the device resident image from the image decompressor. This is still a slight pessimization because applications that release images (but not their codecs) due to memory concerns will still keep a fully decompressed image in memory. But I think this behavior is baked into the contract at this point. Jason & I were prototyping a patch for this regression which we can land on Monday.
We should also figure out where the codec are being referenced though. The pessimization and the fact that both the device and host allocations will be kept for multi-frame codecs makes this behavior particularly memory hungry.
Investigated this some more and noticed that native heap size was continuing to increase even after changing SingleFrameCodec to drop its reference to the compressed image data buffer.
I tried disabling the image cache in the framework, running the Shrine demo in Gallery, and repeatedly scrolling through the images. Native heap size and private dirty pages continued to increase, and the rate of growth was correlated with the size of the image decoder thread pool.
It looks like this is due to implementation details of Android's malloc related to allocating buffers on one thread and freeing them on another thread. In this case, the compressed image buffer is allocated on the UI thread but is freed on a decoder worker thread.
We could work around this by queueing deletion of the buffer to the UI thread or using another mechanism (e.g. anonymous mappings) to allocate the buffer.
flutter-team-archive/engine@b7b791b will help, but the memory usage behavior of the image decoder thread pool will differ from the previous implementation. We can rebase the benchmarks after that PR rolls over to the framework.
The additions in the heap size have been confirmed to not be leaks. Using additional threads for image decompression seems to be causing the system malloc (jemalloc) to allocate additional heap space in thread specific caches. These thread specific caches are a feature used to optimize multi-threaded performance and reduce fragmentation. This memory is available to the application even if shows as being claimed in the heap allocation report. Also, we have added an Android specific mitigation where allocations for images also don't use malloc, instead, we now allocate an anonymous FD mapping for image data and unmap the same when done. The utility of this mitigation is suspect as we now incur syscalls on allocation & deallocation but reported heap allocation is lower. Further optimization for this benchmark will be counterproductive. This regression also does not show up on iOS as it uses a different allocator.
The recommendation is to re-baseline the benchmark.
This should be an improvement instead of a regression: both the start memory and the end memory drops after that PR. The start memory drops more significantly so the diff (end memory - start memory) increases. Presumably, less start memory leaves GC more room to retain more objects so the diff increases. But since the overall memory strictly decreases, this has no negative effect for any users.
Rebaseline and close this issue as the original regression disappeared since 7/19/2019, and the new regression is actually an improvement (#36079 (comment)).
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.
Activity
tvolkert commentedon Jul 12, 2019
This commit caused a similar regression in the "flutter_gallery__image_cache_memory diff-median" and the "flutter_gallery__image_cache_memory diff-min" benchmarks
jason-simmons commentedon Jul 12, 2019
In the devicelab logs complex_layout_scroll_perf__memory shows a consistent pattern after the regression started. The "Native Heap" metric from Android's
dumpsys meminfoincreases by about 3MB during each run of the scroll test.Prior to the regression, that value would only increase by a couple hundred KB.
However, I've been unable to reproduce this when I run complex_layout_scroll_perf__memory on my desktop.
The difference in native heap usage might be related to the image decompression thread pool introduced in flutter-team-archive/engine@ad582b5
jason-simmons commentedon Jul 13, 2019
Apparently complex_layout_scroll_perf__memory is timing sensitive. I can reproduce the difference in native heap usage by running the benchmark controller on an old MacBook or by inserting a delay in
MemoryTest.useMemorybefore the call torecordStart(). Maybe that gives the device time to do a GC before the scroll test starts?Anyway, I can confirm that the difference in behavior started with flutter-team-archive/engine@ad582b5. However, part of the difference seems to be caused by lower native heap consumption at the start of the test. Possibly the new image decoding approach shifts some memory usage into the time interval measured by the test? (versus the old approach doing equivalent work prior to the test's initial data collection)
@chinmaygarde would that make sense?
jason-simmons commentedon Jul 13, 2019
Another change introduced in flutter-team-archive/engine@ad582b5 is that the
SingleFrameCodecis holding on to the encoded image data buffer (descriptor_.data) after decoding the frame.This is likely what accounts for most of the observed difference in native heap usage.
chinmaygarde commentedon Jul 13, 2019
This issue was caused by the removal of a previous hack in the way the compressed host allocation used to be discarded. This hack was getting preventing users from querying the image size from the codec without incurring the cost of a 1:1 decompression (before attempting texture decompression at a reduced size).
I have highlighted the following section from the commit message of the patch that caused this regression.
In the benchmark, for reasons that are not fully apparent ATM, the codec was being kept around after the frame was requested. Now, just like the multi-frame codec, the single-frame codec will also hold onto the host allocation so that it will only decompress to the device allocation lazily when the next frame is requested (or not at all in the resizing case). The reasoning being that the compressed host allocation is many times smaller than the decompressed device allocation. So, when a device allocation is not necessary, a lot of work can be avoided.
After chatting with Jason, I think we can bring back the old behavior by discarding the image descriptors compressed image data in the callback that returns the device resident image from the image decompressor. This is still a slight pessimization because applications that release images (but not their codecs) due to memory concerns will still keep a fully decompressed image in memory. But I think this behavior is baked into the contract at this point. Jason & I were prototyping a patch for this regression which we can land on Monday.
We should also figure out where the codec are being referenced though. The pessimization and the fact that both the device and host allocations will be kept for multi-frame codecs makes this behavior particularly memory hungry.
In a single frame codec, release the encoded image buffer after givin…
jason-simmons commentedon Jul 16, 2019
Investigated this some more and noticed that native heap size was continuing to increase even after changing SingleFrameCodec to drop its reference to the compressed image data buffer.
I tried disabling the image cache in the framework, running the Shrine demo in Gallery, and repeatedly scrolling through the images. Native heap size and private dirty pages continued to increase, and the rate of growth was correlated with the size of the image decoder thread pool.
It looks like this is due to implementation details of Android's malloc related to allocating buffers on one thread and freeing them on another thread. In this case, the compressed image buffer is allocated on the UI thread but is freed on a decoder worker thread.
We could work around this by queueing deletion of the buffer to the UI thread or using another mechanism (e.g. anonymous mappings) to allocate the buffer.
In a single frame codec, release the compressed image buffer after gi…
In a single frame codec, release the compressed image buffer after gi…
In a single frame codec, release the compressed image buffer after gi…
gspencergoog commentedon Jul 18, 2019
@jason-simmons So, what is the status of this bug? Has it been mitigated, or is there still more work to be done?
jason-simmons commentedon Jul 19, 2019
flutter-team-archive/engine@b7b791b will help, but the memory usage behavior of the image decoder thread pool will differ from the previous implementation. We can rebase the benchmarks after that PR rolls over to the framework.
Disabling Firebase Test Lab smoke test to unblock autoroller (#36503)
gspencergoog commentedon Jul 19, 2019
@jason-simmons, Is it mitigated to the point that this is no longer a TODAY bug? (assuming I merge that change into the google3 roll)
Hixie commentedon Jul 22, 2019
(this started around #36238)
chinmaygarde commentedon Jul 22, 2019
The additions in the heap size have been confirmed to not be leaks. Using additional threads for image decompression seems to be causing the system malloc (
jemalloc) to allocate additional heap space in thread specific caches. These thread specific caches are a feature used to optimize multi-threaded performance and reduce fragmentation. This memory is available to the application even if shows as being claimed in the heap allocation report. Also, we have added an Android specific mitigation where allocations for images also don't usemalloc, instead, we now allocate an anonymous FD mapping for image data and unmap the same when done. The utility of this mitigation is suspect as we now incur syscalls on allocation & deallocation but reported heap allocation is lower. Further optimization for this benchmark will be counterproductive. This regression also does not show up on iOS as it uses a different allocator.The recommendation is to re-baseline the benchmark.
Disabling Firebase Test Lab smoke test to unblock autoroller (flutter…
franciscojma86 commentedon Jul 30, 2019
Regression has been re-introduced since 7/26 on 9b150f134b9b6ba103ec305f489645b5fee905d2.
Hixie commentedon Jul 31, 2019
@liyuqian this one too. (see #37176)
liyuqian commentedon Jul 31, 2019
This should be an improvement instead of a regression: both the start memory and the end memory drops after that PR. The start memory drops more significantly so the diff (end memory - start memory) increases. Presumably, less start memory leaves GC more room to retain more objects so the diff increases. But since the overall memory strictly decreases, this has no negative effect for any users.
I think we should:
liyuqian commentedon Jul 31, 2019
Rebaseline and close this issue as the original regression disappeared since 7/19/2019, and the new regression is actually an improvement (#36079 (comment)).
github-actions commentedon Aug 29, 2021
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.