Bug 87467 – [Forms][PerfTest] Performance test for select element rendering

• Component: Forms
• Status: NEW

Rendering of select element is sometimes slow, such as BUG-87466, CR-90094.

We would like to have perf test for select element to detect performance regression.

This bug is preparation for fixing BUG-87466.

Created attachment 143997 [details]
Patch 1

[Ryosuke Niwa]

Comment on attachment 143997 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=143997&action=review

> PerformanceTests/ChangeLog:4
> +        [Forms][PerfTest] Performance test for select element rendering
> +        https://bugs.webkit.org/show_bug.cgi?id=87467

Do we really need to add tests for this? Unlike layout tests, perf tests need to be ran sequentially and hence the cost of adding a new perf test is significantly higher than adding a new layout test. For example, each test that uses runPerSecond would run for at least 18s in total. Here, we're adding 90+ seconds to the perf bots' cycle time. I'm not sure how long these tests take to finish but we have to be careful not to add lots of micro benchmarks since we can write millions of micro benchmarks that won't matter much in practice.

Created attachment 144023 [details]
Patch 2

Yes, we would like to have these perf test for avoiding regression of execution speed.

It seems this performance regression is caused by re-factoring of HTMLOptGroupElement class. If we had had perf test at re-factor time, we could catch it before users saw.

On my linux box (Z600), it takes 58 second to run 16 tests in this patch.

Thanks.

It is hard to guess that changing base class from HTMLFormControlElement to HTMLElement caused performance regression... :-<

[Ryosuke Niwa]

(In reply to comment #5)
> It seems this performance regression is caused by re-factoring of HTMLOptGroupElement class. If we had had perf test at re-factor time, we could catch it before users saw.
> 
> On my linux box (Z600), it takes 58 second to run 16 tests in this patch.

That means we're going to add 2-3 minutes if not 5 minutes on cr-mac perf bots since they're mac minis. That's not an acceptable amount of the cycle time increase for this feature. Performance tests aren't regression tests. We can't have 1,000 perf. tests. It doesn't scale.

[Ryosuke Niwa]

I've talked with tony^work, kling, & anttik about this. I think everyone agrees with me that adding 16 tests for just rendering select element rendering is excessive. We could imagine adding other kinds of form elements and creating a form-control rendering test however.

Also, popup menu, etc... uses completely different code path in DRT so I'm not even sure if these tests are testing what you're intending to test.

[Ryosuke Niwa]

Comment on attachment 144023 [details]
Patch 2

I don't think we want to land this patch as is.