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

Reply via email to