Bug 305739

Comment 1 Darin Wright 2010-03-12 16:36:30 EST

Created attachment 161940 [details]
patch

Proposed patch to clear children rather than retrieve them aggresively.

Comment 2 Darin Wright 2010-03-12 17:42:43 EST

As Pawel pointed out, this looks like a duplicate of bug 266189.

Comment 3 Boris Bokowski 2010-03-12 18:54:28 EST

I get four test failures with this change. These would need to be investigated in detail - it could be that the tests make assumptions that they shouldn't make.

Comment 4 Boris Bokowski 2010-04-06 17:55:45 EDT

Created attachment 163972 [details]
proposed patch

Ran out of time to think this through but here is where I am at. The test failures happen because createChildren is called from other places, and my working assumption for now is that some of the callers assume that all children are properly materialized.

Hitesh, can you have a look at this? Does the proposed change make sense to you? If yes, should we be calling createChildren(widget, false) from other places or just from internalExpandToLevel(Widget, int)?

Comment 5 Hitesh 2010-04-08 07:48:53 EDT

Created attachment 164200 [details]
Snippetv01

(In reply to comment #4)

Looks like a sensible thing to do: save up on the last level of materialization as I gather( right?). This will help greatly in many use-cases, particularly where we have relatively large number of leaf-nodes or partial expansions. 

I got an all viewer test pass after applying the patch, but we need to be sure that code for  materialization is added at all required places; and try to defer materialization in other potential places (this can be done over time). I intend to spend some time on this to avoid any oversight. 

If we can be better handle the selection thing, we should. For example materialize selection or the parent paths up-front (if not cached).

There could be more to this though, for example the attached snippet will still demonstrate a freeze(uses ~ 5500 elements).

Comment 6 Boris Bokowski 2010-04-08 10:53:27 EDT

Darin, does my proposed patch work for the case you had in Debug? We could go with it for now and improve over time as outlined by Hitesh.

Comment 7 Darin Wright 2010-04-08 12:13:19 EDT

(In reply to comment #6)
> Darin, does my proposed patch work for the case you had in Debug? We could go
> with it for now and improve over time as outlined by Hitesh.

Yes, my initial testing shows that this does work with much improved performance.

Comment 8 Darin Wright 2010-04-08 12:32:58 EDT

I need to do a little more testing... The initial suspend is faster (virtual), and after I step a few times, all frames end up being materialized for some reason.

Comment 9 Darin Wright 2010-04-08 12:44:53 EDT

Restoring selection on a call to replace(...) also triggers retrieval of all children:

Thread [main] (Suspended (breakpoint at line 630 in TreeViewer))	
	owns: RunnableLock  (id=393)	
	TreeModelViewer(TreeViewer).createChildren(Widget, boolean) line: 630	
	TreeModelViewer(AbstractTreeViewer).createChildren(Widget) line: 765	
	TreeModelViewer(AbstractTreeViewer).internalExpand(Object, boolean) line: 1596	
	TreeModelViewer(AbstractTreeViewer).setSelectionToWidget(List, boolean) line: 2474	
	TreeModelViewer(AbstractTreeViewer).setSelectionToWidget(ISelection, boolean) line: 2879	
	TreeModelViewer(TreeViewer).replace(Object, int, Object) line: 526	
	TreeModelContentProvider.handleSelect(IModelDelta) line: 369	
	TreeModelContentProvider(ModelContentProvider).updateNodes(IModelDelta[]) line: 864	
	TreeModelContentProvider(ModelContentProvider).updateNodes(IModelDelta[]) line: 884	
	TreeModelContentProvider(ModelContentProvider).updateNodes(IModelDelta[]) line: 884	
	TreeModelContentProvider(ModelContentProvider).updateNodes(IModelDelta[]) line: 884	
	TreeModelContentProvider(ModelContentProvider).updateNodes(IModelDelta[]) line: 884	
	ModelContentProvider$9.runInUIThread(IProgressMonitor) line: 833	
	UIJob$1.run() line: 94	
	RunnableLock.run() line: 35	
	UISynchronizer(Synchronizer).runAsyncMessages(boolean) line: 133	
	Display.runAsyncMessages(boolean) line: 3800	
	Display.readAndDispatch() line: 3425	
	Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2384	
	Workbench.runUI() line: 2348	
	Workbench.access$4(Workbench) line: 2200	
	Workbench$5.run() line: 495	
	Realm.runWithDefault(Realm, Runnable) line: 288	
	Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 490	
	PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149	
	IDEApplication.start(IApplicationContext) line: 113	
	EclipseAppHandle.run(Object) line: 193	
	EclipseAppLauncher.runApplication(Object) line: 110	
	EclipseAppLauncher.start(Object) line: 79	
	EclipseStarter.run(Object) line: 386	
	EclipseStarter.run(String[], Runnable) line: 179	
	NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]	
	NativeMethodAccessorImpl.invoke(Object, Object[]) line: not available	
	DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: not available	
	Method.invoke(Object, Object...) line: not available	
	Main.invokeFramework(String[], URL[]) line: 549	
	Main.basicRun(String[]) line: 504	
	Main.run(String[]) line: 1236	
	Main.main(String[]) line: 1212

Comment 10 Darin Wright 2010-04-08 12:50:06 EDT

Changing the call to createChildren, to use the new method fixes the problem (but you'll need to valdiate whether this is safe/correct).

i.e. in org.eclipse.jface.viewers.AbstractTreeViewer.internalExpand(Object, boolean)

if (pw != null) {
	// let my parent create me
	createChildren(pw, false); // don't materialize
...

Comment 11 Hitesh 2010-04-08 14:16:20 EDT

Created attachment 164264 [details]
Snippetv02

Some passing thoughts...

I guess that for a small sized tree the changes will have a higher percentage of improvement, but the actual difference, I doubt will be noticeable (by the virtue of being small:)). The patch should have a greater impact on larger trees (atleast 2k+) to weigh it's benefits against the changes. *I haven't profiled  the changes yet*.

Here is the updated Snippet to try a different situation (scrolling@1k/Node). 
(In the previous version of the Snippet almost 67 % of elements are unmaterialized as opposed none before the patch. The title displays the leaf and node counts).

Comment 12 Boris Bokowski 2010-04-12 13:32:29 EDT

(In reply to comment #10)
> Changing the call to createChildren, to use the new method fixes the problem
> (but you'll need to valdiate whether this is safe/correct).
> 
> i.e. in org.eclipse.jface.viewers.AbstractTreeViewer.internalExpand(Object,
> boolean)
> 
> if (pw != null) {
>     // let my parent create me
>     createChildren(pw, false); // don't materialize
> ...

This change is not correct - I get four test failures, and I don't see a way to
avoid materializing children when the viewer is asked to expand the tree to a
given element.

Comment 13 Darin Wright 2010-04-12 15:36:42 EDT

(In reply to comment #12)
> (In reply to comment #10)
> > Changing the call to createChildren, to use the new method fixes the problem
> > (but you'll need to valdiate whether this is safe/correct).
> > 
> > i.e. in org.eclipse.jface.viewers.AbstractTreeViewer.internalExpand(Object,
> > boolean)
> > 
> > if (pw != null) {
> >     // let my parent create me
> >     createChildren(pw, false); // don't materialize
> > ...
> This change is not correct - I get four test failures, and I don't see a way to
> avoid materializing children when the viewer is asked to expand the tree to a
> given element.

I may be able to work around this in the debug code - i.e. I can clear the selection before calling replace in this specific case, to avoid the selection re-store.

Comment 14 Natasha D'Silva 2010-04-15 17:27:29 EDT

(In reply to comment #5)
> Created an attachment (id=164200) [details]
> Snippetv01
> 
> (In reply to comment #4)
> 
> Looks like a sensible thing to do: save up on the last level of materialization
> as I gather( right?). This will help greatly in many use-cases, particularly
> where we have relatively large number of leaf-nodes or partial expansions. 
> 
> I got an all viewer test pass after applying the patch, but we need to be sure
> that code for  materialization is added at all required places; and try to
> defer materialization in other potential places (this can be done over time). I
> intend to spend some time on this to avoid any oversight. 
> 
> If we can be better handle the selection thing, we should. For example
> materialize selection or the parent paths up-front (if not cached).
> 
> There could be more to this though, for example the attached snippet will still
> demonstrate a freeze(uses ~ 5500 elements).

I ran the snippet and I'm still seeing the issues you mentioned here  - the apparent freeze, e.t.c - are these unavoidable problems with the patch, or are there plans to improve performance further?

Comment 15 Boris Bokowski 2010-04-15 20:33:10 EDT

"proposed patch" released to R3_4_maintenance.

Comment 16 Darin Wright 2010-09-02 09:13:10 EDT

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