Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 335734

Summary:

[FlexHierarchy] InternalTreeModelViewer saveElementState and doSaveElementState hurts performance for virtual tree

Product:

[Eclipse Project] Platform

Reporter:

Winnie Lai <wlai>

Component:

Debug

Assignee:

Pawel Piech <pawel.1.piech>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

mober.at+eclipse, pawel.1.piech, pchuong

Version:

3.7

Keywords:

performance

Target Milestone:

3.7 M6

Hardware:

OS:

Windows 7

Whiteboard:

Attachments:

Description	Flags
Patch with fix.	none

Description Winnie Lai

2011-01-28 15:18:39 EST

Build Identifier: Version: 3.7.0 m4 Build id: I20101208-1300

org.eclipse.debug.internal.ui.viewers.model.InternalTreeModelViewer is used by org.eclipse.debug.internal.ui.views.variables.VariablesView with a tree style of SWT.VIRTUAL.

When InternalTreeModelViewer's saveElementState() and doSaveElementState are called, the viewer recursively walks through all items under the tree with the invocation of TreeItem's getItemCount() and getItems() methods. The TreeItem's getItemCount and getItems methods call Tree.checkData that get lazy content of tree item if the data are not there. (Tree sends SWT.SetData event)

Let's say I have a view with a visible region of about two rows. I scroll through all remaining rows and then back to the top two rows and stay there. I see the viewer goes to fetch data for invisible rows during the call of saveElementState.
This hurts peformance particular when I do stepping. The fetch happens every other stepping. Please refer to 'steps to reproduce' section.

In our customers case, we have 200+ registers. This makes the stepping much slower than the customers expect. We all expect that we read 1 register rather than 200+ registers when there the viisble region shows only 1 register.

Reproducible: Always

Steps to Reproduce:
I have a program and launch dsf-gdb debug session.
Open a register view, that has about 50 registers in total.
I adjust the register view so that only 1 register is visible, say R1.
I then step through the program one step at a time say for 10 steps.

It turns out that the register view call register service in an pattern of alternate read size,
Step 1 - Read 1 register R1
Step 2 - Read all 50 registers
Step 3 - Read R1
Step 4 - Read all 50 registers
Step 5 - Read R1
Step 6 - Read all 50 registers
Step 7 - Read R1
Step 8 - Read all registers
Step 8 - Read R1
Step 10 - Read all 50 registers

I would expect each step will only read 1 register namely R1 in this case, or perhaps 2 registers when taking partial visibility of a row into account. I did not expect it would read all 50 registers for every other stepping.
And, the reason that invisible registers are called to fetch data is coming from the InternalTreeModelViewer's saveElementState.

Comment 1 Pawel Piech

2011-01-28 16:16:44 EST

I'm rather dismayed that we got this far with such a bug!  It seems that we keep making subtle changes now and then that break lazy loading, which just screams for a unit test to ensure that it works properly.

I'll make sure to look at it for M6.

Comment 2 Martin Oberhuber

2011-02-09 02:22:47 EST

CQ:WIND00254250
Does this defect affect Eclipse 3.6.2 as well?

Comment 3 Winnie Lai

2011-02-09 08:33:45 EST

This performance problem also exists in 3.6.x.

Comment 4 Pawel Piech

2011-02-15 17:00:04 EST

Created attachment 189053 [details]
Patch with fix.

Comment 5 Pawel Piech

2011-02-15 17:02:04 EST

I committed the attached fix along with a test to verify it.  My fix was to only get the item count if the item is expanded.  This should not add any more updates since all expanded elements are updated, even the hidden onces.

Patrick please verify whether this fix addresses your use case.

Comment 6 Winnie Lai

2011-02-16 11:35:13 EST

The fix looks good when I test it on a register view for our own debugger.

Comment 7 Winnie Lai

2011-02-16 11:48:18 EST

As to a related topic of save/restore tree item expand states, I find that both 3.7 latest head and 3.6.x do not handle save/restore expansion states consistently in a variable or expression view. In particular, the view is showing an expression or variable that has 3 or more levels deeps and all levels are expanded initially. When the user steps a program fast, the view eventually collapses each level of the expaned tree items and finally shows the variable in a full collapsed state.
Is this behavior expected by design? Our customers have been insisting the expansion states should be fully and reliablly restored during stepping or switching call stack frames. Do your customers have the same concern that worth for submitting a bug and gets someone to look into it?

Comment 8 Pawel Piech

2011-02-16 13:24:41 EST

(In reply to comment #7)
Yes, we do have a persistent problem with this.  There's a couple of bugs open on this (bug 252515, bug 302458).  As far as I know there are design problems which contribute to this:

1) The restoring of top index in the tree happens to fast during the state restore cycle.  I.e. the top index in the view is restored to an element, but as other elements in the tree are expanded above it, the element that should stay at the top index is moved down.

2) When stepping fast, the view may try to evaluate expressions while target is running, which usually fails.  In this case the view is informed that a given expression has no children and is effectively collapsed.  After that, the element is no longer expanded.  

The first problem is manageable and not that serious, the second problem is more interesting.  The solution needs to come from the model and I tried writing a fix for it in DSF (bug 244048), but I hadn't had time to follow up on it.  If
you have the time to try that fix and help perfect it, it would help us all.

Comment 9 Pawel Piech

2011-02-16 13:25:22 EST

Marking verified based on submitter's comments.

Comment 10 Pawel Piech

2011-02-17 16:23:51 EST

(In reply to comment #8)
> 1) The restoring of top index in the tree happens to fast during the state
> restore cycle.  I.e. the top index in the view is restored to an element, but
> as other elements in the tree are expanded above it, the element that should
> stay at the top index is moved down.
Never mind, this one is already addressed.