Bug 575787 - Optimize Tree.setItemCount
Summary: Optimize Tree.setItemCount
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.22   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 4.23 M1   Edit
Assignee: Alexandr Miloslavskiy CLA
QA Contact: Niraj Modi CLA
URL:
Whiteboard:
Keywords: performance
Depends on:
Blocks:
 
Reported: 2021-09-02 16:37 EDT by Alexandr Miloslavskiy CLA
Modified: 2022-08-04 16:45 EDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandr Miloslavskiy CLA 2021-09-02 16:37:58 EDT
It can be faster if it inserts items at minimum possible position. For example, for an empty TreeItem, it's best to `gtk_tree_store_prepend` instead of `gtk_tree_store_append()`.
Comment 1 Eclipse Genie CLA 2021-09-09 22:08:07 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185276
Comment 2 Eclipse Genie CLA 2021-09-09 22:08:11 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185275
Comment 3 Alexandr Miloslavskiy CLA 2021-09-09 22:29:39 EDT
The speedup for 'Tree.setItemCount()' is pretty dramatic:

     |      Before       |     1 patch       |     2 patches     |
     +---------+---------+---------+---------+---------+---------+
items| REGULAR | VIRTUAL | REGULAR | VIRTUAL | REGULAR | VIRTUAL |
-----+---------+---------+---------+---------+---------+---------+
 10k |   1.44s |   0.99s |   0.17s |   0.18s |   0.14s |   0.13s |
100k | 284.60s | 165.97s |   1.71s |   1.77s |   1.40s |   1.27s |
  1m | ??????? | ??????? |  17.08s |  17.78s |  14.59s |  13.46s |

The patches also improved Table's speed (2x at 100k items).
Comment 4 Alexandr Miloslavskiy CLA 2021-09-10 20:39:01 EDT
I figured that the same idea can be applied to win32 Tree to speed it up as well. I'll do that some week later, need to do something else first.
Comment 7 Lars Vogel CLA 2021-09-15 03:08:00 EDT
Thanks, Alexandr. Is this bug still open? If not please update its status.
Comment 8 Alexandr Miloslavskiy CLA 2021-09-15 18:33:25 EDT
I noticed that win32 Tree could benefit from the same patch. I'm going to implement it somewhere in next 7 days and then I will mark Bug as fixed.
Comment 9 Lars Vogel CLA 2021-09-16 02:19:41 EDT
(In reply to Alexandr Miloslavskiy from comment #8)
> I noticed that win32 Tree could benefit from the same patch. I'm going to
> implement it somewhere in next 7 days and then I will mark Bug as fixed.

Thanks. Every change which make win faster is highly appreciated.
Comment 10 Eclipse Genie CLA 2021-10-31 17:12:58 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187202
Comment 11 Eclipse Genie CLA 2021-10-31 17:12:59 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187203
Comment 14 Alexandr Miloslavskiy CLA 2021-11-08 12:50:55 EST
Niraj, thanks for handling the patches!
Comment 15 Alexandr Miloslavskiy CLA 2021-11-08 12:56:47 EST
Windows part of the patch also achieves dramatic speedup of 'Tree.setItemCount()' :

                |      before patch     |      after patch      |
                +-----------+-----------+-----------+-----------+
                |   REGULAR |   VIRTUAL |   REGULAR |   VIRTUAL |
----------------+-----------+-----------+-----------+-----------+
   10_000 items |   0,44sec |   0,43sec |   0,07sec |   0,06sec |
  100_000 items | 113,83sec | 111,95sec |   0,70sec |   0,63sec |
1_000_000 items |  too long |  too long |   6,82sec |   6,28sec |
Comment 16 Niraj Modi CLA 2021-11-09 02:42:05 EST
(In reply to Alexandr Miloslavskiy from comment #15)
> Windows part of the patch also achieves dramatic speedup of
> 'Tree.setItemCount()' :
> 
>                 |      before patch     |      after patch      |
>                 +-----------+-----------+-----------+-----------+
>                 |   REGULAR |   VIRTUAL |   REGULAR |   VIRTUAL |
> ----------------+-----------+-----------+-----------+-----------+
>    10_000 items |   0,44sec |   0,43sec |   0,07sec |   0,06sec |
>   100_000 items | 113,83sec | 111,95sec |   0,70sec |   0,63sec |
> 1_000_000 items |  too long |  too long |   6,82sec |   6,28sec |

Thanks Alexandr, This is very much appreciated :)

Resolving for M3, any further ideas/improvements are still welcomed but should be tracked in separate bugs.
Comment 17 Niraj Modi CLA 2021-11-11 07:50:28 EST
Marking verified based on comment 15
Comment 18 Sebastian Ratz CLA 2021-11-11 07:55:33 EST
Change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187203 broke the variables and breakpoints view:

The 1st part of the SashForm containing the Tree is not rendered correctly anymore.

I opened bug 577212 for this.
Comment 19 Eclipse Genie CLA 2021-11-11 08:32:22 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187117
Comment 20 Andrey Loskutov CLA 2021-11-11 08:36:39 EST
(In reply to Eclipse Genie from comment #19)
> New Gerrit change created:
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187117

This reverts original commit, as it causes blocker issue for debugging / using IDE on Windows, see bug 577212.

Since original patch caused such a regression, I would propose to postpone with the fix for this bug to 4.23 to get enough test coverage for the *fixed* patch that probably follows.

@Sebastian: thanks for reporting!
Comment 21 Niraj Modi CLA 2021-11-11 09:10:37 EST
(In reply to Andrey Loskutov from comment #20)
> (In reply to Eclipse Genie from comment #19)
> > New Gerrit change created:
> > https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187117
> 
> This reverts original commit, as it causes blocker issue for debugging /
> using IDE on Windows, see bug 577212.
> 
> Since original patch caused such a regression, I would propose to postpone
> with the fix for this bug to 4.23 to get enough test coverage for the
> *fixed* patch that probably follows.
> 
> @Sebastian: thanks for reporting!

+1 Sounds good to me.
Comment 23 Eclipse Genie CLA 2021-11-11 18:15:56 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187659
Comment 24 Alexandr Miloslavskiy CLA 2021-11-11 18:17:16 EST
The mistake was a simple one, early return could have left Tree in a state where drawing is disabled. Fixed that now. Sorry for the trouble!
Comment 26 Niraj Modi CLA 2021-12-07 03:16:12 EST
(In reply to Eclipse Genie from comment #25)
> Gerrit change
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/187659 was
> merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=17b4024759c8084afa680a06b7b2803463e84114

Thanks Alexandr for the revised fix, resolving now.
Comment 27 Niraj Modi CLA 2022-01-06 05:03:06 EST
Verified(including scenario for bug 577212) on Win10 using Build id: I20220106-0000
Comment 28 Jonah Graham CLA 2022-08-04 16:45:50 EDT
As best as I can tell this caused a regression in the Tree. Please see https://github.com/eclipse-platform/eclipse.platform.swt/issues/287#issuecomment-1205748918