[GTK] Optimize Tree population by removing ID #882

[basilevs]

trafficstars

This is a follow-up to https://github.com/eclipse-platform/eclipse.platform.swt/pull/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 TreeItems from Tree to parent TreeItem, 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 in setText(), 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.

[github-actions[bot]]

Test Results

   553 files  +14     553 suites  +14   1h 13m 27s ⏱️ + 42m 20s  4 376 tests +44   4 364 ✅ +42   12 💤 +3  0 ❌  - 1  16 652 runs  +65  16 540 ✅ +61  112 💤 +5  0 ❌  - 1 

Results for commit 9c0049b5. ± Comparison against base commit 1119bac8.

:recycle: This comment has been updated with latest results.

[basilevs]

014dfc030adaec2fa176b44b276a977f8892d9e7 builds the star shaped virtual tree 700 times slower than master 😞

Update 1: 8da6e4144cabdb9bab71d8f8b4ff736f7e9e28bf fixes the build test to be 6 times slower than master Overall test suite gives measured execution time speed up of 47%. All test timings are listed in a Google Sheet. Further work is needed.

Update 2:

Good results achieved. Following table contains relative execution times for 1b235a9621ec43411cd66a5775f8c56b9387d31c

A script to convert test output to CSV

# %%
from re import compile, Pattern
filename = 'after'

# %%
from dataclasses import dataclass, astuple

@dataclass(kw_only=True)
class Result:
	name: str
	shape: str
	virtual: bool
	short_time: int
	long_time: int
	ratio: float
	degree: float
	

# %%
f = open(filename+'.txt', 'r')
lines = [line.replace('\n', '') for line in f.readlines() ]
print('\n'.join(lines[0:7]))

# %%
chunks = []
chunk = []
for l in lines:
	if not l:
		if chunk:
			chunks.append(list(chunk))
		chunk.clear()
		continue
	chunk.append(l)
if chunk:
	chunks.append(list(chunk))
chunks[32], chunks[30]

# %%
header_pattern:Pattern = compile('(\w+)\[Shape: (\w+), virtual: (true|false)\]')
time_pattern: Pattern = compile('Time for (\d+) elements: ([\d.]+) ns')
ratio_pattern: Pattern = compile('Ratio: ([\d.]+)')
degree_pattern: Pattern = compile('Degree: ([\d.-]+)')
def parse(chunk):
	(name, shape, virtual) = header_pattern.fullmatch(chunk[0]).groups()
	virtual = virtual == 'true'
	(count, short_time) = time_pattern.fullmatch(chunk[2]).groups()
	assert count == '10000'
	(count, long_time) = time_pattern.fullmatch(chunk[3]).groups()
	ratio = float(ratio_pattern.fullmatch(chunk[4]).group(1))
	degree = float(degree_pattern.fullmatch(chunk[5]).group(1))
	return Result(name=name, shape=shape, virtual=virtual, short_time = short_time, long_time = long_time, ratio = ratio, degree=degree )
parse(chunks[0])


# %%
with open(filename+".csv", 'w') as f:
	for chunk1 in chunks:
		try:
			print(*astuple(parse(chunk1)), sep=', ', file=f)
		except AttributeError:
			raise AttributeError(chunk1)

Test	Shape	Virtual	Small tree	Large tree
setForeground	STAR	FALSE	0.66	0.57
jfaceReveal	STAR	FALSE	0.78	0.58
showItem	STAR	FALSE	0.79	0.56
build	STAR	FALSE	0.60	0.53
secondTraverse	STAR	FALSE	0.44	0.18
traverse	STAR	FALSE	0.23	0.11
dispose	STAR	FALSE	1.06	1.28
getForeground	STAR	FALSE	0.98	0.51
setText	STAR	FALSE	0.78	0.55
setForeground	STAR	TRUE	1.10	0.92
jfaceReveal	STAR	TRUE	0.01	0.01
showItem	STAR	TRUE	1.46	0.54
build	STAR	TRUE	0.29	0.57
secondTraverse	STAR	TRUE	0.03	0.15
traverse	STAR	TRUE	0.46	0.42
dispose	STAR	TRUE	0.87	1.29
getForeground	STAR	TRUE	0.31	0.44
setText	STAR	TRUE	1.89	2.03
setForeground	BINARY	FALSE	0.99	0.99
jfaceReveal	BINARY	FALSE	1.33	1.04
showItem	BINARY	FALSE	1.18	0.89
build	BINARY	FALSE	1.07	0.94
secondTraverse	BINARY	FALSE	0.25	0.30
traverse	BINARY	FALSE	0.15	0.25
dispose	BINARY	FALSE	1.33	1.14
getForeground	BINARY	FALSE	0.79	0.83
setText	BINARY	FALSE	0.84	0.97
setForeground	BINARY	TRUE	0.95	0.80
jfaceReveal	BINARY	TRUE	0.47	0.78
showItem	BINARY	TRUE	33.04	0.61
build	BINARY	TRUE	1.08	1.07
secondTraverse	BINARY	TRUE	0.31	0.14
traverse	BINARY	TRUE	0.71	0.75
dispose	BINARY	TRUE	0.94	1.19
getForeground	BINARY	TRUE	0.77	0.79
setText	BINARY	TRUE	1.08	1.09

