Speed up loading file statuses in Git commit dialog by batching events and skipping events for up-to-date files#9324
Conversation
Replace per-file PROP_FILE_STATUS_CHANGED firing in refreshStatusesBatch with a single PROP_FILES_STATUS_CHANGED batch event, eliminating 100k redundant propertyChange/schedule/SwingUtilities.invokeLater calls on first load. This improves performance a bit because it eliminates many method calls. However, in the end, the number of files changed is the same so the event handler still needs to process all of them. Also skip firing events for up-to-date files that are not yet in the cache, since UPTODATE is the default for managed files. This drastically reduces the time spent in the refreshStatusesBatch method on big repositories executed when Commit dialog opens. For example, on the Netbeans repository, from around 8 seconds to 20ms.
mbien
added
git
mbien
left a comment
mbien
left a comment
There was a problem hiding this comment.
this looks good to me. Thanks a lot for the improvements.
left some comments inline but its nothing important
| for (ChangedEvent event : events) { | ||
| fireFileStatusChanged(event); | ||
| if (!events.isEmpty()) { | ||
| listenerSupport.firePropertyChange(PROP_FILES_STATUS_CHANGED, null, events); |
There was a problem hiding this comment.
good catch. aggregating events can certainly reduce overhead under load. Reminds me a little on #8955 where I saw 1.5mil event handler invocations when a large project group opened.
| fireStatusChanged(changedEvent.getFile()); | ||
| } else if (event.getPropertyName().equals(FileStatusCache.PROP_FILES_STATUS_CHANGED)) { | ||
| List<FileStatusCache.ChangedEvent> changedEvents = (List<FileStatusCache.ChangedEvent>) event.getNewValue(); | ||
| Set<File> files = new HashSet<>(changedEvents.size()); |
There was a problem hiding this comment.
this is super nitpicky. but the constructor arg isn't for the element count, it is to size the internal table - which sometimes leads to initial sizes which are too small.
if you want you can bump the module to javac.release=21 (see project.properties) and then use the HashSet.newHashSet(numElements) factory which does some math before setting the size of the internal table.
feel free to update the commit and force push in place.
| for (FileStatusCache.ChangedEvent changedEvent : changedEvents) { | ||
| if (revisionLeft == Revision.HEAD // remove when we're able to refresh single file changes for Local vs. any revision | ||
| && revisionRight == Revision.LOCAL && affectsView(changedEvent)) { |
There was a problem hiding this comment.
could the revisionLeft/Right check be moved before the loop?
2 participants
Replace per-file PROP_FILE_STATUS_CHANGED firing in refreshStatusesBatch with a single PROP_FILES_STATUS_CHANGED batch event, eliminating 100k redundant propertyChange/schedule/SwingUtilities.invokeLater calls on first load. This improves performance a bit because it eliminates many method calls. However, in the end, the number of files changed is the same so the event handler still needs to process all of them.
Also skip firing events for up-to-date files that are not yet in the cache, since UPTODATE is the default for managed files. This drastically reduces the time spent in the refreshStatusesBatch method on big repositories executed when Commit dialog opens. For example, on the Netbeans repository, from around 8 seconds to 20ms.
Complements #9304, which speeds up the process even more, in a different area.
Click to collapse/expand PR instructions
PR approval and merge checklist:
If this PR targets the delivery branch: don't merge. (full wiki article)