engine icon indicating copy to clipboard operation
engine copied to clipboard

[dart:ui] remove expensive index assertion in Vertices.

Open jonahwilliams opened this issue 1 year ago • 6 comments

Iterating through every index value is really, really expensive. Especially if users are suppllying hundreds of thousands of vertices.

jonahwilliams avatar Jun 25 '24 14:06 jonahwilliams

Where are we expecting this validation to occur then? At some point we have to protect against reading past the ends of the data arrays.

flar avatar Jun 25 '24 17:06 flar

We don't read the indices though, we pass it to the gpu driver. I suppose we should validate that reasonable things happen there.

jonahwilliams avatar Jun 25 '24 17:06 jonahwilliams

verifying on GLES/Vulkan/Metal backend, no crashes. We don't ever read the index data on the CPU

jonahwilliams avatar Jun 26 '24 03:06 jonahwilliams

We should add a test case that using out of range index values works fine if that is the expected behavior.

We may not currently read them on the CPU, but might in the future. One example is the computation of the bounds of the vertices. Currently their bounds are the bounds of all vertices, but if we have 100k vertices and only 100 indices then there is wasted effort there - so we might in the future switch to using the indices for bounds (possibly with a quick test as to whether there are more vertices than indices or vice versa) - at which point we want to make sure that there is a test case that would remind us that OOB indices are specifically allowed...

flar avatar Jun 26 '24 18:06 flar

I can add a test at the DL-impeller level and produces a golden. SG?

jonahwilliams avatar Jun 26 '24 21:06 jonahwilliams

For rendering - as long as it goes through DlBuilder. ui.Canvas is basically a pass through, but starting at Builder we have increasing reasons to inspect the data.

Another thing to verify is constructing the Dart Vertices object itself - the constructor might decide to prune the data at some point if indices is small? (Probably not....? But good to have a test that makes sure it doesn't trip over a bad index just in case)

flar avatar Jun 27 '24 18:06 flar

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #53558 at sha 819f5c075c1dc7c2ea4bb16c13e246df7ea551c0

flutter-dashboard[bot] avatar Jul 05 '24 20:07 flutter-dashboard[bot]

PTAL @flar

jonahwilliams avatar Jul 08 '24 16:07 jonahwilliams

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53558 at sha c863b4f431472c7f7c8adc8fd103be12478ce194

flutter-dashboard[bot] avatar Jul 08 '24 22:07 flutter-dashboard[bot]

Jim is out this week. maybe @jtmcdole PTAL?

jonahwilliams avatar Jul 09 '24 19:07 jonahwilliams

@jonahwilliams Can we land this?

chinmaygarde avatar Jul 11 '24 20:07 chinmaygarde

@chinmaygarde did you mean to close this? We should be able to land it, didn't realize it got reviewed.

jonahwilliams avatar Jul 11 '24 21:07 jonahwilliams

Oh, yeah. Definitely a mistake. Sorry.

chinmaygarde avatar Jul 11 '24 21:07 chinmaygarde

no worries!

jonahwilliams avatar Jul 11 '24 21:07 jonahwilliams