While most tests show drastic speedup (average measure time is reduced by half), some are slower. I need to investigate setText and dispose

Update 3: Deferred cell procedure after setText and dispose is executing longer, because without indexing during tree building the cell procedure has to call org.eclipse.swt.internal.gtk.GTK.gtk_tree_model_get_path[native] () this is expected - the work is moved around, not increased.

[SyntevoAlex]

Please mention me explicitly when you think it's ready for review and I will try to find time.

[SyntevoAlex]

By the way, would you like to be nominated to have committer rights in SWT? Your contributions look reasonable to me, and SWT really needs more active members. If you accept, I guess I will nominate you after one more contribution.

[basilevs]

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.

[SyntevoAlex]

That's a pity. Still, thanks for your work.

[basilevs]

Please mention me explicitly when you think it's ready for review and I will try to find time.

Will do. I will also remove the Draft flag

[basilevs]

Please mention me explicitly when you think it's ready for review and I will try to find time.

@SyntevoAlex this change is ready for review. Please and thank you. This is a low priority project, so feel free to take your time.

[basilevs]

Failing tests are unrelated.

Error:  Tests run: 185, Failures: 1, Errors: 0, Skipped: 4, Time elapsed: 26.82 s <<< FAILURE! -- in org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser
Error:  org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.test_execute_and_closeListener -- Time elapsed: 15.04 s <<< FAILURE!
java.lang.AssertionError: Either browser.execute() did not work (if you still see the html page) or closeListener Was not triggered if browser looks disposed, but test still fails.
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.test_execute_and_closeListener(Test_org_eclipse_swt_browser_Browser.java:1648)

[basilevs]

Tests have started to fail with an unexpected reason:

Error:  Cannot resolve project dependencies:
Error:    You requested to install 'osgi.bundle; org.eclipse.swt.fragments.localbuild 0.0.0' but it could not be found

Converting back to draft.

[basilevs]

Tests have started to fail with an unexpected reason:

Error:  Cannot resolve project dependencies:
Error:    You requested to install 'osgi.bundle; org.eclipse.swt.fragments.localbuild 0.0.0' but it could not be found

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 https://github.com/eclipse-platform/eclipse.platform.swt/pull/1010 where mvn install step is changed to mvn verify

[SyntevoAlex]

I was instructed to not spend time on this. sorry.

[basilevs]

The build fails with:

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)

Update: fixed in #1114

[akurtakov]

Bump the tests bundle version to x.y.z+100

[basilevs]

Tests are failing with:

java.lang.SecurityException: class "org.eclipse.core.runtime.IAdapterManager"'s signer information does not match signer information of other classes in the same package
	at java.base/java.lang.ClassLoader.checkCerts(ClassLoader.java:1163)
	at java.base/java.lang.ClassLoader.preDefineClass(ClassLoader.java:907)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1015)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
	at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862)
	at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
	at org.eclipse.core.runtime.Platform.getDebugOption(Platform.java:742)
	at org.eclipse.test.performance.Performance.createPerformanceMeterFactory(Performance.java:237)
	at org.eclipse.test.performance.Performance.getPeformanceMeterFactory(Performance.java:227)
	at org.eclipse.test.performance.Performance.createPerformanceMeter(Performance.java:159)
	at org.eclipse.swt.tests.junit.performance.SwtPerformanceTestCase.createMeter(SwtPerformanceTestCase.java:45)
	at org.eclipse.swt.tests.junit.performance.Test_situational.test_imageDrawing(Test_situational.java:226)

I have not touched org.eclipse.core.runtime.IAdapterManager and surprised it is used by SWT tests. What's going on here?

UPDATE: Resolved by rebuild. Thanks @merks

[merks]

Builds before this fix have a problem with split packages with inconsistent signatures:

https://github.com/eclipse-equinox/equinox/pull/685

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.

[basilevs]

Browser tests are failing:

java.lang.AssertionError: Too many (60) leaked file descriptors: [/home/jenkins/.m2/repository/jakarta/el/jakarta.el-api/3.0.3/_remote.repositories, /home/jenkins/.m2/repository/jakarta/el/jakarta.el-api/3.0.3/_remote.repositories, /home/jenkins/.m2/repository/jakarta/servlet/jakarta.servlet-api/4.0.4/_remote.repositories, /home/jenkins/.m2/repository/jakarta/servlet/jakarta.servlet-api/4.0.4/_remote.repositories, /home/jenkins/.m2/repository/javax/servlet/jsp/javax.servlet.jsp-api/2.3.3/_remote.repositories, /home/jenkins/.m2/repository/javax/servlet/jsp/javax.servlet.jsp-api/2.3.3/_remote.repositories, /home/jenkins/.m2/repository/net/bytebuddy/byte-buddy-agent/1.15.3/_remote.repositories, /home/jenkins/.m2/repository/net/bytebuddy/byte-buddy-agent/1.15.3/_remote.repositories, /home/jenkins/.m2/repository/net/bytebuddy/byte-buddy/1.15.3/_remote.repositories, /home/jenkins/.m2/repository/net/bytebuddy/byte-buddy/1.15.3/_remote.repositories, /home/jenkins/.m2/repository/net/bytebuddy/byte-buddy/1.15.3/byte-buddy-1.15.3-sources.jar, /home/jenkins/.m2/repository/org/bouncycastle/bcprov-jdk18on/1.78.1/_remote.repositories, /home/jenkins/.m2/repository/org/bouncycastle/bcutil-jdk18on/1.78.1/_remote.repositories, /home/jenkins/.m2/repository/org/bouncycastle/bcutil-jdk18on/1.78.1/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-apache-jsp/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-apache-jsp/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-nested/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-nested/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-security/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-security/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-servlet/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/ee8/jetty-ee8-servlet/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-http/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-http/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-io/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-io/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-security/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-security/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-server/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-server/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-session/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-session/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-util-ajax/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-util-ajax/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-util/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/jetty-util/12.0.14/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/toolchain/jetty-servlet-api/4.0.6/_remote.repositories, /home/jenkins/.m2/repository/org/eclipse/jetty/toolchain/jetty-servlet-api/4.0.6/_remote.repositories, /home/jenkins/.m2/repository/org/glassfish/jakarta.el/3.0.4/_remote.repositories, /home/jenkins/.m2/repository/org/glassfish/jakarta.el/3.0.4/_remote.repositories, /home/jenkins/.m2/repository/org/hamcrest/hamcrest/2.2/_remote.repositories, /home/jenkins/.m2/repository/org/hamcrest/hamcrest/2.2/_remote.repositories, /home/jenkins/.m2/repository/org/jsoup/jsoup/1.18.1/_remote.repositories, /home/jenkins/.m2/repository/org/jsoup/jsoup/1.18.1/_remote.repositories, /home/jenkins/.m2/repository/org/mortbay/jasper/apache-el/9.0.90/_remote.repositories, /home/jenkins/.m2/repository/org/mortbay/jasper/apache-el/9.0.90/_remote.repositories, /home/jenkins/.m2/repository/org/mortbay/jasper/apache-jsp/9.0.90/_remote.repositories, /home/jenkins/.m2/repository/org/mortbay/jasper/apache-jsp/9.0.90/_remote.repositories, /home/jenkins/.m2/repository/org/ow2/sat4j/org.ow2.sat4j.core/2.3.6/_remote.repositories, /home/jenkins/.m2/repository/org/ow2/sat4j/org.ow2.sat4j.core/2.3.6/_remote.repositories, /home/jenkins/.m2/repository/org/ow2/sat4j/org.ow2.sat4j.pb/2.3.6/_remote.repositories, /home/jenkins/.m2/repository/org/ow2/sat4j/org.ow2.sat4j.pb/2.3.6/_remote.repositories, /home/jenkins/.m2/repository/org/slf4j/slf4j-api/2.0.16/_remote.repositories, /home/jenkins/.m2/repository/org/slf4j/slf4j-api/2.0.16/_remote.repositories, /home/jenkins/.m2/repository/org/slf4j/slf4j-simple/2.0.16/_remote.repositories, /home/jenkins/.m2/repository/org/slf4j/slf4j-simple/2.0.16/_remote.repositories, /home/jenkins/.m2/repository/org/tukaani/xz/1.10/_remote.repositories, /home/jenkins/.m2/repository/org/tukaani/xz/1.10/_remote.repositories, /proc/396/cgroup, /proc/meminfo, /proc/zoneinfo, /tmp/surefire-jenkins/stdout-20241007102403052_13deferred, /tmp/tycho_wrapped_source6905709942349064195.jar, socket:[1699328613]]
	at org.junit.Assert.fail(Assert.java:89)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.reportOpenedDescriptors(Test_org_eclipse_swt_browser_Browser.java:245)
	at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.tearDown(Test_org_eclipse_swt_browser_Browser.java:220)

The same failure happens on master branch