On Fri, 2 Feb 2024 10:39:49 GMT, Robert Lichtenberger <rlich...@openjdk.org> wrote:
>> Well, the `tableRow` is still set (but before the loop), we just update the >> index in this loop. >> It looks like setting the table row in this loop is not needed, just once >> above. I also don't see that `setTableRow` has any side effects. So maybe >> worth a try. > > You're right. I tried it out and it seems to work. I've tried out an improved version of the patch with cell.updateTableRow(tableRow) moved out of the loop. However, this makes overall performance a bit slower. This patch: > Warmup #0 > Warmup #1 > Warmup #2 > Warmup #3 > Warmup #4 > Run #0: 978 ms. > Run #1: 972 ms. > Run #2: 983 ms. > Run #3: 984 ms. > Run #4: 978 ms. > Run #5: 977 ms. > Run #6: 996 ms. > Run #7: 982 ms. > Run #8: 996 ms. > Run #9: 1044 ms. > Run #10: 1018 ms. > Run #11: 1058 ms. > Run #12: 1016 ms. > Run #13: 1006 ms. > Run #14: 1026 ms. > Run #15: 1029 ms. > Run #16: 1049 ms. > Run #17: 988 ms. > Run #18: 974 ms. > Run #19: 975 ms. > JFX 23-internal+0-2024-02-05-114532 average run time: 1001 vs. "improved version": > Warmup #0 > Warmup #1 > Warmup #2 > Warmup #3 > Warmup #4 > Run #0: 1037 ms. > Run #1: 1030 ms. > Run #2: 1034 ms. > Run #3: 1031 ms. > Run #4: 1033 ms. > Run #5: 1031 ms. > Run #6: 1015 ms. > Run #7: 1015 ms. > Run #8: 1013 ms. > Run #9: 1020 ms. > Run #10: 1013 ms. > Run #11: 1017 ms. > Run #12: 1019 ms. > Run #13: 1022 ms. > Run #14: 1025 ms. > Run #15: 1023 ms. > Run #16: 1024 ms. > Run #17: 1017 ms. > Run #18: 1025 ms. > Run #19: 1023 ms. > JFX 23-internal+0-2024-02-05-114532 average run time: 1023 Since the difference between the two is so small I reran the test with 500_000 rows. This patch: average run time: 9628 "Improved" patch: average run time: 10412 To that end, I'd leave it as is (although the performance differences are admittedly small). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1358#discussion_r1478182155
