Bug 358898

Comment 1 Andrew Gvozdev 2011-09-30 08:43:56 EDT

I am a fan of synchronize view and use it often. But some fairly recent improvements in synchronize view made it unbearable to use it. Egit folks claim it made the view "snappy" but once you use it the whole eclipse slows down and slows even more as time go even if you don't synchronize anymore. Setting breakpoints in java code takes like half a minute. I am getting used to avoiding synchronize view and quitting and restarting eclipse right after I use it.

Comment 2 James Blackburn 2011-09-30 08:56:22 EDT

(In reply to comment #1)
> I am getting used to
> avoiding synchronize view and quitting and restarting eclipse right after I use
> it.

Yep, that's what I've been doing.  Synchronize two branches to see an overview of what's changed, and use git externally + an external text editor.  The IDE can't be used for editing files having done synchronize of CDT, unless you restart.

Comment 3 Andrew Gvozdev 2011-09-30 09:07:41 EDT

(In reply to comment #2)
> (In reply to comment #1)
> > I am getting used to
> > avoiding synchronize view and quitting and restarting eclipse right after I
> > use it.
> Yep, that's what I've been doing.  Synchronize two branches to see an overview
> of what's changed, and use git externally + an external text editor.  The IDE
> can't be used for editing files having done synchronize of CDT, unless you
> restart.
Well, I have a short window when I am able to edit files in the view (usually I synchronize uncommitted changes against index) but it lasts about 5-10 min or so. That is really sad experience to share.

Comment 4 Stephan Herrmann 2011-10-01 11:30:47 EDT

(In reply to comment #1)
> [...] Setting breakpoints in java code takes like half a minute.

I'm not happy with EGit performance either.

But this one looks particularly weird: is EGit really updating the world
just because a breakpoint is set??
Why should EGit be interested in marker changes?

For the time being I'd really love to see an option to turn off all
that updating that locks the UI. Really.

Comment 5 Stephan Herrmann 2011-10-01 11:53:02 EDT

On a similar note EGit seems to aggravate an existing problem in compare:
while compare editors are known to leak under unknown conditions,
throwing NPE long after they're closed, only with EGit in the loop did
I also see a question dialog "The file has been modified outside the
compare editor ..." without any compare editor being open any longer.

So, if EGit has special interaction with compare editors both troubles
might multiply here.

Comment 6 Stephan Herrmann 2011-10-01 12:01:06 EDT

(In reply to comment #5)
> .. "The file has been modified outside the
> compare editor ..." without any compare editor being open any longer.

I was quoting from memory. "Luckily" the error occurred again, I see:

The resources being compared have changed outside the compare editor. Do
you want to save your changes? Any unsaved changes will be discarded.
Yes / No

I know it's not the exact same problem, but also an issue of some
listeners working way past their term.

Comment 7 Remy Suen 2011-10-01 13:22:10 EDT

(In reply to comment #4)
> But this one looks particularly weird: is EGit really updating the world
> just because a breakpoint is set??
> Why should EGit be interested in marker changes?

Sounds weird but this does seem to be the case. It seems the resource change listener is not ignoring marker events.

Thread [Worker-1] (Suspended)	
	FileInputStream.<init>(File) line: 96	
	FileTreeIterator$FileEntry.openInputStream() line: 198	
	FileTreeIterator(WorkingTreeIterator).idBufferBlob(WorkingTreeIterator$Entry) line: 257	
	FileTreeIterator(WorkingTreeIterator).idBuffer() line: 235	
	FileTreeIterator(AbstractTreeIterator).getEntryObjectId(MutableObjectId) line: 409	
	TreeWalk.getObjectId(MutableObjectId, int) line: 675	
	ThreeWayDiffEntry.scan(TreeWalk) line: 93	
	GitSyncCache.loadDataFromGit(GitSynchronizeData, GitSyncObjectCache) line: 87	
	GitSyncCache.getAllData(GitSynchronizeDataSet, IProgressMonitor) line: 50	
	GitResourceVariantTreeSubscriber.refresh(IResource[], int, IProgressMonitor) line: 140	
	GitResourceVariantTreeSubscriber(Subscriber).refresh(ResourceTraversal[], IProgressMonitor) line: 466	
	GitSubscriberMergeContext.update(GitResourceVariantTreeSubscriber, RepositoryMapping) line: 135	
	GitSubscriberMergeContext.access$0(GitSubscriberMergeContext, GitResourceVariantTreeSubscriber, RepositoryMapping) line: 116	
	GitSubscriberMergeContext$2.resourceChanged(IResourceChangeEvent) line: 70	
	NotificationManager$1.run() line: 291	
	SafeRunner.run(ISafeRunnable) line: 42	
	NotificationManager.notify(ResourceChangeListenerList$ListenerEntry[], IResourceChangeEvent, boolean) line: 285	
	NotificationManager.broadcastChanges(ElementTree, ResourceChangeEvent, boolean) line: 149	
	Workspace.broadcastPostChange() line: 395	
	Workspace.endOperation(ISchedulingRule, boolean, IProgressMonitor) line: 1530	
	Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 2353	
	JavaLineBreakpoint(Breakpoint).run(ISchedulingRule, IWorkspaceRunnable) line: 335	
	JavaLineBreakpoint.<init>(IResource, String, int, int, int, int, boolean, Map, String) line: 153	
	JavaLineBreakpoint.<init>(IResource, String, int, int, int, int, boolean, Map) line: 132	
	JDIDebugModel.createLineBreakpoint(IResource, String, int, int, int, int, boolean, Map) line: 274	
	ToggleBreakpointAdapter$2.run(IProgressMonitor) line: 267	
	Worker.run() line: 54

Comment 8 Stephan Herrmann 2011-10-01 14:00:56 EDT

(In reply to comment #7)
> (In reply to comment #4)
> > But this one looks particularly weird: is EGit really updating the world
> > just because a breakpoint is set??
> > Why should EGit be interested in marker changes?
> 
> Sounds weird but this does seem to be the case. It seems the resource change
> listener is not ignoring marker events.

Good grief!!

Could that also be one reason why each save in a JavaEditor takes so long
with EGit in the loop? Are they listening also to those marker changes?

That's none of their business :)

Comment 9 Remy Suen 2011-10-23 20:11:38 EDT

*** Bug 361763 has been marked as a duplicate of this bug. ***

Comment 10 Dariusz Luksza 2011-10-24 17:17:31 EDT

I'm not sure does change[1] will improve something in this case but IMHO it is good start point to investigate on this topic

[1] http://egit.eclipse.org/r/4403

Comment 11 Remy Suen 2011-10-26 20:20:38 EDT

Dariusz, what is the purpose of the resource change listener anyway? It seems to me that it should not be necessary for us to read the contents of the .git folder every time somebody saves a file. The .git folder should only be read if the branch's tip pointer changes, correct?

Comment 12 Dariusz Luksza 2011-10-27 02:34:03 EDT

(In reply to comment #11)
> Dariusz, what is the purpose of the resource change listener anyway? It seems
> to me that it should not be necessary for us to read the contents of the .git
> folder every time somebody saves a file. The .git folder should only be read if
> the branch's tip pointer changes, correct?

Here we are not interested in changes in repository but in changes in workspace and git index. Resource change event indicates that we should refresh results in synchronize view (and recalculate what changes in git index). Now this seams to be little bit obsolete solution, since for five weeks we have EGit specific IndexDiffChangeListener with could be used here.

Also we could replace FileTreeIterator while building GitSyncCache by reusing data in IndexDiffCache with is used for calculating decorators.

I'll look on this solution later this week.

Comment 13 Remy Suen 2011-10-27 06:25:24 EDT

(In reply to comment #12)
> Here we are not interested in changes in repository but in changes in workspace
> and git index. Resource change event indicates that we should refresh results
> in synchronize view (and recalculate what changes in git index).

Okay, that makes sense to me. However, the resource change listener ultimately invokes GitSyncCache's getAllData(*) method which calls loadDataFromGit(*) and a full diff between commits are performed. If one file changes then the diff should only check that one file and not diff the entire tree.

Comment 14 Remy Suen 2011-10-27 07:01:58 EDT

(In reply to comment #13)
> However, the resource change listener ultimately
> invokes GitSyncCache's getAllData(*) method which calls loadDataFromGit(*) and
> a full diff between commits are performed. If one file changes then the diff
> should only check that one file and not diff the entire tree.

I've attached a change set to Gerrit for this, Dariusz. It probably needs some work but I hope it illustrates what I mean.
http://egit.eclipse.org/r/#change,4417

Comment 15 Dariusz Luksza 2011-11-18 18:21:09 EST

Merged as commit 3edaac451ef0a2977f3f250f827c4ed19400f7b6

Comment 16 Remy Suen 2011-11-24 14:43:57 EST

*** Bug 364685 has been marked as a duplicate of this bug. ***