Community
Participate
Working Groups
3.3 I20070525-1350 When I start on an empty workspace, open the plug-in development perspective, reveal the "plug-ins" view. You will notice that eclipse is unresponsive while the target is being initialized (note that this is a guess). It could be beneficial to populate the view in the background.
Should the view use a deferred content provider instead? I guess the question is whether the cause really is because of the initialization and calculation or not.
The problem is definitely initialization. Since PDE hasn't needed to start up until loading that view, it is upon the view opening that PDE goes out and reads the files on the file system for the target platform. With the extension registry work (which Pascal has asked for and not yet received performance data for), the time to do this should be just about cut in half. Since the old initialization needed to read both Manifests and plugin.xmls, the new initialization will only read Manifests and read the plugin.xmls when it needs extension information (which it doesn't for this view). The result should be half the time waiting. If this is still a problem in the current build, we can look at other means to fix this problem.
This is a scalability issue effecting RSA/RSD/RSM/RAD, RSD for example has upwards of 1200 plugins and populating the plug-ins view can take several minutes while the entire UI is hung. (Long enough for me to fire up bugzilla and find this existing bug.) For customers extending Eclipse this will give an extremely negative experience.
patches welcome! :D
... and then they wonder why we don't log issues as quickly as we could ...
Alex, welcome to open-source. If this is critical to you, please provide a patch or wait until it gets addressed. If there's a patch, the bug will get addressed quickly. The PDE team would love to help everyone it can, but it's not like we have an army of committers.
adding Mylyn context to help people get started
Created attachment 81288 [details] mylyn/context/zip
We'll target for 3.4, maybe a clever use of DeferredTreeContentManager would help here.
Created attachment 81445 [details] JUnit Test to perform automated execution of scenario To use this, import the archive into eclipse and run the JUnit (plug-in) test. Included in the archive are two launchers one for Eclipse, the other for RSA. They are not in any way special and you could just as easily create a new launcher. The test will always succeed (except for a catastrophic failure in Eclipse - e.g. missing PDE) and will output some timing results to System.out. To mimic user think time, some pauses have been introduced. The three figures relate to : 1) The time to show the "Plug-ins" view 2) The total time to run the test 3) The total time to run the test, less the additional delays.
Created attachment 81446 [details] Patch to initialise the PDE on opening of the perspective
I have just attached a couple of files to this bug; as a result of a little lateral thinking N.B this fix does not in anyway improve the actual performance of the PDE initialisation. The patch initialises the underlying PDE model at the point the PDE perspective is opened. It does this as a background job, so the user can continue to use Eclipse. I think, if you've gone into the PDE perspective you're very likely to tigger the initialisation pretty soon anyway, e.g. via showing the plug-ins view or by creating a plug-in. If your workspace has a plug-in already then initialisation appears to take place anyway. Therefore, by performing the initialisation in the background when the perspective is opened, we are reducing the impact on the user by utilising their 'think time' and therefore creating the perception of a better performing product. By using the test plugin that I have also attached, I got the following figures: * For Eclipse 3.4 (I20071023-0800) on Vista (about 150 plugins) the average time to open the view dropped from 975ms to 375ms. * For RSA 7003 on Vista (about 2100 plugins!) the average time to open the view dropped from 6633ms to 1417ms. The other reason for considering this change is that I perceive it to be of considerably less impact than re-working the view to utilise deferred logic, or to rework the initialisation to be a more optimal implementation and therefore would be more likely to be capable of being included in 3.4 with little or no impact. Thoughts?
To be clear about the figures in my previous post; the average times relate to multiple executions showing the plug-ins view, with each execution starting a new process using a completely new workspace. The average is not merely open/close/open/close/etc of the plug-ins view, nor is it the time to open the view on an already initialised workspace.
Obviously, it should be noted that this fix, that I've supplied, performs an 'eager load' of the plug-in data; this is contrary to the normal 'lazy load' behaviour of Eclipse. Therefore, I fully understand if this fix may not be acceptable for inclusion into the product.
Hey Les, I appreciate the effort you put into this bug. This is a good though and definitely a valid way to try to solve the problem. Unfortunately, like you said above the "eager" loading is not optimal. There are two problems with this approach. 1. It is not uncommon to accidentally enter a perspective you don't intend to use. I had this problem with the "Planning" view when Chris was introducing me to Mylan (which rocks). If the user accidentally entered the perspective, we would initialize PDE which creates a bunch of models/data the user may never use. 2. Though most people use the Plug-ins view in the PDE perspective, I know some plug-in developers that work in the java perspective. They can then open individual views which they need. For users doing this, they would still see a problem. The good news, I think I have a theory on how this can be done. In the PluginsContentProvider.getChildren(), before calling PluginModelManager.getAllModels(), we should first check to see if the PluginModelManager.isInitialized(). If not, then I would return an empty array (or something more creative) and kick off the initialization job. When the job is finished, it could then go back and refresh the Plug-ins View. I have not tried any of this, but this is how I might look at doing it. It would be lazy loading and completely local to the Plug-ins View itself. I haven't looked into this at all, so there might be better solutions like DeferredTreeContentManager like Chris pointed out.
Created attachment 81573 [details] Patch to initialise in the background. Brian, I liked your idea so much that I've attached a patch. (Built on I20071023, subsequently tested successfully on HEAD). When opening the view, it checks to see if the PDE is initialised, if it's not it starts off a job that will refresh the view on completion (with disposal checks of course). Didn't need to move to a DeferredTreeContentManager to effect this change. Interestingly, I spotted a race condition in the use of the PDE's PluginModelManager.isInitialized() that I have also fixed (for this scenario).
nice!
Oh my gosh, it is so pretty! I thought the idea was simple but you definitely made it elegant and super polished. I liked returning the string saying the view is initializing, that was a very nice touch. Good catch on the race condition. I am still doing some testing, but thought I would mention the few little things I modified. These are all pretty minor so I have made the modifications in my workspace and you don't need to redo your patch. First a really minor change, if you use JobChangeAdapter instead of IJobChangeListener you can save a few lines of code and make it look just a little more elegant. During testing I was able to create an NPE when I created a plug-in project, closed the project, restarted, and reopened the project. There is a code path in initializeTable function that will call addWorkspaceBundleToState. This function had a reference to fEntries which has not been initialized yet causing the NPE. And lastly, when I thought about it more I figured it would be nice to do something similar to this for the other PDE view (Plug-in Dependencies). So I am trying to make the initialization Job more reusable. Again, very nice!
Totally agree with the IJobChangeListener vs JobChangeAdapter; much neater. As for the other "fEntries causing an NPE" issue; I'd toyed with the idea of keeping the early initialisation of fEntries and altering the way that isInitialized() worked (i.e. using a flag). Once I thought it through I wasn't too keen and prompted for the approach in the patch. Shame I missed the addWorkspaceBundleToState() call though :-( Good job you caught it.
Should we do this for M5?
(In reply to comment #20) > Should we do this for M5? Yes, if I was on top of things we should have done this for M4. Sorry Les, I will make sure this gets in.
No worries; I guess since it's not an API change that it'll easily make it in for 3.4 proper even if it's postponed beyond M4?
I don't foresee running into any problems postponing until M5.
*ping* ... just checking this hasn't fallen off the radar ;-)
> *ping* ... just checking this hasn't fallen off the radar ;-) Funny you ask, I was working on this last night and am just finishing up some last testing before I release it. I hope to get it wrapped up by the end of the day :)
... and there was much rejoicing ... :-) Thanks Brian!
Unfortunately, there is a bit of a hold up with the discovery of blocking bug 215734. I can find a work around that is not very elegant, but I am holding off in the hope 215734 will be fixed. In the mean time, I went ahead and fixed the race condition which Les found. Thanks a lot Les for spending as much time as you did to make sure this was perfect!
OK, after a long delay, this is all in HEAD now. After some rough wrist watch calculations, I found DeferredTreeContentManager was a 2 seconds or so quicker than running an initialization job and refreshing the view (with ~1000 bundles). With the enhancements in DeferredTreeContentManager, we can update the count properly after the PendingUpdateAdapter is removed, so the don't have to hack away at the item count. This makes the code much nicer and allows us to do this the right way. I took a look at the Plug-in Dependencies View to see if we could do the same thing. What I found was it would butcher the code for no improvements. Since the Plug-in Dependencies View is not displayed by default, users most often have to go through the Open View menu, which seems to trigger PDE's initialization. This means that by the time the view is opened, PDE is initialized 99% of the time. So the is not much performance to be gained there and therefore did not update it. So without much further ado, the Plug-ins View will now populate in the background when PDE is not already initialized. Thanks Les for your help on this bug. I appreciate your patience with reviewing this one. Finding that race condition was very impressive :)
Verified on I20080204-0010
