Community
Participate
Working Groups
If I import an executable whose sources are located on a networked drive, the UI grinds to a halt whenever I select the view and that executable is selected. This is because the SourceLabelProvider in the executables view is doing multiple redundant lookups of all the files referenced in the executable, via one or more calls to File#exists, File#length, etc for each column and each row of the table. This information could be cached and refreshed either when the executable has changed or at some lower time interval, like 15-30 seconds (it doesn't seem critical to have real-time updates of whether a file exists or not). Also, note that even hiding all the columns doesn't help, because the JFace framework seems to ask for all the file data no matter what.
Actually, having the view open at all reveals that it gets refreshed inordinately often (e.g. due to SWT expose events) and makes it difficult to get any work done. What's worse, it looks like the SourceFilesContentProvider is still wired up and refreshing content long after closing the view! I really have to do something for this... I copied files to my local machine and imported them into the Executables view to avoid a different network slowness issue, and now this ;)
Created attachment 170964 [details] proposed patch This patch addresses most of the performance issues: > What's worse, it looks like the SourceFilesContentProvider is still wired up > and refreshing content long after closing the view! 1) Ensure we remove the launch configuration changed listener when the view is disposed. > If I import an executable whose sources are located on a networked drive, the > UI grinds to a halt whenever I select the view and that executable is selected. 2) One refresh job was being launched explicitly on the UI thread, though it had no real need to be. This is a normal job now, that uses Display#asyncExec to avoid any SWT invalid thread access issues when refreshing the view. > This is because the SourceLabelProvider in the executables view is doing > multiple redundant lookups of all the files referenced in the executable, via > one or more calls to File#exists, File#length, etc for each column and each row > of the table. 3) I added a cache (using the existing CDT core LRUCache) of the various pieces of information fetched from the ITranslationUnit and from the original source file. This info is updated after the info is 30 seconds old or whenever the executables list or an executable changes. Note, this doesn't fully address the performance issues, since the SourceFilesLabelProvider and the file info lookup is still called on the UI thread, but it's a lot better in general now over a network. I hope this is acceptable for 7.0.
Argh, I just hit another bottlneck -- an explicit Job#join() call from SourceFilesContentProvider#getElements(). This is actually one of the causes for the original UI hang that I hadn't detected. I'll post a separate patch for that.
Created attachment 170981 [details] updated patch This patch replaces on the previous one. 1) Another UI job is vanquished. When the selection on the executable viewer changes, we immediately set the new input. This avoids the main blocking call to Executable#getSourceFiles(), whose contents are thrown away and presumed to have been cached for use by the content provider later (yuck). 2) A background job fetches the source file information, returning a dummy "Refreshing..." content item in the meantime. Then when the job is complete, it refreshes the viewer with the new content. (The source files tree is flagged with deferred updates == true while this refresh occurs, or else the UI updates very slowly.) 4) The TranslationUnitInfo cache is moved into SourceFilesViewer itself, since SourceFilesContentProvider needs to check file existence too in a #hasChildren() check. Again, as before, there is still an issue that the SourceFilesLabelProvider can hang the UI for a little while if it is first looking for the file information when the tree is refreshed, but fixing this would be too intrusive for this fix. (This UI hanging is just a 2-5 secon blip now for an executable with sources on a network, rather than a minute-long drag.)
Could you look at this, John? Ken and I think this is pretty important for our users.
Ed, I've been reviewing the patch in detail. I can yield to Ken if you want.
(In reply to comment #6) > Ed, I've been reviewing the patch in detail. I can yield to Ken if you want. Thanks -- keep going on if you've already started. (I only realized recently that I had set all the flags wrong, which accounts for those changes. :)
(In reply to comment #6) > Ed, I've been reviewing the patch in detail. I can yield to Ken if you want. Please keep looking, I"m in the middle of looking at something else.
(In reply to comment #5) > Could you look at this, John? Ken and I think this is pretty important for our > users. Ed, here's my feedback on the patch. 1. In QuickParseJob.QuickParseJob(Executable) the initialization of 'this.tus' to null is unnecessary. 2. In SourceFilesContentProvider.getElements(Object) a. access to fetchedExecutables should be synchronized b. the JobChangeAdapter should check the event to see if it was cancelled or there was a failure and only update fetchedExecutables if it's neither. c. It's safer for the JobChangeAdapter to remove the entry to pathToJobMap based on the job object and not the executable. It could theoretically run after some other code has since cleared the map (via cancelQuickParseJobs) and added a new job for the same executable 3. In SourceFilesContentProvider.executablesListChanged(), the comment "But cancel any jobs that might be for closed projects." seems a bit accurate. I think it cancels *all jobs*, the motivation being that some may be for executables that are in now-closed projects. This leads me to wonder if IExecutablesChangeListener.executablesListChanged() shouldn't provide the before and after list so that the listeners can react more intelligently. 4. Why does SourceFilesContentProvider.inputChanged(Viewer, Object, - Object) cancel the parse jobs? The list of executables hasn't changed. 5. Why a timestamp is passed to SourceFilesViewer.fetchTranslationUnitInfo(). The implementation could just get the current timestamp at the beginning of the method. 6. In SourceFilesLabelProvider.update(ViewerCell) cell.getElement() is called four times. I'd make one call and store it in a local. More efficient, less clutter, and theoretically safer. 7. Seems to me 30 seconds is a fairly large window, catering to your worst case scenario (which I think is the exception rather than the norm). Should we not make that user configurable even in this initial release, and bring down the default value? 8. I'd rather see SourceFilesViewer.translationUnitInfoCache be private and a static method added for flushing it rather than exposing it and expecting the caller to worry about the synchronization requirements 9. I think the fields of TranslationUnitInfo need explanation. Let's not require folks trying to understand the code to search and analyze how these fields are set. 10. TranslationUnitInfo.isFile is unused 11. SourceFilesViewer.refreshContent() was changed to use a regular job for part of the work, but that works starts off by calling a jFace method (ContentViewer.getInput()) and that shouldn't be done on a non-GUI thread. 12. in SourceFilesViewer.fetchTranslationUnitInfo(), the qualification of static fields and types with 'SourceFilesViewer' seems unorthodox and adds clutter 13. In SourceFilesViewer.fetchTranslationUnitInfo(), if element is not a translation unit, we end up creating a TUI for it all the same. Does that entry serve any purpose. If not, wouldn't it better to just return null at that point? 14. SourceFilesViewer.fetchTranslationUnitInfo() seems to be ok with a null executable reference being passed in. Maybe I'm overlooking something but when is that ever going to be a valid scenario?
> 1. In QuickParseJob.QuickParseJob(Executable) the initialization of 'this.tus' > to null is unnecessary. Ok. > > 2. In SourceFilesContentProvider.getElements(Object) > a. access to fetchedExecutables should be synchronized Right, good catch. > b. the JobChangeAdapter should check the event to see if it was cancelled or > there was a failure and only update fetchedExecutables if it's neither. Ok. > c. It's safer for the JobChangeAdapter to remove the entry to pathToJobMap > based on the job object and not the executable. It could theoretically run > after some other code has since cleared the map (via cancelQuickParseJobs) and > added a new job for the same executable Ok, I see. > 3. In SourceFilesContentProvider.executablesListChanged(), the comment "But > cancel any jobs that might be for closed projects." seems a bit accurate. I > think it cancels *all jobs*, the motivation being that some may be for > executables that are in now-closed projects. This leads me to wonder if > IExecutablesChangeListener.executablesListChanged() shouldn't provide the > before and after list so that the listeners can react more intelligently. Yes, it clears all jobs in case one is closed. I didn't want to add complex N-N mapping logic for executable <-> project just to avoid killing some jobs unnecessary. And yes, I think the listener should provide more info, but I'm not daring to change API here ;) We can cover that later if needed. > 4. Why does SourceFilesContentProvider.inputChanged(Viewer, Object, - Object) > cancel the parse jobs? The list of executables hasn't changed. I guess you're right. That was a trained response of mine due to typically caching objects related to the content model, but we're keying off IPath, which may be shared between different models. > 5. Why a timestamp is passed to SourceFilesViewer.fetchTranslationUnitInfo(). > The implementation could just get the current timestamp at the beginning of the > method. True. This code was stolen from something that was querying a long list of files, where I wanted a consistent timestamp for the whole set. > > 6. In SourceFilesLabelProvider.update(ViewerCell) cell.getElement() is called > four times. I'd make one call and store it in a local. More efficient, less > clutter, and theoretically safer. Okay. > 7. Seems to me 30 seconds is a fairly large window, catering to your worst case > scenario (which I think is the exception rather than the norm). Should we not > make that user configurable even in this initial release, and bring down the > default value? I'd prefer not to do this right now (either the pref or changing the timeout). The executable manager's listeners will trigger flushing of this cache, which is the most likely case where the source files info is expected to have changed. But, this did prod me to find out why the "refresh" action (which would be the workaround for users who really wanted up-to-date info) didn't affect the list, so I will add another flush call in the SourceFilesViewer#refreshContent() method. > 8. I'd rather see SourceFilesViewer.translationUnitInfoCache be private and a > static method added for flushing it rather than exposing it and expecting the > caller to worry about the synchronization requirements Ok. This was an oversight when I moved the cache to another class. > 9. I think the fields of TranslationUnitInfo need explanation. Let's not > require folks trying to understand the code to search and analyze how these > fields are set. Ok. > 10. TranslationUnitInfo.isFile is unused Thanks. > 11. SourceFilesViewer.refreshContent() was changed to use a regular job for > part of the work, but that works starts off by calling a jFace method > (ContentViewer.getInput()) and that shouldn't be done on a non-GUI thread. I disagree. This is a model-only method and not a UI call in any implementation. But, that did make me notice that there is no need whatsoever for #refreshContent() to spawn a job, since all the real work has been moved to SourceFilesContentProvider. So I'll put all of that in Display.asyncExec(). > 12. in SourceFilesViewer.fetchTranslationUnitInfo(), the qualification of > static fields and types with 'SourceFilesViewer' seems unorthodox and adds > clutter Quite! Another side effect of moving the cache to another class (and you were so good to me 'til now, JDT ;). Will clean up. > 13. In SourceFilesViewer.fetchTranslationUnitInfo(), if element is not a > translation unit, we end up creating a TUI for it all the same. Does that entry > serve any purpose. If not, wouldn't it better to just return null at that > point? Yes, good point. > 14. SourceFilesViewer.fetchTranslationUnitInfo() seems to be ok with a null > executable reference being passed in. Maybe I'm overlooking something but when > is that ever going to be a valid scenario? Probably never. I think I had been calling this from a third place, since deleted, which is not needed anymore.
Created attachment 171046 [details] full patch with proposed changes All the described changes are in this patch, plus an externalized string from QuickParseJob that somehow got lost from the last effort.
(In reply to comment #10) > > 11. SourceFilesViewer.refreshContent() was changed to use a regular job for > > part of the work, but that works starts off by calling a jFace method > > (ContentViewer.getInput()) and that shouldn't be done on a non-GUI thread. > > I disagree. This is a model-only method and not a UI call in any > implementation. Hm. I'm not so sure. Look at the implementation of ContentViewer.setInput(). It modifies the instance field 'input' without any serialization and the field isn't volatile. That may seem thread-safe, but technically it isn't (Java Concurrency in Practice, chapter 2). Anyway, I can't say I'm 100% sure but I'd be surprised if any JFace viewer methods can be technically called on the non-gui thread unless an exception is documented
(In reply to comment #12) > > I disagree. This is a model-only method and not a UI call in any > > implementation. > > Hm. I'm not so sure. Look at the implementation of ContentViewer.setInput(). It > modifies the instance field 'input' without any serialization and the field > isn't volatile. That may seem thread-safe, but technically it isn't (Java > Concurrency in Practice, chapter 2). Anyway, I can't say I'm 100% sure but I'd > be surprised if any JFace viewer methods can be technically called on the > non-gui thread unless an exception is documented Ok, I see. I thought you meant (in referring to UI threads) that this would cause an SWT exception. I hadn't considered the volatile field and that class of threading issues. In any case, in the last patch, all this is done on the UI thread as you suggested. :) Let me know when you think this is good to commit.
(In reply to comment #13) > Let me know when you think this is good to commit. Let's move forward with the commit. I'll do a double-check later this morning.
Ok. Oh, one last thing -- in the original version of the SourceFilesContentProvider class, the call to Executable#getSourceFiles(IProgressMonitor) is not a warning, but it is now. (That method is marked "@noreference" in org.eclipse.cdt.debug.core.) I see no existing .api_filters file that allowed this reference before, so I'm unsure why it wasn't detected. This was originally called from an anonymous override of org.eclipse.core.runtime.Job#run(IProgressMonitor) and is now called from QuickParseJob#run(IPM). Do I need to add a usage problem filter to quell the new warning?
Fixed in HEAD (.api_filter change pending)
*** cdt cvs genie on behalf of eswartz *** Bug 315415 fixed performance issues of Executables view, esp. with files over networks [*] messages.properties 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/messages.properties?root=Tools_Project&r1=1.5&r2=1.6 [*] Messages.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/Messages.java?root=Tools_Project&r1=1.5&r2=1.6 [*] SourceFilesLabelProvider.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/SourceFilesLabelProvider.java?root=Tools_Project&r1=1.4&r2=1.5 [*] SourceFilesViewer.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/SourceFilesViewer.java?root=Tools_Project&r1=1.6&r2=1.7 [*] ExecutablesView.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/ExecutablesView.java?root=Tools_Project&r1=1.7&r2=1.8 [*] SourceFilesContentProvider.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/SourceFilesContentProvider.java?root=Tools_Project&r1=1.5&r2=1.6
(In reply to comment #15) > Ok. Oh, one last thing -- in the original version of the > SourceFilesContentProvider class, the call to > Executable#getSourceFiles(IProgressMonitor) is not a warning, but it is now. > (That method is marked "@noreference" in org.eclipse.cdt.debug.core.) > > I see no existing .api_filters file that allowed this reference before, so I'm > unsure why it wasn't detected. This was originally called from an anonymous > override of org.eclipse.core.runtime.Job#run(IProgressMonitor) and is now > called from QuickParseJob#run(IPM). > > Do I need to add a usage problem filter to quell the new warning? This looks like an API tooling bug. I've opened bug 315776. Yes; we should have an api filter for that. Hopefully the platform folks will introduce a concept similar to x-friends for API tooling.
Ok. done. Thanks a lot for your detailed review!
*** cdt cvs genie on behalf of eswartz *** Bug 315415: add .api_filters exception for Executable#getSourceFiles() call [+] .api_filters http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/.settings/.api_filters?root=Tools_Project&revision=1.1&view=markup
(In reply to comment #19) > Ok. done. > > Thanks a lot for your detailed review! Thanks for diligently addressing all the issues, minor or not.
(In reply to comment #14) > (In reply to comment #13) > > Let me know when you think this is good to commit. > > Let's move forward with the commit. I'll do a double-check later this morning. Ed, the latest patch looks good. Just one minor thing with the resolution to 2b. The removal of the job from the map should happen whether or not the job succeeded. With your adjustment, the removal only happens if it succeeded.
> Ed, the latest patch looks good. Just one minor thing with the resolution to > 2b. The removal of the job from the map should happen whether or not the job > succeeded. With your adjustment, the removal only happens if it succeeded. Oh yes. Hmm, both the updates to pathToJobMap and fetchedExecutables are in the asyncExec call too, which seems a bit late. What about: final QuickParseJob theJob = job; job.addJobChangeListener(new JobChangeAdapter() { public void done(IJobChangeEvent event) { synchronized (pathToJobMap) { pathToJobMap.values().remove(theJob); } if (event.getResult().isOK()) { synchronized (fetchedExecutables) { fetchedExecutables.put(exePath, theJob.tus); } Display.getDefault().asyncExec(new Runnable() { public void run() { // update the viewer ...
(In reply to comment #23) > > Ed, the latest patch looks good. Just one minor thing with the resolution to > > 2b. The removal of the job from the map should happen whether or not the job > > succeeded. With your adjustment, the removal only happens if it succeeded. > > Oh yes. Hmm, both the updates to pathToJobMap and fetchedExecutables are in > the asyncExec call too, which seems a bit late. What about: > [snip] Looks good to me.
*** cdt cvs genie on behalf of eswartz *** Bug 315415: always clear a terminated job from pathToJobsMap, and store off fetchedExecutables as soon as possible, leaving only UI work to Display#asyncExec(). [*] SourceFilesContentProvider.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/SourceFilesContentProvider.java?root=Tools_Project&r1=1.6&r2=1.7
