Summary: | [performance] Subsystem Wizard Pages should not be contributed automatically from property pages | ||||||
---|---|---|---|---|---|---|---|
Product: | [Tools] Target Management | Reporter: | Martin Oberhuber <mober.at+eclipse> | ||||
Component: | RSE | Assignee: | Martin Oberhuber <mober.at+eclipse> | ||||
Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||
Severity: | normal | ||||||
Priority: | P2 | CC: | kmunir, uwe.st | ||||
Version: | 2.0 | Keywords: | performance | ||||
Target Milestone: | 2.0.1 | Flags: | mober.at+eclipse:
pmc_approved+
ddykstal.eclipse: review+ |
||||
Hardware: | All | ||||||
OS: | All | ||||||
Whiteboard: | |||||||
Bug Depends on: | |||||||
Bug Blocks: | 176490, 181939, 197129 | ||||||
Attachments: |
|
Description
Martin Oberhuber
2007-02-20 10:40:42 EST
Kushal, since you're more involved in the wizard stuff as of late, I'm going to assign this to you for now. Feel free to give it back if you don't have time. #2 is fixed (the NPE), but if we can get rid of the whole code, it better would be. This is a bigger non-critical thing that we'll need to look at in M7. It is related to bug 176490 (wizard improvements). Hmm... this is an API change in the sense that we'd change whether extenders can expect property pages being added to the wizard, or not. If we are to make this change for 2.0, I'd like to clearly understand 1. Why the code is there 2. Who would potentially use the code 3. Whether it currently works as expected or not I'd love to get rid of instantiating property pages for the wizard, because it is another reason for activating plugins too early and unnecessarily (bug #181939). But we need to clearly know what we are doing. Can we chat about this at the committer meeting? Please be prepared with answers. In a large product based on Eclipse, it can be really problematic that each and every plugin contributing a Property Page may be activated here. We should investigate what the intention was of having this code, and whether it actually works or is needed. If it doesn't work as expected anyways, we should take it out for 2.0.1 -- it would not be an API change in that case because the API was never described and never actually worked. (In reply to comment #5) > > We should investigate what the intention was of having this code, and whether > it actually works or is needed. If it doesn't work as expected anyways, we > should take it out for 2.0.1 -- it would not be an API change in that case > because the API was never described and never actually worked. > The original intention of this code was to allow existing property pages for subsystems to be configured in the new connection wizard. A particular interface needed to be implemented in order for it to be acceptable. As noted, there's a nasty side-effect of checking the type of the object for which the property page is enabled for - that it can load plugins that aren't needed/wanted. In the past, IBM products have used this for configurating dstore connections, however, we now have the services property pages to do the same thing and developers can override getWizardPages() if custom pages are desired. So I think we can safely remove this code. I made some more investigations. 1. The code in question is only executed when looking at an ISubSystemConfiguration that is not an IServiceSubSystemConfiguration. In stock RSE, these are DeveloperSubSystemConfiguration and TestSubSystemConfiguration -- without these, nothing is reproducable. But note that commercial adopters are likely to have subsystems which are not service (Wind River does). 2. When any systemType has a non-servicesubsystem registered against it, the code is executed. In my workspace, it checks for 73 propertyPages, and loads 10 different interface classes specified in the objectClass attribute. Among these interface classes are: org.eclipse.cdt.debug.core.model.ICDebugTarget org.eclipse.core.resources.IProject org.eclipse.cdt.core.model.ITranslationUnit org.eclipse.jdt.core.IJavaProject org.eclipse.core.resources.IFile org.eclipse.core.resources.IFolder org.eclipse.team.internal.ccvs.ui.repo.RepositoryRoot org.eclipse.update.internal.ui.model.IFeatureAdapter org.eclipse.update.internal.core.SiteLocal This means that on wizard startup, the following plugins are activated: cdt.core, cdt.debug.core, core.resources, jdt.core, team.cvs.ui, update.ui The first 3 are no problem because they are in earlyStartup anyways. The other 3 are not activated by default -- and in a large product, there are potentially many more activated because of our wizard. 3. In order for the code to actually succeed eventually, the propertyPage must be registered against an ISubSystem that is created from the SubSystemConfiguration being investigated. Plus, the propertyPage must implement ISystemConnectionWizardPropertyPage in stock RSE, there are two implementations of this interface, both of which are registered against Dstore, which is an IServiceSubSystem, so nothing actually happens. 4. Moreover, for RSE-internal the mechanism got "deactivated" when we replaced the objectClass attribute of our property pages by the new not-deprecated <enabledWhen> / <instanceof> mechanism. I think we should really remove the code in question for 2.0.1 since I'm scared about the performance impact of activating at least 3 unnecessary plugins in a "small" installation -- so what about a really large installation? I do think that this is a change in functionality, but I do not consider it an API change since that functionality has never been documented (Javadoc of ISystemConnectionWizardPropertyPage and SystemSubSystemsPropertiesWizardPage is very very vague, and nobody says that propery pages can automatically be contributed as wizard pages -- it is just part of the default implementation in the SubSystemConfigurationAdapter. If anybody really really has a problem with this change, he can write his own SubSystemConfigurationAdapter, deriving from our default impl, and override the getNewConnectionWizardPages() method to restore the old functionality. That way, somebody could restore the 2.0 functionality in a 2.0.1 based product and it would work the same in both versions. So, I removed the [api] tag from this bug and give my go for the change in 2.0.1 -- I filed bug #197129 for a future API change to complete the task in the 3.0 timeframe. DaveD -- do you agree that this change is OK for 2.0.1? Created attachment 74145 [details]
Patch to fix the performance issue and deprecate obsolete API
Attached patch removes the offending code, and deprecates the offending methods for future removal. I think this is the best we can do for 2.0.1, and it is the right thing to do.
Based on the discussion, this sounds like exactly the right thing to do. Makes sense to me. Martin, were you going to commit this? Patch committed and released for I20070719-1300. |