On Fri, 20 Mar 2026 20:31:46 GMT, Andy Goryachev <[email protected]> wrote:
>> The problem was caused, because the items were added individually instead of >> batched. Here is a performance comparison created with a profiler. >> >> | Before | After | >> | --- | --- | >> | <img width="912" height="489" alt="image" >> src="https://github.com/user-attachments/assets/794314ef-09d6-49e1-bbbe-d2f1a5c97cf2"; >> />|<img width="922" height="497" alt="Screenshot 2026-03-20 202806" >> src="https://github.com/user-attachments/assets/e3a38fde-97dc-4889-a229-3539787c60e9"; >> />| >> >> The tests added here back up the functionality of the choice box itself >> since this is just a performance improvement. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxTest.java > line 610: > >> 608: } >> 609: >> 610: @Test > > +1 for adding tests! > > However, the new tests do not test the issue being fixed in this PR, since > they don't add 1000s of items and no timing checks. > > Speaking of timing checks, how do we meaningfully and reliably measure the > timing in a test? > > For example, in this case we could measure adding 100 items, then set a > threshold at N*10 for 1000 items where N is 2 or maybe 3 to account for > slower access time with large number of items. It is still unclear whether > the test will be reliable enough given different platforms, architectures, > L2/L3 CPU caches etc. > What do you think? I don't think there is a good way to test that. In the past, we instead made sure that all the branches are covered by tests that were optimized. We could probably measure the timing and have an arbitrary value we check against. But I'm not sure this is really needed. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2118#discussion_r2968200835
