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.

I'm sorry I didn't notice that there were 2 PRs from 2 different authors for this same thing, but #100127 already landed so is this PR now a duplicate?

I'm sorry I didn't notice that there were 2 PRs from 2 different authors for this same thing, but #100127 already landed so is this PR now a duplicate?

(⊙o⊙)… @JsouLiang and @wangying3426 work on the same team.
This test case is added to demonstrates that the saveLayer is need for cached child layer.

I'm sorry I didn't notice that there were 2 PRs from 2 different authors for this same thing, but #100127 already landed so is this PR now a duplicate?

(⊙o⊙)… @JsouLiang and @wangying3426 work on the same team. This test case is added to demonstrates that the saveLayer is need for cached child layer.

This is a benchmark. Nobody ever sees what it looks like as it is run in an anonymous lab. If this is testing that the right output is generated then this should not be a benchmark, it should be a golden test.

I think I see what is being added here now. Unfortunately, this raises a few issues which I'll address in a new comment.

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

This looks like it is modifying an existing benchmark, but that is usually not a practice we encourage because it will disrupt the tracked history of the existing benchmark. We record the performance of all benchmarks over time so we can look for trends.

One reason to modify a benchmark is if we've made breaking changes and it needs to be updated to match the new APIs. That doesn't apply here.

Another reason to modify a benchmark is if it isn't actually exercising the issues that we originally wanted to address (in other words, the execution of the benchmark misses its mark). I don't think that is the case here, but let me know if this applies.

It looks like the existing benchmark was likely measuring something that we want to keep tracking, so it didn't really warrant any updates.

The new code looks like it is testing more conditions beyond the original benchmark, so it will both break the trends we have recorded for the previous version as well as now have 1 benchmark measuring the performance of a couple of cases. If so, then the different conditions warrant being measured in their own benchmarks so that we don't miss regressions when one of the test cases gets faster while the other gets slower - that could hide performance changes that we want to make visible.

I would like to see a better description of what this benchmark was measuring before these changes and what the new changes are measuring and how they differ from or complement each other.