Community
Participate
Working Groups
I20100413-1521 (introduced in R3.3). 1. start with new workspace 2. check out 'org.eclipse.jdt.ui' from CVS 3. open plugin.xml and switch to 'plugin.xml' tab 4. go to the last line (Ctrl+End) 5. move the line "</plugin>" one line up (above " </extension>") ==> editor scrolling and typing is now extremely (almost unusably) slow
Bug 1: ====== org.eclipse.pde.internal.ui.editor.actions.HyperlinkAction.detectHyperlink() is called on each key press. This is wrong: it should only detect hyperlinks on Ctrl+mouse move. To fix this simply switch to the official hyperlink support offered by the text editor, see bug 177136. Bug 2: ====== org.eclipse.pde.internal.ui.editor.plugin.ExtensionAttributePointDectector.keyPresssed() is called on each keystroke which loads the XML model. No idea whether this is really needed since the Javadoc for that class doesn't tell much ;-) Bug 3: This is the bad one! =========================== org.eclipse.pde.internal.core.text.plugin.PluginModelBase.getPluginBase(boolean) uses a flag 'fLoaded' which is never set to 'true' when the XML is not correct (SAXParseException). This leads to extreme load in case of invalid XML since all actions try to create the plug-in base again and again. The correct fix should only try to create the plug-in base once (i.e. always set 'fLoaded' to 'true') and set 'fLoaded' to 'false' when the next documentChanged event happens.
Should be investigated for 3.6.
Created attachment 164831 [details] Starting point for fix The attached patch is a starting point for the fix and might even do the trick as a quick hack. Please also see that the current code recursively loads the model and currently only works because 'fLoaded' is set to 'true' at the very beginning of the call chain. I did not verify whether my fix for this in PluginDocumentHandler is the right solution to break the recursion i.e. whether it's OK to not force the loading. Another problem is that AbstractEditingModel.isValid() is based on (same as) 'fLoaded' and there are many methods that either call isValid() or isLoaded(). This needs to be cleaned up i.e. decided which one is really needed.
M1?
We reproduced the performance problems (just try modifying JDT UI's plugin.xml with and without the xml structure being broken). We should definitely see whether there are small and simple fixes we can make here. Leaving marked for 3.7 as we do not know who has time to commit to this in M1.
+1 for fixing this. I have a plugin.xml with +5000 lines and every time I make a type-o and break the XML structure I have to wait for about a minute until I can continue working because Eclipse becomes completely unresponsive. Which is not very pleasant, obvious. I'm using I20101028-1441.
Ankur, can you target this for M4 or M5? Thanks.
targeting for 3.7 M4
(In reply to comment #1) > Bug 1: > ====== > org.eclipse.pde.internal.ui.editor.actions.HyperlinkAction.detectHyperlink() is > called on each key press. This is wrong: it should only detect hyperlinks on > Ctrl+mouse move. To fix this simply switch to the official hyperlink support > offered by the text editor, see bug 177136. Bug #177136 has already been fixed. The problem was calling detectHyperlink on keypress. The call was just to set the action names in context menu. This wasn't needed until context menu was invoked. I have removed both mouse and key listeners from HyperlinkAction. Now only one call made to detecthyperlink when context menu is invoked. > Bug 2: > ====== > org.eclipse.pde.internal.ui.editor.plugin.ExtensionAttributePointDectector.keyPresssed() > is called on each keystroke which loads the XML model. No idea whether this is > really needed since the Javadoc for that class doesn't tell much ;-) > Again the key and mouse listeners were not needed. removed. > Bug 3: This is the bad one! > =========================== > org.eclipse.pde.internal.core.text.plugin.PluginModelBase.getPluginBase(boolean) > uses a flag 'fLoaded' which is never set to 'true' when the XML is not correct > (SAXParseException). This leads to extreme load in case of invalid XML since all > actions try to create the plug-in base again and again. The correct fix should > only try to create the plug-in base once (i.e. always set 'fLoaded' to 'true') > and set 'fLoaded' to 'false' when the next documentChanged event happens. This was fixed by the patch submitted by Dani.
Created attachment 183949 [details] Patch Dani, please review the patch and confirm this fixes the problem for you satisfactorily.
(In reply to comment #10) > Created an attachment (id=183949) [details] [diff] > Patch > > Dani, please review the patch and confirm this fixes the problem for you > satisfactorily. The fix breaks the enabling and the tool tip of the HyperlinkAction in the toolbar. >Another problem is that AbstractEditingModel.isValid() is based on (same as) >'fLoaded' and there are many methods that either call isValid() or isLoaded(). >This needs to be cleaned up i.e. decided which one is really needed. I could not see this in the patch.
Created attachment 186149 [details] Updated Patch
(In reply to comment #11) . > The fix breaks the enabling and the tool tip of the HyperlinkAction in the > toolbar. restored the listeners which were breaking it > >Another problem is that AbstractEditingModel.isValid() is based on (same as) > >'fLoaded' and there are many methods that either call isValid() or isLoaded(). > >This needs to be cleaned up i.e. decided which one is really needed. > I could not see this in the patch. This two method come from different interfaces IModel#isLoaded() and IBaseModel#isValid() so we can't get rid of one of two. And since there just too many places they are called, I don't think it will be a good idea to touch it.
Ankur, the patch is good to be released for M5.
This is weird. I remember applying the patch to the HEAD and resolving this bug last night. Looks like my eyes tricked me. anyway...Fixed in HEAD. Good for M5.
Verified in I20110124-1800.
*** Bug 172788 has been marked as a duplicate of this bug. ***
NOTE: This fix caused a regression, see bug 340640.
