Community
Participate
Working Groups
Steps: 1. Open Create Branch 2. Paste "invalid branch name" 3. Add more characters Typing gets *very* slow once the text gets longer than the input field.
Created attachment 206434 [details] Fix with delay This patch adds a delay before the page is validated so it does not run on each keystroke. I didn't update the copyrights on the file.
Don't know if eGit uses the [patch] tag. Hopefully this can be reviewed for a future release.
Ping? The patch just adds a delay which is a low risk change.
(In reply to comment #3) > Ping? The patch just adds a delay which is a low risk change. Curtis, I looked at the patch. It looks good but I would - use the same delay as the FilteredTree - cancel the job when the wizard closes Do you agree with this? If so, I can submit the patch to Gerrit for review if you're not familiar with it.
Created attachment 207675 [details] Additional fixes
(In reply to comment #4) > Curtis, I looked at the patch. It looks good but I would > - use the same delay as the FilteredTree > - cancel the job when the wizard closes I changed the delay from 250 to 200 and have the job cancel when the dialog is disposed (though the job also checks that the composite is not disposed before updating the page). > Do you agree with this? If so, I can submit the patch to Gerrit for review if > you're not familiar with it. I haven't used gerrit for submitting, so it would be appreciated if you can do this for me. I did create a gerrit account, but have not set up the necessary ssh keys.
Curtis, the fix looks good but I cannot reproduce the performance problem even without the patch using 1.2.0.201111271913. Maybe the validation has been improved already and the fix is no longer needed? Steffen or Curtis, please try with 1.2.0.201111271913 and provide more detailed steps (including the repo you use) if you still see the problem.
(In reply to comment #7) > Maybe the validation has been improved already and the fix is no longer needed? > > Steffen or Curtis, please try with 1.2.0.201111271913 and provide more detailed > steps (including the repo you use) if you still see the problem. I just tried this with the egit repository and typed "a b c asdksadf askfadskfsa faskdfaksfdksakfaskdf sakkasd asksdfakasdf askdfasdas dsakdkkas sakdfksa" as the branch name. Once the error message starts to wrap I notice a delay on Gtk. It seemed that performance has already improved but it's still noticeably slower than in other dialogs, e.g. when providing an invalid source folder in the New Java Class dialog.
> It seemed that performance has already improved but it's still noticeably > slower than in other dialogs, e.g. when providing an invalid source folder in > the New Java Class dialog. OK, thanks for the info. I think we should first measure what takes long here. I would expect that checking the name comes at no cost. Maybe the suggested patch is just a workaround for another bug. Steffen, I don't have access to a Linux box right now. Can you attach some thread dumps that show where the time is spent?
It's most obvious if you hold a character. This is the stack trace that I captured: "main" prio=10 tid=0x000000004079d000 nid=0x1b73 runnable [0x00007f7da4a4f000] java.lang.Thread.State: RUNNABLE at java.io.UnixFileSystem.getLastModifiedTime(Native Method) at java.io.File.lastModified(File.java:826) at org.eclipse.jgit.storage.file.FileSnapshot.save(FileSnapshot.java:100) at org.eclipse.jgit.storage.file.RefDirectory.scanRef(RefDirectory.java:867) at org.eclipse.jgit.storage.file.RefDirectory.readRef(RefDirectory.java:838) at org.eclipse.jgit.storage.file.RefDirectory.getRef(RefDirectory.java:271) at org.eclipse.jgit.lib.Repository.resolveSimple(Repository.java:592) at org.eclipse.jgit.lib.Repository.resolve(Repository.java:566) at org.eclipse.jgit.lib.Repository.resolve(Repository.java:376) at org.eclipse.egit.ui.internal.ValidationUtils$1.isValid(ValidationUtils.java:45) at org.eclipse.egit.ui.internal.repository.CreateBranchPage.checkPage(CreateBranchPage.java:356) at org.eclipse.egit.ui.internal.repository.CreateBranchPage.access$4(CreateBranchPage.java:319) at org.eclipse.egit.ui.internal.repository.CreateBranchPage$7.modifyText(CreateBranchPage.java:314) "main" prio=10 tid=0x000000004079d000 nid=0x1b73 runnable [0x00007f7da4a50000] java.lang.Thread.State: RUNNABLE at org.eclipse.swt.internal.gtk.OS._gtk_widget_size_request(Native Method) at org.eclipse.swt.internal.gtk.OS.gtk_widget_size_request(OS.java:13352) at org.eclipse.swt.widgets.Control.gtk_widget_size_request(Control.java:3177) at org.eclipse.swt.widgets.Control.forceResize(Control.java:653) at org.eclipse.swt.widgets.Composite.getClientArea(Composite.java:598) at org.eclipse.swt.layout.GridLayout.layout(GridLayout.java:193) at org.eclipse.swt.widgets.Composite.updateLayout(Composite.java:1563) at org.eclipse.swt.widgets.Composite.layout(Composite.java:951) at org.eclipse.swt.widgets.Composite.layout(Composite.java:909) at org.eclipse.egit.ui.internal.repository.CreateBranchPage.checkPage(CreateBranchPage.java:330) My guess is that the call to upstreamConfigGroup.getParent().layout(true) is the culprit on Gtk. Would it make sense to add a check so layout is only called when control visibility has actually changed instead of on every keystroke?
Can you confirm that it's always "hanging" in UnixFileSystem.getLastModifiedTime()? If so, it looks like this takes long for files that don't exist. In that case we could check earlier in RefDirectory.scanRef(LooseRef, String) and return with 'null'.
(In reply to comment #11) > Can you confirm that it's always "hanging" in > UnixFileSystem.getLastModifiedTime()? No, I only got that once. It's reasonably fast now that it's difficult to capture a stack trace. I can only reliably reproduce the stack trace with the layout call.
(In reply to comment #12) > (In reply to comment #11) > > Can you confirm that it's always "hanging" in > > UnixFileSystem.getLastModifiedTime()? > > No, I only got that once. It's reasonably fast now that it's difficult to > capture a stack trace. I can only reliably reproduce the stack trace with the > layout call. I see. Guess then we go with the proposed fix.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Can you confirm that it's always "hanging" in > > > UnixFileSystem.getLastModifiedTime()? > > > > No, I only got that once. It's reasonably fast now that it's difficult to > > capture a stack trace. I can only reliably reproduce the stack trace with the > > layout call. > > I see. Guess then we go with the proposed fix. When using the eGit repo I too find it difficult to reproduce. However, using a larger repository (not sure if it has to do with the number of branches or the number of tag references) the performance gain from the user perspective is huge for me. Using JDT Core, before the fix it would take 3+ seconds for the checkpage() call to catch up to my typing 'integration'.
> Using JDT Core, before the fix it would take 3+ seconds for the > checkpage() call to catch up to my typing 'integration'. Since it takes 3s, can you provide a stack trace as well? I tested the patch a bit more and the LAF is not so nice: when typing a bit slowly, the 'Finish' button flickers because it gets enabled and disabled over and over again. That's ugly.
> However, using a larger repository I tried this with jdt.core and platform.common and I see zero delay on Windows 7.
OS._gtk_widget_size_request(int, GtkRequisition) line: not available [native method] OS.gtk_widget_size_request(int, GtkRequisition) line: 13227 Combo(Control).gtk_widget_size_request(int, GtkRequisition) line: 3177 Combo(Control).computeNativeSize(int, int, int, boolean) line: 635 Combo.computeSize(int, int, boolean) line: 347 GridData.computeSize(Control, int, int, boolean) line: 502 GridLayout.layout(Composite, boolean, int, int, int, int, boolean) line: 479 GridLayout.layout(Composite, boolean) line: 194 Composite.updateLayout(boolean) line: 1563 Composite.layout(boolean, boolean) line: 951 Composite.layout(boolean) line: 909 CreateBranchPage.checkPage() line: 330 CreateBranchPage.access$4(CreateBranchPage) line: 319 Every time I suspend I end up with this stack trace. I have been experiencing poor performance overall in my Eclipse installation whenever I am using git. The finish button disabling is caused by an explicit call to setPageComplete(false) before scheduling the job to prevent a user from typiing something and hitting finish before the job validates. This is unlikely to happen, so we could simply remove it, or have the wizard finish wait on job completion.
> Every time I suspend I end up with this stack trace. Strange. This looks less reasonable to me than the one from Steffen. > The finish button disabling is caused by an explicit call to > setPageComplete(false) before scheduling the job to prevent a user from typiing > something and hitting finish before the job validates. I know ;-) > This is unlikely to > happen, so we could simply remove it, or have the wizard finish wait on job > completion. If we remove it, we will end up with an error dialog and a .log entry in case of an invalid name. Not so nice. Waiting for the job could be tricky as this is a UI job (deadlock). Also, what I don't like too much is that it adds a delay for Windows users where the delay doesn't seem necessary.
Removing the unnecessary calls to layout should be straight forward and not have any negative impact. Are you interested in a patch?
(In reply to comment #19) > Removing the unnecessary calls to layout should be straight forward and not > have any negative impact. Are you interested in a patch? But this looks like a different issue than you encountered, no?
(In reply to comment #20) > (In reply to comment #19) > > Removing the unnecessary calls to layout should be straight forward and not > > have any negative impact. Are you interested in a patch? > > But this looks like a different issue than you encountered, no? Good point. To be honest, I am not sure. Maybe there was another problem or something on my local system changed. If I try with an older version of EGit I get similar stack traces but the slow down isn't as noticeable as I remember: at org.eclipse.swt.widgets.Composite.layout(Composite.java:951) at org.eclipse.swt.widgets.Composite.layout(Composite.java:909) at org.eclipse.egit.ui.internal.repository.CreateBranchPage.checkPage(CreateBranchPage.java:330) at org.eclipse.egit.ui.internal.repository.CreateBranchPage.access$4(CreateBranchPage.java:319) at org.eclipse.egit.ui.internal.repository.CreateBranchPage$7.modifyText(CreateBranchPage.java:314) I think it's a small fix and should hopefully fix the remaining problems until we have a better stack trace to analyze.
OK, so you both see the "hang" in the layout when using the latest EGit version? If so, please try whether not doing the layout fixes the performance issue and then attach a patch here.
Using my windows machine at home today and I have no performance problems in the wizard at all. Will take another look at this when I am back at the office.
After optimizing one of the calls I noticed that updating of the error message also causes an extra layout update: "main" prio=10 tid=0x000000004077d000 nid=0x76cc runnable [0x00007faf801e2000] java.lang.Thread.State: RUNNABLE at org.eclipse.swt.internal.gtk.OS._gtk_widget_size_request(Native Method) at org.eclipse.swt.internal.gtk.OS.gtk_widget_size_request(OS.java:12469) at org.eclipse.swt.widgets.Control.gtk_widget_size_request(Control.java:2988) at org.eclipse.swt.widgets.Control.forceResize(Control.java:608) at org.eclipse.swt.widgets.Composite.getClientArea(Composite.java:567) at org.eclipse.swt.layout.FormLayout.layout(FormLayout.java:281) at org.eclipse.swt.widgets.Composite.updateLayout(Composite.java:1428) at org.eclipse.swt.widgets.Composite.layout(Composite.java:920) at org.eclipse.swt.widgets.Composite.layout(Composite.java:878) at org.eclipse.jface.dialogs.TitleAreaDialog.layoutForNewMessage(TitleAreaDialog.java:451) at org.eclipse.jface.dialogs.TitleAreaDialog.setErrorMessage(TitleAreaDialog.java:398) at org.eclipse.jface.wizard.WizardDialog.updateMessage(WizardDialog.java:1303) at org.eclipse.jface.wizard.WizardPage.setErrorMessage(WizardPage.java:257) at org.eclipse.egit.ui.internal.repository.CreateBranchPage.checkPage(CreateBranchPage.java:395) I have opened http://egit.eclipse.org/r/#change,4767 with a proposed fix.
> I have opened http://egit.eclipse.org/r/#change,4767 with a proposed fix. I've commented there. Basically, I think the layout should only be done once on dialog creation.
*** Bug 360658 has been marked as a duplicate of this bug. ***
Gerrit still seems to be down, so I can't comment on the last patch. But the right solution is really to compute all refs exactly once when the dialog is opened. Bug 360658 comment 0 has a performance trace.
I'd love to see performance improvements in this area :) Creating a local branch of the JDT/Core repo I see these times: - click create branch: 12 seconds until dialog appears - start typing a new branch name: 20 seconds until dialog reacts - each character typed: > 2 secs until character appears I was going to repeat the measurement, but then bug 386193 hit again.
(In reply to comment #28) > I'd love to see performance improvements in this area :) > > Creating a local branch of the JDT/Core repo I see these times: > - click create branch: 12 seconds until dialog appears > - start typing a new branch name: 20 seconds until dialog reacts > - each character typed: > 2 secs until character appears I should add that these measurements were taken when invoking the action from the Repositories view. When invoking from the History view I get *much* better responsiveness.
Any news? I just tried to enter a pretty long branch name and it seems that every time I make a tiny pause in typing, the > 20 seconds wait hits, so just typing the branch name takes several minutes -- creating new branches being fast is a major selling argument for git, if just typing the branch name takes minutes we should better not mention that argument :) But then I noticed another disturbance: at some points typing was interrupted because the text field lost focus. This definitely happens whenever a validation change occurs. For example: I already have a branch "sherrmann/bugx" and want to create a new branch "sherrmann/bugx_v2" and while typing this the text field looses focus 4 times: after "sherrmann/" (illegal), after "sherrmann/b" (legal again), after "sherrmann/bugx" (duplicate), after "sherrmann/bugx_" (no longer duplicate). I've seen the same sympton also in the apply patch wizard when typing a file name. In case the focus issue is platform dependent: I'm on Kubuntu.
(In reply to comment #30) > But then I noticed another disturbance: at some points typing was > interrupted because the text field lost focus. The same symptom occurs in text editors, I filed bug 388419 against Platform/UI, but even in that context the bug can only be reproduced if the EGit history is involved somehow ...
fixed with commit ca76a72dee39d02ef1cb4d7e80797c12c628c771
I can confirm that the situation has significantly improved: when creating a branch in the JDT/Core repo (which has *lots* of commits and references), the initial delay before key strokes are accepted is below 10 secs, consecutive characters are accepted without delay, after a pause in typing a delay occurs which is around 2-3 secs. Not perfect, but usable. Together with the fix in bug 388419 things look good to me, thanks. BTW: http://eclipse.org/egit/download/ doesn't advertise EGit 2.1, any reason why not?
(In reply to comment #33) > BTW: http://eclipse.org/egit/download/ doesn't advertise EGit 2.1, any > reason why not? We didn't yet pass the release review which is scheduled for Sept 26. Before that we shouldn't advertise it. I.e. it's technically available but standard install URL and web pages are not yet updated.
(In reply to comment #34) > (In reply to comment #33) > > BTW: http://eclipse.org/egit/download/ doesn't advertise EGit 2.1, any > > reason why not? > > We didn't yet pass the release review which is scheduled for Sept 26. Ah, thanks, that perfectly explains. Ups, Wayne's announcement of that review was delayed in delivery, I received it only today ... Anyway, great to see all these improvements in 2.1!
