-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GTK] Optimize Tree population by removing ID #882 #918
base: master
Are you sure you want to change the base?
Conversation
74b9607
to
f5b8eb0
Compare
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/TreeItem.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/TreeItem.java
Outdated
Show resolved
Hide resolved
014dfc0 builds the star shaped virtual tree 700 times slower than master 😞 Update 1: Update 2: Good results achieved. Following table contains relative execution times for 1b235a9 A script to convert test output to CSV
While most tests show drastic speedup (average measure time is reduced by half), some are slower. I need to investigate Update 3: |
Please mention me explicitly when you think it's ready for review and I will try to find time. |
By the way, would you like to be nominated to have committer rights in SWT? |
No I only intend to get to linear execution times of SWT Tree and do not intend to be involved afterwards. |
That's a pity. |
Will do. I will also remove the Draft flag |
15591ee
to
3700f42
Compare
@SyntevoAlex this change is ready for review. Please and thank you. |
Failing tests are unrelated.
|
Tests have started to fail with an unexpected reason:
Converting back to draft. |
This MR only changes java files, which means, that dependencies can't be changed. I conclude that build failure cause is external. I assume the build is broken by #1010 where |
I was instructed to not spend time on this. sorry. |
70b25e9
to
d4fdded
Compare
The build fails with:
Update: fixed in #1114 |
Bump the tests bundle version to x.y.z+100 |
This is needed to fix builds in eclipse-platform#918 The builds were failing with message: Only qualifier changed for (org.eclipse.swt.tests/3.107.300.v20240313-1307). Expected to have bigger x.y.z than what is available in baseline (3.107.300.v20240227-1638)
This is needed to fix builds in eclipse-platform#918 The builds were failing with message: Only qualifier changed for (org.eclipse.swt.tests/3.107.300.v20240313-1307). Expected to have bigger x.y.z than what is available in baseline (3.107.300.v20240227-1638)
This is needed to fix builds in #918 The builds were failing with message: Only qualifier changed for (org.eclipse.swt.tests/3.107.300.v20240313-1307). Expected to have bigger x.y.z than what is available in baseline (3.107.300.v20240227-1638)
d4fdded
to
b077ffc
Compare
b077ffc
to
274078e
Compare
Tests are failing with:
I have not touched org.eclipse.core.runtime.IAdapterManager and surprised it is used by SWT tests. UPDATE: |
Builds before this fix have a problem with split packages with inconsistent signatures: I'm not sure what SWT is build against/with. The last build with the problem is I20241003-1800 (which is still part of the composite) while the problem is fixed in I20241004-1800 and later. |
Browser tests are failing:
|
…tform#882 The ID system tracking association of every TreeItem with GTK model entry had been using GTK.gtk_tree_store_set() to persist a key in GTK model. This operation has linear execution time over model size. IDs were replaced with tree paths and strong references, which, if implemented right, have logarithmic execution time on balanced trees and constant execution time on wide trees. Performance improvement is proven by running org.eclipse.swt.tests.junit.performance.Test_org_eclipse_swt_widgets_Tree.jfaceReveal() In test jfaceReveal[Shape: STAR, virtual: true]: 10_000 elements: 141_061_117ns -> 4_369_889ns 100_000 elements: 10_761_449_641ns -> 181_898_611ns (-98%)
45acd41
to
7691da6
Compare
This is a follow-up to #904, a third out of four optimizations suggested in #882.
The prototype illustrating the performance problem is in #908. There, after obvious bottlenecks are eliminated, ID system unavoidably takes significant execution time. It proves that without its removal further improvement is impossible.
Disclaimer
SWT Tree uses ID association to track parent-child relationship, associate GTK and SWT models, lazy TreeItem population, etc. Therefore its removal is a significant modification to SWT GTK. However 360x speedup of large tree population demonstrated in a prototype for #882 justifies any drastic measures.
Problem
Tree had been using
GTK.gtk_tree_store_set()
to persist a key (ID) of TreeItem in underlying GTK model. The ID is set each time a TreeItem is hydrated (either explicitly accessed, or rendered on screen). This operation has linear execution time over model size, leading to quadratic tree population times.Solution
Replace IDs with tree paths and strong Java references, which, if implemented right, produce logarithmic tree population time for balanced trees and linear population time for wide trees. This necessitates moving ownership of
TreeItem
s fromTree
to parentTreeItem
, mimicking approach taken in MacOS implementation.Performance improvement is proven by running
org.eclipse.swt.tests.junit.performance.Test_org_eclipse_swt_widgets_Tree.jfaceReveal()
this test creates a large Tree and reveals its last node.In test jfaceReveal[Shape: STAR, virtual: true]:
10_000 elements: 141_061_117ns -> 4_369_889ns
100_000 elements: 10_761_449_641ns -> 181_898_611ns (-98%)
Caveats
Tree traversal is a bit slower after this change. This is unavoidable, as now instead of an indexed lookup to access a TreeItem, a full descent from Tree root is required. However, this is not a regression - the work done during Tree population moved to traversal time. Test scenarios that involve both building and traversal are still 2x - 50x faster.
Next step
GTK.gtk_tree_store_set()
and similar in performance methods are used insetText()
,setForeground()
,setBackground()
and other setters. The setters are eager and synchronous, meaning that they access GTK model even if the element in question is not visible on screen. To eliminate this complexity, I suggest to lazify these methods and only modify GTK model when necessary (when elements are shown on screen) in a manner similar to how VIRTUAL tree delays the data fetching until last possible moment. The prototype has shown that with this final step, linear Tree population times can be finally achieved.