Do not cache itemBuilder calls in a GridView

[dnfield]

Description

Avoid caching the result of the itemBuilder for GridView. This causes problems particularly when the widget being built is expensive to hold in memory, such as when it's a memory image.

Ideally, this shouldn't land until after #61025 has run through the devicelab to show the improvement in memory usage expected.

This might potentially regress frame performance, but I can't find a benchmark ti should negatively impact. Either way, it's causing OOMs for a customer so it needs to be fixed. If we can determine a memory safe way of doing an LRU cache for this we could try that. However, we generally say that things that build widgets should be fast - I doubt we're really saving much time with this optimization.

/cc @chunhtai @Hixie

Related Issues

Fixes #61006

Tests

I added the following tests:

Unit test that we call the itemBuilder when scrolling.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.

[mehmetf]

I doubt we're really saving much time with this optimization.

I would be more comfortable if we validated this claim; either via benchmarks or if that's too hard, by manual testing. Is the performance acceptable with the repro case that the client gave us (200 6M images)? I admit it is highly synthetic but it could give us an idea of what we are losing.

[dnfield]

The use case this should help is when you have a GridView and rapidly scroll back and forth, it's caching the result of the itemBuilder function and so doesn't have to keep invoking it.

Anecdotally, I don't notice any significant performance difference with the customer's simplified demo, which I think is fairly close to their real use case. But I'm also testing on relatively recent hardware (iPhone 11, Pixel 4). Is there any chance the customer has any benchmarks for this?

I didn't think transitions perf would be much impacted by this, but it looks like the numbers are better overall:

on master: 
    "missed_transition_count": 0,
    "average_frame_build_time_millis": 2.2558806306306307,
    "90th_percentile_frame_build_time_millis": 3.367,
    "99th_percentile_frame_build_time_millis": 22.765,
    "worst_frame_build_time_millis": 63.287,
    "missed_frame_build_budget_count": 15,
    "average_frame_rasterizer_time_millis": 5.261718785151857,
    "90th_percentile_frame_rasterizer_time_millis": 6.276,
    "99th_percentile_frame_rasterizer_time_millis": 21.771,
    "worst_frame_rasterizer_time_millis": 80.495,
    "missed_frame_rasterizer_budget_count": 15,
    "frame_count": 888,
    "frame_rasterizer_count": 889,
    "average_vsync_transitions_missed": 1.1791044776119404,
    "90th_percentile_vsync_transitions_missed": 2.0,
    "99th_percentile_vsync_transitions_missed": 2.0

-========


on grid_memory_widget with master:
    "missed_transition_count": 0,
    "average_frame_build_time_millis": 2.2474882943143832,
    "90th_percentile_frame_build_time_millis": 3.33,
    "99th_percentile_frame_build_time_millis": 22.208,
    "worst_frame_build_time_millis": 41.701,
    "missed_frame_build_budget_count": 12,
    "average_frame_rasterizer_time_millis": 5.14609933035714,
    "90th_percentile_frame_rasterizer_time_millis": 6.636,
    "99th_percentile_frame_rasterizer_time_millis": 19.603,
    "worst_frame_rasterizer_time_millis": 50.042,
    "missed_frame_rasterizer_budget_count": 12,
    "frame_count": 897,
    "frame_rasterizer_count": 896,
    "average_vsync_transitions_missed": 1.1388888888888888,
    "90th_percentile_vsync_transitions_missed": 1.0,
    "99th_percentile_vsync_transitions_missed": 5.0

[dnfield]

Hmm. Looking at 59524c6, I'm wondering if this change is quite right. @Hixie wdyt?

At any rate, this patch should remove the remaining comment if we're not using it.

It's not clear to me what the failure mode would be here.

[dnfield]

I'm going to go ahead with this. I can't come up with a case where this would cause problems, and if we find one we can fix it.