Defer initialization of classpath to the background if called from UI #1525
Conversation
|
I'm very happy to see that you working on the startup performance of Eclipse @laeubi Thank you. |
Test Results 275 files - 10 275 suites - 10 39m 27s ⏱️ - 11m 21s For more details on these failures and errors, see this check. Results for commit 588fad8. ± Comparison against base commit 5baaa6d. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Thank you Christoph for working on this issue, I'm very happy about that as well.
But the presented solution does seem to be very robust and I don't think pde.core should handle issues that arise from wrong usage in the UI. Therefore I would prefer a solution in the UI classes where this is caused. E.g. the editor should defer the initialization to a background thread. I would like to have this only as a last resort solution.
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/RequiredPluginsInitializer.java
Show resolved
Hide resolved
It is not nice but reflects reality in Eclipse IDE runtime environment and we can quite easy remove it once a better solution is there. Also there are sadly numerous places (and maybe some unknown places) where this happens and at least we get a warning in the log now when new things arise. |
|
Interestingly the warning even triggers in the CI build already :-D |
|
Thanks for working on the issue. But i think one should not make decisions based on thread name. Better solve it in the caller. |
Will you propose PR to do so for JDT? And as mentioned before the worst that can happen is it will work as before, so there is no real "danger" in this decision. |
no, but that is no reason to introduce dirty hacks. |
This is not a dirty "hack" it is a workaround for the situation that is is very unlikely that changes will occur any time soon in an unrelated project (JDT) as it is "understaffed" (as every other project as well... So if we can improve the situation quite easy and make it better for all users of PDE there is no reason to wait for something that maybe never happens. And even if it does, we can easily remove the code again without any problem. |
|
I agree that we should apply this PR. PDE should protect itself from bad callers which seems to be in this case JDT. |
I'm not sure you understand possible implications of that PR. JDT will see classpath containers that are not fully initialized or use containers that can't be initialized at all. This may lead to unpredictable plugin compilation results. Please check I agree with @jukzi this is a dirty hack and dangerous one too. |
When the computation is done, we notify JDT that something has changed and then it will simply reevaluate. Also as compilation does not happen in the UI thread for this case nothing changes at all, this all calling from the UI is as far as I can see only to inspect the classpath entries and the init of the container is just a side-effect. Also as you mention the javadoc it says:
So it would be even valid to do nothing and simply return, I just wanted to be a little bit nice here ;-) I would be happy to not require this so I suggest anyone who feels uncomfortable with this put a breakpoint in
and fix all call sites that enter her in the main (== UI thread) when you start an eclipse, sadly those are very deep nested so its not trivial, and obviously those are not compile threads in any case. |
|
I now have made some more tests:
but the container is there because at UI startup the init of container is triggered automatically. So an alternative could be to simply throw an exception... |
There was a problem hiding this comment.
I now have made some more tests:
1. Simply return when called from UI Thread --> The Plugin Container is not added (as expected) unless one changes something from the Manifest or something else so PDE set it again 2. Current solution I see the container is set due to the Job I started (like if someone changes the manifest or similar) 3. Throw an exception, then I see
I still think this should be solved in JDT, i.e. the Java Editor should defer the initialization if called from the UI thread. Since it's a UI component it can also test if the main thread is really the UI thread. I'm really not inclined to workaround bad behavior of other UI parts in the core of PDE.
Concurrency in the intialization of JDT is already a problem and introducing more of it probably doesn't make it better. One also has to consider headless apps that just run in the main thread. They will also be affected of this change.
Therefore I want to ask the JDT committers again to help here as they know JDT better and propose a solution for JDT UI.
|
I have now added a check that uses dynamic import package + reflection to check if it is called from the (SWT) UI thread, so there is no need to check for the thread name anymore. I'll wait if the JDT issue(s) are solved until next week (13.01.2025) and then plan to merge this to unblock PDE, the IDE and its users from UI blocking during startup phase. |
Please either abstain from reviews or leave actionable comments, this is not a hack but common practice to use reflection if direct calls are not possible or not wanted (due to coupling). Dynamic Import Package is also a standard OSGi technology for late binding of optional imports. |
the only legit action is to close this PR as there is no legit way to fix it in pde. any descision made here based on the caller or calling thread is just too bad. |
laeubi
dismissed
jukzi’s stale review
Not interested in providing reasonable review.
laeubi
changed the title
There was a problem hiding this comment.
My review was requested. It is hard to decide what to do here for me. @laeubi maybe send an email to the JDT dev list so that the team can decide if they can fix it. If not, I would suggest to bring it to the PMC to discuss.
|
@vogella I added you as reviewer as you are left a comment that one can interpret as a positive review but you haven't given explicit review here yet. So if you don't care (anymore) feel free to remove you from the list of reviewers again. Beside that @HannesWell has already expressed positive feedback here but (valid) concerns where expressed about how to check for an UI thread. This is now addressed with a check that is done hundred of times in our platform code (but because we do not want direct UI binding in an optional non intrusive way). Still @HannesWell and me agree that fixing it "at the root" would be preferable of course, but now after 4 weeks I don't see much hope of any progress here and the current situation is simply unacceptable as it potentially blocking the IDE startup when one has package-explorer/java editor in an IDE (what we can assume as a very common use-case here). And as PDE and JDT are distinct project there is nothing from any PMC perspective PDE can do to "force" movement in JDT, so having a "grace-period" of just another week seems fair to me instead of waiting an undefined amount of time for "something" to happen. If this changes any time we can easily remove the workaround at any time, because there is no API involved or any behavior change that would influence valid callers so there is really no need to worry even tough we all would wish a better alternative. |
|
I'm still interested and I also still share the same opinion (PDE should protect itself from bad callers which seems to be in this case JDT.). @HannesWell is PL in PDE so he can make the cut-off decision if two committers (you and @jukzi) disagree. If someone disagrees with this decision he can escalate this to the PMC. |
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/RequiredPluginsInitializer.java
Outdated
Show resolved
Hide resolved
Thank you for the update. I think the code is less 'dangerous' now, but yes I'm still strongly favor of fixing this at the root in JDT. I consider this as a really last resort solution. Async work can already be a problem now and introducing more (even just for special cases) can potentially cause more problems. Therefore I can just repeat myself and ask the JDT committer that are active in this discussion to help here so that we don't have to decide if it's better to not fix the problem to to apply this change.
I wouldn't sign this statement in general. While it's nice to protect callers from bad behavior it's practically not feasible because it would blow up a lot of code-parts if done everywhere. And what's right and what's wrong might also depend on the exact case (although I cannot tell if there is a reasonable use-case for calling the code discussed from the UI thread, but who knows...).
We can also have a vote on this among all PDE committers. |
|
I support this PR. justification: in the absence of a better alternative, a working solution to the stated problem (deadlock) with no known side effects in the common code path is reasonable. people with concerns on (real / imaginary) side effects are best placed to refine the code base through future PRs. |
Currently there are some bad behaving UI components in the eclipse IDE that trigger resolving of the classpath containers in the process of creating the UI, this leads to very bad startup performance and even deadlocks in startup. This now detects the issue, logs a warning of the offending component and defer the initialization of classpath to a background job, this currently decrease time from starting eclipse until UI is shown noticeable. Fix eclipse-pde#1481
|
I don't think a vote would be the right thing to do as this change does not affect PDE project as a whole. A simple PL decision by @HannesWell and @vik-chand would be enough. Regarding the concern about "async" operation, a classpath container can change any time and it does for example if you edit the manifest, so this is really nothing to worry about and just usual usage. |
|
In case we are voting: +1 from me for this change (assuming JDT cannot / doesn't want to fix their code) |
|
The whole eclipse plugin concept was designed such that an RCP app could be render a skeleton before any plugin code is executed. The plugin.xml was choosen to be a text file such that it was not needed to run any java code while already knowing which ui parts they would contribute. Somewhen that good initial idea was violated and the plugin code was executed before the workbench is shown. I do not know when or why it started but my best guess is that it is the e4.compatibility.CompatibilityEditor which got it wrong. So while it may be possible to be fixed in JDT it would be even better to solve it e4 for all UI plugins somewhere here: |
|
e4.compatibility.CompatibilityEditor is only a small wrapper to call the inherited methods in the 3x based editors. |
|
e4 calls it a feature not a bug: |
Sounds like you are mixing stuff here. The lazy instantiation feature applies to editors and views which are currently not visible / selected in a stack. So if you have 300 Java editors open in the editor area not all are intialized at startup but only the active one. This was also the case in the 3.x time and is one of the reason which the startup time of Eclipse is not linear dependent of the number of editors / views open. |
Well, we had lot of vacations in this time. Now we see some signs of life in JDT: So assuming the offending call would be removed we would not need this PR. @laeubi : would you please push a PR to JDT core with this line removed, and assuming we would not have any JDT test fails, we can try to merge this for M1 and see which possible side effects in PDE/m2e/Xtext will arise. |
|
Even without this call there is still one issue left in jdt ui: also platform has a pending issue: |
Would still makes sense to push the PR to JDT core I think. Are you planning to push it or shall someone else do it? I'm surprised that @jarthana did not push a PR as he already had the change in his workspace. |
So what? This shouldn't be an excuse to add workarounds in PDE? Let fix the problems where they are rooted, not add workarounds here and there at all possible JDT / Platform clients. |
but I'm not very confident I should push such changes forward, because beside from deleting the offending line I can't in any way answer question or decide what consequences this has. So actually a JDT committer would be much better person to "own" such change... anyways I have created a PR here: |
I also had the same impression and was surprised that neither @jarthana nor another JDT committer actively discussing this issue here created the PR |
You claimed
what is simply wrong, because it will still be required unless someone
because otherwise the problem will simply persist and this has some serious consequences as it possibly deadlocks the whole IDE startup (as complained by @jukzi). Because of that I do not want to "wait" for too much things just to see we missed the M1 / whatever / ... and need to wait another release for that. If we are fast, the code can simply be removed without any effort, so we do not miss anything (beside not let people complain about hanging Eclipse). |
|
The problem is not a new one and is not observable by many people (otherwise we will see more bugs reported), so I do not see urgent need to apply the proposed workaround. Also if is a new problem, we've missed yet another component that must be fixed because the code in question is pretty old. With that, I agree with @jukzi and I do not see a value to add a questionable workaround in PDE (which will remain forever) if we can consistently work on improving things that cause the problem in the platform/JDT. |
|
Just search for "eclipse freezes on startup splash screen" and you will see that this is far from being "not observable by many people"... I don't want to state that it is exactly this problem, but the people see such problems and are annoyed, and the usual "workaround" is to kill the process and try again (and being annoyed)...
Well the first reaction on JDT was "can you fix it yourself" so I'm not very confident that it is "consistently work" on ... but the blame will be put on PDE. |
|
There are two issues:
|
The problem is that the called method can't decide "how early" it is called and in the usual case (UI is showing) one actually want a job, so one is back to the problem to how detect/decide that (what is this PR about)
The join do not do any deadlock, it simply returns and that is documented, the problem is that the Job itself never executes and the code needs to wait for the result. In this particular case it needs to contact an updatesite to get the available IUs, without this information it could not proceed --> wait for ever (so it is not a real deadlock)
In any case it will lead to bad performance, it will just be maybe very slow (but not wait forever) |
Currently there are some bad behaving UI components in the eclipse IDE that trigger resolving of the classpath containers in the UI, this leads to very bad startup performance and even deadlocks in startup.
This now detects the issue, logs a warning of the offending component and defer the initialization of classpath to a background job, this currently increase time from starting eclipse until UI is shown noticeable.
Fix #1481