Regression in Tree from Bug 575787 #287
Comments
|
Instead of the steps in my original I can reproduce with Java + some other debug environment. By creating 5ish Java breakpoints and 5ish non-Java breakpoints and then toggling "Show Breakpoints Supported by Selected Target" while debugging the Java project will lead to the same problem, with the following exception: and screenshot:
|
Explicit steps that don't need as much setup to experiment with:
|
|
@SyntevoAlex Please have a look. |
niraj-modi
added
regression
|
Will do, but it will probably have to wait a few days. |
I was going to test on Mac but couldn't get past:
I tried adding the update site at |
|
It's in every release train repository: https://download.eclipse.org/releases/2022-06 |
It's certainly there in the repo, but when I try to install from within Eclipse ("Help -> Install New Software") I can only see |
|
@Phillipus Perhaps you typed lsp4j instead of lsp4e? Anyway the mapping from ID to name is not always obvious/known - so here is more step-by-step:
In the install new software it is called "Language Server Protocol client for Eclipse IDE (incubation)" and "Debug Adapter Client for Eclipse IDE (incubation)"*:
Though you may prefer to do it explicitly at the command line with * Perhaps a feature request for "Install New Software" is to know/understand plug-in IDs as typing "org.eclipse.lsp4e" does not give useful results for installing LSP4E |
|
I think this feature includes both those bundles and yes you generally can't choose to install bundles but only features.: |
@merks comment refers to lsp4j - you need lsp4e FWIW The lsp4e bundles are in p2 and can be installed in the gui or command line even though they are not features. |
|
@jonahgraham Thanks for the more detailed explanation. I installed those plugins and am now able to set breakpoints on JS files as well as Java files and these are showing in the Breakpoints view. However, pressing "Show Breakpoints Supported by Selected Target" on and off does nothing. (Testing on Windows before I test on Mac.) |
Did you debug
|
OK, thanks for that. Got it working now. So I can reproduce this on Windows but not Mac. 👍 So, as you say, a Windows issue. |
…exOf to return wrong values Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
…exOf to return wrong values Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
|
Thanks, I was able to reproduce using your steps. I'm working on a patch. |
@SyntevoAlex : do you think you will make it for 4.25 M3? M3 is planned to be this week. |
|
I'm not completely sure. A few other tasks required my attention. I already fixed one problem in the patch, but there is another problem. Seems to be a regression from some other recent patch. I need to investigate it. |
|
@SyntevoAlex : would it be OK to drop optimization then, since this causes a regression? @jonahgraham : how severe the problem is? Should we revert 17b4024 for 4.25 M3? |
|
I understand that it's been 8 months since the patch was merged and the problem is only noticed now. Sounds like it's not too much of a nuisance. |
|
For the problematic use case in CDT it is bad as breakpoints view is busted and requires restarting IDE. However I don't know how many people use that feature vs benefit from optimization. I would lean on leaving it broken in 4.25 over removing the optimization as my understanding is reverting optimization would essentially be a big performance regression? Is there a mitigation we can do in 4.25? Eg can breakpoints view behave differently? |
Not really. The Windows port worked "good enough" for years without this optimization.
We also don't know how many people are affected by other things that do not work anymore, simply because they don't know where the problem is coming from. Waiting 3 months longer for optimization (for performance issue that was there for years) is better as delivering second broken platform release. @SyntevoAlex : could you please provide a clean revert of the code in question for M3? |
|
I will try to fix tomorrow, and if it doesn't work, I'll compose a revert. |
…exOf to return wrong values Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
…exOf to return wrong values Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
…exOf to return wrong values This patch solves two problems after `Tree.setItemCount()` * Unexpected values of `Tree.indexOf()` - was caused by non-coherent cached values in `Tree.lastIndexOf`, `Tree.hLastIndexOf`. This was an oversight in Bug 575787 that optimized `Tree.setItemCount()`. * `SWT.SetData` events were received with the same value in `Event.index` - this was partially fixed years ago in Bug 206806, but it overlooked the case of items that are direct descendants of Tree root. It didn't matter much until `Tree.setItemCount()` was optimized in Bug 575787, which changed how items are inserted. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
|
Please find patch in #318 |
…rong values This patch solves two problems after `Tree.setItemCount()` * Unexpected values of `Tree.indexOf()` - was caused by non-coherent cached values in `Tree.lastIndexOf`, `Tree.hLastIndexOf`. This was an oversight in Bug 575787 that optimized `Tree.setItemCount()`. * `SWT.SetData` events were received with the same value in `Event.index` - this was partially fixed years ago in Bug 206806, but it overlooked the case of items that are direct descendants of Tree root. It didn't matter much until `Tree.setItemCount()` was optimized in Bug 575787, which changed how items are inserted. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
|
Schould be fixed via #335 |
On Windows, Tree maintains its items with a single linked list. For this
reason, "find previous" operation is O(N) because it requires walking
from start of the list.
When inserting an item, it needs to be linked into the list, which
involves finding the previous item and updating its 'next' field, which
is O(N) for one item and O(N*N) for inserting N items.
The workaround is to bulk insert all items at the same position. This
means that if TreeItem had no children, inserting N items into it is
O(N) instead of O(N*N).
On top of that, this patch preallocates 'Tree.items[]' to hold exactly
the required number of items.
Resulting performance improvement on my machine:
| 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 |
This patch combines:
* Original patch for Bug 575787 (the parts that were not reverted later)
* Fix for Issue eclipse-platform#287
* Fix for Issue eclipse-platform#333
* Converting some test snippets into JUnit tests
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
On Windows, Tree maintains its items with a single linked list. For this
reason, "find previous" operation is O(N) because it requires walking
from start of the list.
When inserting an item, it needs to be linked into the list, which
involves finding the previous item and updating its 'next' field, which
is O(N) for one item and O(N*N) for inserting N items.
The workaround is to bulk insert all items at the same position. This
means that if TreeItem had no children, inserting N items into it is
O(N) instead of O(N*N).
On top of that, this patch preallocates 'Tree.items[]' to hold exactly
the required number of items.
Resulting performance improvement on my machine:
| 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 |
This patch combines:
* Original patch for Bug 575787 (the parts that were not reverted later)
* Fix for Issue eclipse-platform#287
* Fix for Issue eclipse-platform#333
* Converting some test snippets into JUnit tests
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
On Windows, Tree maintains its items with a single linked list. For this
reason, "find previous" operation is O(N) because it requires walking
from start of the list.
When inserting an item, it needs to be linked into the list, which
involves finding the previous item and updating its 'next' field, which
is O(N) for one item and O(N*N) for inserting N items.
The workaround is to bulk insert all items at the same position. This
means that if TreeItem had no children, inserting N items into it is
O(N) instead of O(N*N).
On top of that, this patch preallocates 'Tree.items[]' to hold exactly
the required number of items.
Resulting performance improvement on my machine:
| 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 |
This patch combines:
* Original patch for Bug 575787 (the parts that were not reverted later)
* Fix for Issue eclipse-platform#287
* Fix for Issue eclipse-platform#333
* Converting some test snippets into JUnit tests
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
On Windows, Tree maintains its items with a single linked list. For this
reason, "find previous" operation is O(N) because it requires walking
from start of the list.
When inserting an item, it needs to be linked into the list, which
involves finding the previous item and updating its 'next' field, which
is O(N) for one item and O(N*N) for inserting N items.
The workaround is to bulk insert all items at the same position. This
means that if TreeItem had no children, inserting N items into it is
O(N) instead of O(N*N).
On top of that, this patch preallocates 'Tree.items[]' to hold exactly
the required number of items.
Resulting performance improvement on my machine:
| 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 |
This patch combines:
* Original patch for Bug 575787 (the parts that were not reverted later)
* Fix for Issue eclipse-platform#287
* Fix for Issue eclipse-platform#333
* Converting some test snippets into JUnit tests
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
On Windows, Tree maintains its items with a single linked list. For this
reason, "find previous" operation is O(N) because it requires walking
from start of the list.
When inserting an item, it needs to be linked into the list, which
involves finding the previous item and updating its 'next' field, which
is O(N) for one item and O(N*N) for inserting N items.
The workaround is to bulk insert all items at the same position. This
means that if TreeItem had no children, inserting N items into it is
O(N) instead of O(N*N).
On top of that, this patch preallocates 'Tree.items[]' to hold exactly
the required number of items.
Resulting performance improvement on my machine:
| 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 |
This patch combines:
* Original patch for Bug 575787 (the parts that were not reverted later)
* Fix for Issue eclipse-platform#287
* Fix for Issue eclipse-platform#333
* Converting some test snippets into JUnit tests
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
On Windows, Tree maintains its items with a single linked list. For this
reason, "find previous" operation is O(N) because it requires walking
from start of the list.
When inserting an item, it needs to be linked into the list, which
involves finding the previous item and updating its 'next' field, which
is O(N) for one item and O(N*N) for inserting N items.
The workaround is to bulk insert all items at the same position. This
means that if TreeItem had no children, inserting N items into it is
O(N) instead of O(N*N).
On top of that, this patch preallocates 'Tree.items[]' to hold exactly
the required number of items.
Resulting performance improvement on my machine:
| 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 |
This patch combines:
* Original patch for Bug 575787 (the parts that were not reverted later)
* Fix for Issue #287
* Fix for Issue #333
* Converting some test snippets into JUnit tests
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Bug 575787 which optimized
Tree.setItemCountcausesTree.indexOf(TreeItem)to return negative numbers.This issue was originally reported as a bug with CDT in Bug 580518
To Reproduce
At the moment the steps to reproduce involve using CDT on Windows:
I'll try to find time to make a standalone test case. The above test case uses virtual trees + platform debug's async view model which makes it fairly complicated.
Expected behavior
When toggle is on, only 2 breakpoints of project B displayed. When toggle is off, all 7 breakpoints displayed.
Actual behaviour
When toggled back off, the breakpoints view is "corrupted" - see screenshot. There is also an exception in the CDT code due to the index not being in the expected range of >= -1. See bug 580518 for full stack trace.
Screenshots
Environment:
Definitely Windows, can't seem to reproduce on Linux.
Additional OS info (e.g. OS version, Linux Desktop, etc)
JRE/JDK version
JustJ Java 17 as shipped with EPP.
Version since
Since commit 17b4024 - reverting 17b4024 resolves the issue.
The text was updated successfully, but these errors were encountered: