Perform element changes always in a non UI thread #1916
Conversation
...clipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/packageview/PackageExplorerContentProvider.java
Outdated
Show resolved
Hide resolved
|
JavaElementDelta is not thread safe. the initialization may take place after it is used by another thread :-( |
Can you give a concrete example where this can happen? |
|
This test is failing: ContentProviderTests3.testDeleteBottomLevelFragmentFolding Beside this (and missing "system" job flag) the change goes in the right direction. The delta itself should be immutable after sending, so I do not see issues with that, however there might be multiple deltas, and so the dedicated job should be re-scheduled with the updated list of deltas to process, something like org.eclipse.jdt.ui.ProblemsLabelDecorator.AdornmentCalculationJob. |
The members need to be marked final or synchronized |
|
I have poked around a bit more and actually the source seem to be the java editor performs "MakeWorkingCopy" operation and this triggers the an POST CHANGE event and then the whole thing runs inside the context of the Editor I created now to see what happens if we perform it async but it seems all not very optimal here, the Javadoc of the Beside that I get the impression that the actual event is not really interesting for the package explorer anyways, but I have not enough insights to judge if thats the case and how it can be detected earlier. |
|
I think you step back a bit to think what should happen/shown: When the javamodel is not up to date and things are displayed based on an outdated model they are either wrong drawn or need to be drawn again once the model is updated. Wrong data shown is wrong. Draw again may reduce a ui freeze but may in sum perform slower. The only clean solution would be to not show javamodel depended stuff until the model is (asnychronous) updated and then draw the java depended stuff once finished. That can not be archived by just deferring some calls but you would need to explicitly render java independend first. Possibly an explicit "loading..." text instead of showing a wrong (not) formatted sourcecode |
|
Feel free to all implement that, I can only tell that the code here do not is designed to run in the UI, so this is not about presentation (what already is rendered in the UI once the changes are applied). |
|
Fails existing tests... |
This is expected and OK, only one test should be updated. |
|
Maybe someone can pick this up as an idea an polish this, I'm not sure how to best adjust the test or handle the concerns regarding usage of the Delta outside of event notification. |
|
Beside this, this one was the most promising experiment as indeed the UI is actually shown (even though other tasks are still execute in the UI) in contrast to that the UI is not shown before. |
I strongly believe that this is doable and right way to go & encourage you to follow up on this PR. |
laeubi
force-pushed
the
asnyc_exec_update
branch
2 times, most recently
from
02f7174 to
70ffedf
Compare
|
I tried to adjust the one failing test but for me it seems the testcase itself depends on other places on synchronous event execution as well... it even seem that this particular test has a misplaced contruct ... |
...clipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/packageview/PackageExplorerContentProvider.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/packageview/ContentProviderTests3.java
Outdated
Show resolved
Hide resolved
|
@iloveeclipse I think this should be merged before as it seems the main reason for the test failure: |
Currently PackageExplorerContentProvider receives events for changed elements from different threads and already has handling to perform only certain actions in the UI thread. But it does not really account for the case when the event itself is triggered from the UI thread in which case lengthy operation can take place in the UI. This now checks if the change is using the UI and otherwise schedules a job to perform the required actions in the background.
laeubi
force-pushed
the
asnyc_exec_update
branch
from
70ffedf to
f361f4b
Compare
|
I hopefully have now addressed all concerns. |
| public void run() { | ||
| try { | ||
| Job.getJobManager().join(PackageExplorerContentProvider.class, new NullProgressMonitor()); | ||
| } catch (OperationCanceledException | InterruptedException e) { |
There was a problem hiding this comment.
please do not ignore unexpected exceptions but at least log them
There was a problem hiding this comment.
If anything goes wrong here the test will simply fail and one has to investigate that, I don't think that any logmessage is useful here.
I would then assume this will be taken by a JDT developer in advance, this PR is all I can currently offer here and it solves the issue for my testcase. @iloveeclipse |
Currently PackageExplorerContentProvider receives events for changed elements from different threads and already has handling to perform only certain actions in the UI thread. But it does not really account for the case when the event itself is triggered from the UI thread in which case lengthy operation can take place in the UI.
This now checks if the change is using the UI and otherwise schedules a job to perform the required actions in the background.
See for example: