Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

basilevs
Copy link
Contributor

@basilevs basilevs commented Nov 29, 2023

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 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.

Copy link
Contributor

github-actions bot commented Nov 29, 2023

Test Results

   490 files  + 4     490 suites  +4   29m 3s ⏱️ + 20m 58s
 4 166 tests + 7   4 158 ✅ + 7   8 💤 ±0  0 ❌ ±0 
16 418 runs  +28  16 324 ✅ +26  94 💤 +2  0 ❌ ±0 

Results for commit 7691da6. ± Comparison against base commit bb0f8dd.

♻️ This comment has been updated with latest results.

@basilevs
Copy link
Contributor Author

basilevs commented Jan 23, 2024

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

Update 1:
8da6e41 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 1b235a9

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
Copy link
Member

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

@SyntevoAlex
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@basilevs
Copy link
Contributor Author

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 basilevs force-pushed the issue_882_noId branch 4 times, most recently from 15591ee to 3700f42 Compare January 27, 2024 18:33
@basilevs basilevs marked this pull request as ready for review January 27, 2024 19:06
@basilevs
Copy link
Contributor Author

basilevs commented Jan 27, 2024

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
Copy link
Contributor Author

basilevs commented Jan 27, 2024

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 basilevs changed the title [GTK] Optimize TreeItem constructor by removing ID #882 [GTK] Optimize Tree population by removing ID #882 Feb 4, 2024
@basilevs basilevs marked this pull request as draft March 8, 2024 14:37
@basilevs
Copy link
Contributor Author

basilevs commented Mar 8, 2024

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
Copy link
Contributor Author

basilevs commented Mar 8, 2024

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 #1010 where mvn install step is changed to mvn verify

@SyntevoAlex
Copy link
Member

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

@basilevs
Copy link
Contributor Author

basilevs commented Mar 14, 2024

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
Copy link
Member

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

basilevs added a commit to basilevs/eclipse.platform.swt that referenced this pull request Mar 14, 2024
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)
basilevs added a commit to basilevs/eclipse.platform.swt that referenced this pull request Mar 14, 2024
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)
iloveeclipse pushed a commit that referenced this pull request Mar 14, 2024
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)
@basilevs basilevs marked this pull request as ready for review March 14, 2024 13:19
@basilevs
Copy link
Contributor Author

basilevs commented Oct 6, 2024

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
Copy link
Contributor

merks commented Oct 6, 2024

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

eclipse-equinox/equinox#685

image

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
Copy link
Contributor Author

basilevs commented Oct 8, 2024

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

…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%)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants