[GTK] Optimize TreeItem constructor #882 #903

basilevs · 2023-11-26T17:14:42Z

This eliminates a single hotspot. The full complexity for tree building is still quadratic.

Profile before the fix:

Profile after the fix:

SyntevoAlex · 2023-11-26T17:50:04Z

I would prefer some numbers here in the commit message.
For example:

when inserting 100_000 items into the Tree using this code

Tree tree = new Tree(shell, SWT.VIRTUAL);
for (int i = 0; i < 100_000; i++)
{
    TreeItem item = new TreeItem(tree, 0);
    item.setText("item#" + i);
}

the speedup on my machine is 99.99sec -> 88.88sec

SyntevoAlex

With these small issues fixed, I am willing to merge this PR.

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java

SyntevoAlex · 2023-11-26T18:19:06Z

Please also rename commit to have [GTK] prefix, because it only affects GTK. Also, the commit message is poor in the usual way: it answers what instead of why. A better commit message header would be:
[GTK] Tree optimization: speedup TreeItem constructor by using known iterator

SyntevoAlex

Now that this commit has more issues, I would rather remove it from PR for now.

SyntevoAlex · 2023-11-26T18:24:39Z

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/AllWidgetTests.java

@@ -33,6 +33,7 @@
 		Test_org_eclipse_swt_widgets_List.class, Test_org_eclipse_swt_widgets_Text.class,
 		Test_org_eclipse_swt_widgets_ScrollBar.class, Test_org_eclipse_swt_widgets_Sash.class,
 		Test_org_eclipse_swt_widgets_Tree.class, Test_org_eclipse_swt_widgets_TabFolder.class,
+		org.eclipse.swt.tests.junit.performance.Test_org_eclipse_swt_widgets_Tree.class,


It is not appropriate to put performance tests into the main tests package.
Unit tests should run as fast as possible, and performance tests are slow by design.

Where should I put these tests to run them on CI?

To my knowledge, there is no such place currently.
You can introduce new GitHub job maybe. But frankly I would rather not have it at all. Once problems are fixed, they are unlikely to come back. Yet you are going to burn hours of CPU time over years.

Can leave them in for now and disable before merge to master?

You can keep whatever you want on your branch.
You can create another PR for testing.
If you want this PR to be merged, remove tests.

Removed the test from all suites for now.

I've investigated disabled performance tests and the problems "have came back". The test createComposites

eclipse.platform.swt/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/performance/Test_situational.java

Line 72 in 02c8a22

public void test_createComposites() {

executes in seconds on Windows and Linux, but takes 28 minutes on Mac OS. It uses a disabled assertions library so would never fail even if it was enabled on CI though.

I've created a bug #912

...s/JUnit Tests/org/eclipse/swt/tests/junit/performance/Test_org_eclipse_swt_widgets_Tree.java

SyntevoAlex · 2023-11-26T18:33:05Z

With small fixes, commit with the fix is good to merge.
As for the commit with tests, I would rather remove it from PR for now.

I have used up my (rather small) time to look at someone else's PRs this week, so I could only review the next batch in a few days. Feel free to ask someone else to review, but it's not going to be too easy because there're very few active members currently.

If you polish the first commit and don't introduce new problems, I can merge it when I see it.

basilevs · 2023-11-26T19:18:12Z

I would prefer some numbers here in the commit message. For example:

when inserting 100_000 items into the Tree using this code
Tree tree = new Tree(shell, SWT.VIRTUAL);
for (int i = 0; i < 100_000; i++)
{
    TreeItem item = new TreeItem(tree, 0);
    item.setText("item#" + i);
}
the speedup on my machine is 99.99sec -> 88.88sec

New commit message:

[GTK] Tree optimization: speedup TreeItem constructor by using known iterator #882

This change eliminates a single hotspot. The overall complexity of
tree building and element reveal is still quadratic.

Performance improvement is proven by running
org.eclipse.swt.tests.junit.performance.Test_org_eclipse_swt_widgets_Tree.jfaceReveal()

Before the change:
jfaceReveal[Shape: STAR, virtual: true]
Execution time should grow as 1.100000 degree polynom.
Time for 10000 elements: 620682717.000000 ns
Time for 100000 elements: 62804214489.000000 ns
Ratio: 10.118570
Degree: 2.005119

After the change:
jfaceReveal[Shape: STAR, virtual: true]
Execution time should grow as 1.100000 degree polynom.
Time for 10000 elements: 501378662.000000 ns
Time for 100000 elements: 52781899817.000000 ns
Ratio: 10.527353
Degree: 2.022319

Execution time is decreased by 16% for reveal of 100000 elements.

SyntevoAlex · 2023-11-26T19:21:11Z

You still have spaces changed in _getItem.

SyntevoAlex · 2023-11-26T19:21:53Z

Also, a nitpick: you added unneeded noise in commit message.
Instead of this

Before the change:
jfaceReveal[Shape: STAR, virtual: true]
Execution time should grow as 1.100000 degree polynom.
Time for 10000 elements: 620682717.000000 ns
Time for 100000 elements: 62804214489.000000 ns
Ratio: 10.118570
Degree: 2.005119

After the change:
jfaceReveal[Shape: STAR, virtual: true]
Execution time should grow as 1.100000 degree polynom.
Time for 10000 elements: 501378662.000000 ns
Time for 100000 elements: 52781899817.000000 ns
Ratio: 10.527353
Degree: 2.022319

I would write

In test jfaceReveal[Shape: STAR, virtual: true]:
10_000 elements: 620_682_717ns -> 501_378_662ns
100_000 elements: 62_804_214_489ns -> 52_781_899_817ns

Can you see how much easier it is to understand without the noise?
But this is just a rant, I would be pleased to have it changed, but this is not required.

basilevs · 2023-11-26T19:33:57Z

You still have spaces changed in _getItem.

Added spaces before opening parentheses. Hard to spot. Is there a formatter configuration? I could not find one in contribution guide.

basilevs · 2023-11-26T19:35:16Z

Also, a nitpick: you added unneeded noise in commit message. Instead of this
I would write

New message:

[GTK] Tree optimization: speedup TreeItem constructor by using known
iterator #882

This change eliminates a single hotspot. The overall complexity of
tree building and element reveal is still quadratic.

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: 620_682_717ns -> 501_378_662ns
100_000 elements: 62_804_214_489ns -> 52_781_899_817ns (-16%)

SyntevoAlex · 2023-11-26T19:36:52Z

Unfortunately you still have spaces changed.
I don't know about formatter configuration. Use some git client that can show edited spaces well. I use SmartGit.

basilevs · 2023-11-26T19:40:13Z

Unfortunately you still have spaces changed.

Fixed two more.

SyntevoAlex · 2023-11-26T19:40:15Z

Oh well. So that you're not upset by spaces, I'll forget it in this commit.
I will however ask you to not change codestyle unnecessarily next time.

SyntevoAlex · 2023-11-26T19:40:43Z

Once CI finishes testing this PR, I will merge it.

iloveeclipse · 2023-11-26T19:41:21Z

Just in case: master is not open yet for merging.

SyntevoAlex · 2023-11-26T19:43:28Z

Ahh, I always forget about that. Thanks for reminding.
Do I understand it correctly that we'll need to wait until 4.30 is released?

basilevs · 2023-11-26T19:44:51Z

@SyntevoAlex thanks for the review.

@iloveeclipse I've considered merging this in #902 meanwhile, but confused everyone with workflow discussion, so I'm not sure anymore 🤷

iloveeclipse · 2023-11-26T19:45:47Z

We have to wait till the official mail saying "master is open".
Right now release related code is prepared for next version, build jobs created etc.

SyntevoAlex · 2023-11-26T19:46:22Z

I hope I didn't upset you too much with all the requests. Now that you understand SWT things a bit better, next PR will likely require fewer changes.

SyntevoAlex · 2023-11-26T19:47:27Z

@iloveeclipse while you're around, can I ask you about assert usage in SWT?
This PR introduces another assert which is the... third! Do you have any ideas why it was avoided before?

basilevs · 2023-11-26T19:48:17Z

The requests were sensible. The workflow takes some getting used to.

basilevs · 2023-11-26T19:50:25Z

@iloveeclipse while you're around, can I ask you about assert usage in SWT? This PR introduces another assert which is the... third! Do you have any ideas why it was avoided before?

eclipse.platform.swt/bundles/org.eclipse.swt/Readme.md

Lines 36 to 47 in 518769d

    
           Using Assertions: 
        
           ---------------- 
        
           Assertions are added to the code. These don't run in production, but they do when: 
        
           * JUnits are ran, they turn on assertions by default. 
        
           * If you run a java run configuration and add '-ea' to the 'VM Arguments' 
        
           Assertions look like: 
        
           	assert expression ; 
        
           	assert expression : msg ; 
        
           See: https://docs.oracle.com/javase/8/docs/technotes/guides/language/assert.html

github-actions · 2023-11-26T19:50:34Z

Test Results

    299 files ±0     299 suites ±0 6m 8s ⏱️ +32s
  4 095 tests ±0   4 087 ✔️ ±0   8 💤 ±0 0 ❌ ±0
12 197 runs ±0 12 124 ✔️ ±0 73 💤 ±0 0 ❌ ±0

Results for commit f5edd67. ± Comparison against base commit ffbacfd.

♻️ This comment has been updated with latest results.

SyntevoAlex · 2023-11-26T19:50:50Z

@basilevs you can now preserve this PR's git branch as is until it's merged.
You can now create another PR (but I won't review it until a few days after because I've hit my limit).
You can rebase the other PR on top of this branch (so that it will contain copies of commits from this PR).
When this PR is merged, the other PR will become based on master.

iloveeclipse · 2023-11-26T19:54:56Z

@iloveeclipse while you're around, can I ask you about assert usage in SWT? This PR introduces another assert which is the... third! Do you have any ideas why it was avoided before?

I can't say why it wasn't used but I personally prefer not to use it too, so I would prefer we would not introduce it. I've not seen yet code that would really need that ancient C feature that was by mistake added to Java.

Thinking more about it, I believe SWT was created before this mistake happened to Java.

SyntevoAlex · 2023-11-26T19:56:28Z

Why do you consider it a mistake?
I myself like to add asserts whenever appropriate, and they actually catch quite a few bugs,

iloveeclipse · 2023-11-26T20:02:41Z

In most places I saw an assert it was a lazy way to say "actually we should check that and somehow handle possible wrong flow, but let it go wrong in production code, it can't really happen". So either it can't happen and assert is not needed, or the code will run with wrong state in production with side effects we haven't tested, because in tests we use assertions and flow is different anyway. So either one uses defensive code style, and no assertions needed because all possible code paths are handled by regular code flow, or one uses assertions.

SyntevoAlex · 2023-11-26T20:08:56Z

Yes I can understand this logic.

However, for every possible assert location, let's estimate the likelihood of things going wrong.
If likelihood is high, then proper error handling code is needed. But handling errors properly is difficult, so there's a cost with it.
If likelihood is low, yet still your gut feeling is worried, that's a good place for assert. It's better than nothing, and cheap enough.
If likelihood is next to zero, then it's fine to not add any handling.

So my point is that there are many cases where paying the cost for proper handling is not justified because the problem seems to be very unlikely, yet still I'm not 100% sure, so I add an assert.

laeubi · 2023-11-27T05:45:32Z

If I assume/assert something can't happen I throw AssertionError with a message why I think it can't happen, that is independent of if assertions are enabled or not if throwing an exception is not appropriate. Right after that I most of the time delete it and handle it in a proper way... because according to Murphy's law "Anything that can go wrong will go wrong, and at the worst possible time." :-)

SyntevoAlex · 2023-11-28T17:01:02Z

master is not open. Please rebase your PR onto master so that tests are run, then I can merge it.

SyntevoAlex · 2023-11-28T17:01:59Z

As a result of the rebase, your commit Merge branch 'master' into issue_882_useKnownIter shall disappear.

SyntevoAlex · 2023-11-28T17:03:30Z

In the future, don't merge master into your branch. Instead, rebase your branch onto master.

Testing asymptotic execution times for operations over large trees.

iterator eclipse-platform#882 This change eliminates a single hotspot. The overall complexity of tree building and element reveal is still quadratic. 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: 620_682_717ns -> 501_378_662ns 100_000 elements: 62_804_214_489ns -> 52_781_899_817ns (-16%)

SyntevoAlex · 2023-11-28T19:48:31Z

In master now. Thanks for your contribution!

You can now rebase your other branch onto master.

basilevs · 2023-11-30T18:09:19Z

Next optimization step is #904

eclipse-platform#903

Add spaces before parentheses and brackets The defects are mentioned in eclipse-platform#903

basilevs mentioned this pull request Nov 26, 2023

TreeItem.getItemCount() and TreeItem.getItem(int) have linear complexity on Linux #882

Open

SyntevoAlex requested changes Nov 26, 2023

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java Show resolved Hide resolved

SyntevoAlex reviewed Nov 26, 2023

View reviewed changes

basilevs force-pushed the issue_882_useKnownIter branch from 9ff75d4 to df8e738 Compare November 26, 2023 19:02

basilevs force-pushed the issue_882_useKnownIter branch from df8e738 to 84484eb Compare November 26, 2023 19:31

SyntevoAlex approved these changes Nov 26, 2023

View reviewed changes

basilevs force-pushed the issue_882_useKnownIter branch from 84484eb to 466cb37 Compare November 26, 2023 19:38

basilevs marked this pull request as ready for review November 26, 2023 19:41

basilevs requested a review from sravanlakkimsetti as a code owner November 26, 2023 19:41

basilevs mentioned this pull request Nov 27, 2023

[GTK] Speedup TreeItem retrieval by removing redundant iterations #882 #904

Merged

basilevs changed the title ~~Do not iterate over siblings in TreeItem constructor #882~~ [GTK] Optimize TreeItem constructor #882 Nov 27, 2023

This was referenced Nov 27, 2023

Enable PerformanceTests #882 #906

Merged

[GTK] Optimize TreeItem.getItem(int) by caching TreeItem.getItems() #882 #908

Closed

laeubi added the performance Performance issue label Nov 27, 2023

basilevs added 2 commits November 28, 2023 23:21

Performance tests for Tree widget eclipse-platform#882

4457bb8

Testing asymptotic execution times for operations over large trees.

basilevs force-pushed the issue_882_useKnownIter branch from 05d3d5e to f5edd67 Compare November 28, 2023 19:21

SyntevoAlex merged commit 16d0ae0 into eclipse-platform:master Nov 28, 2023
13 checks passed

basilevs deleted the issue_882_useKnownIter branch November 28, 2023 19:51

basilevs mentioned this pull request Nov 30, 2023

Feature branch - faster GTK Tree #882 #902

Closed

basilevs added a commit to basilevs/eclipse.platform.swt that referenced this pull request Jan 22, 2024

This fixes some of style defects mentioned in

9f8c509

eclipse-platform#903

basilevs mentioned this pull request Jan 22, 2024

[GTK] Add missing spaces for consistent style #996

Closed

basilevs added a commit to basilevs/eclipse.platform.swt that referenced this pull request Jan 22, 2024

[GTK] Fixes style defects

addf7d8

Add spaces before parentheses and brackets The defects are mentioned in eclipse-platform#903

[GTK] Optimize TreeItem constructor #882 #903

[GTK] Optimize TreeItem constructor #882 #903

Conversation

basilevs commented Nov 26, 2023 • edited Loading

SyntevoAlex commented Nov 26, 2023

SyntevoAlex left a comment

Choose a reason for hiding this comment

SyntevoAlex commented Nov 26, 2023

SyntevoAlex left a comment

Choose a reason for hiding this comment

SyntevoAlex Nov 26, 2023

Choose a reason for hiding this comment

basilevs Nov 26, 2023

Choose a reason for hiding this comment

SyntevoAlex Nov 26, 2023

Choose a reason for hiding this comment

basilevs Nov 26, 2023

Choose a reason for hiding this comment

SyntevoAlex Nov 26, 2023

Choose a reason for hiding this comment

basilevs Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

basilevs Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

SyntevoAlex commented Nov 26, 2023

basilevs commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

basilevs commented Nov 26, 2023 • edited Loading

basilevs commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

basilevs commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

iloveeclipse commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

basilevs commented Nov 26, 2023

iloveeclipse commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

basilevs commented Nov 26, 2023

basilevs commented Nov 26, 2023

github-actions bot commented Nov 26, 2023 • edited Loading

Test Results

SyntevoAlex commented Nov 26, 2023

iloveeclipse commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023

iloveeclipse commented Nov 26, 2023

SyntevoAlex commented Nov 26, 2023 • edited Loading

laeubi commented Nov 27, 2023

SyntevoAlex commented Nov 28, 2023

SyntevoAlex commented Nov 28, 2023

SyntevoAlex commented Nov 28, 2023

SyntevoAlex commented Nov 28, 2023

basilevs commented Nov 30, 2023

basilevs commented Nov 26, 2023 •

edited

Loading

basilevs Nov 26, 2023 •

edited

Loading

basilevs Nov 27, 2023 •

edited

Loading

basilevs commented Nov 26, 2023 •

edited

Loading

github-actions bot commented Nov 26, 2023 •

edited

Loading

SyntevoAlex commented Nov 26, 2023 •

edited

Loading