Defer initialization of classpath to the background if called from main #1525
Conversation
|
I'm very happy to see that you working on the startup performance of Eclipse @laeubi Thank you. |
Test Results 254 files - 31 254 suites - 31 34m 41s ⏱️ - 17m 30s Results for commit 9b217a6. ± Comparison against base commit 0b123e0. This pull request skips 1 test.♻️ 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.
| Job job = JOB_MAP.get(javaProject); | ||
| if (job != null) { | ||
| try { | ||
| job.join(); |
There was a problem hiding this comment.
Doesn't this have the effect that the method blocks again until the classpath-set returns?
How would this then improve the start-up performance? Or is it just done to resolve dead-locks?
There was a problem hiding this comment.
This codepath is only executed when it is called outside the UI (e.g. in a build thread) where it is fine to block, the goal is to join on a possible execution that previously was scheduled in an UI thread.
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. |
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 eclipse-pde#1481
|
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... |
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