Bug 197444 - Draw2d FocuseTraverseManager Performances
Summary: Draw2d FocuseTraverseManager Performances
Status: NEW
Alias: None
Product: GEF
Classification: Tools
Component: GEF-Legacy Draw2d (show other bugs)
Version: unspecified   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: gef-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-23 03:39 EDT by Manuel Selva CLA
Modified: 2009-12-08 02:38 EST (History)
2 users (show)

See Also:


Attachments
Diff between FocusTraverseManager and proposed FocusTraverseManager (822 bytes, patch)
2007-08-02 04:03 EDT, Manuel Selva CLA
no flags Details | Diff
Patch using Team Synchronizing perspective (3.33 KB, patch)
2009-04-08 10:26 EDT, Manuel Selva CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Selva CLA 2007-07-23 03:39:51 EDT
Build ID: I20070625-1500

Steps To Reproduce:
1. Create a Draw2D chart with one parent figure
2. Add to this parent, a lot of children (10000)
3. Observe the time spend in FocuseTraverseManager when a focuse event occurs in the SWT canvas containing the Draw2D chart


More information:
After several discussions on the Gef-Draw2d mailling list, Randy Hudson told me that this is a known issue:

"There's an n2 performance
issue in the way that lightweight system handle focus gained"

A simple workaround for this consists in telling the parent figure to be focusable. This way the FocuseTraverseManager stops to find a focusable focus on the first figure (the parent).
Comment 1 Manuel Selva CLA 2007-07-24 08:09:25 EDT
Hi all,

I looked the preorder tree traverse implementation of getNextFocusableFigure().

Randy told me about recursion but the implementation is fully iterative so i didn't understand ...

The bottleneck in my case is the following line :

int parentIndex = gp.getChildren().indexOf(p);

This line is used when there is no sibling to the right of the current node to go up the tree until a node with un-traversed siblings is found.

The indexOf method on array list is O(n) with n = list.size() (in the worst case, the looked for element index is the last one, so n comparisons are necessary)

I wrote a simple patch, that store for each level of the tree the index of the last visited node. This way instead of find again this index using indexOf we just look inside a map that directly gives the index of the last visited node. This works since the preorder algorithm traverse the tree from left to right.

What do you think of such idea ? 

PS : I will submit my patch soon
Comment 2 Manuel Selva CLA 2007-08-02 04:03:10 EDT
Created attachment 75192 [details]
Diff between FocusTraverseManager and proposed FocusTraverseManager

Proposed patch using a hashmap to sotre for each level the index of last visited node.
Comment 3 Manuel Selva CLA 2007-09-19 02:34:34 EDT
Any comments about the supplied patch ? Does any body already encountered this bug , in my case this is quite blocking ?

Manuel
Comment 4 Manuel Selva CLA 2007-10-16 02:37:55 EDT
Hi all,

I am reviewing all the bug i opened in Bugzilla, and of course trying to close them ;o)

So, sorry for being repetitive, but does anybody already encountered this bug ? What do you think about the solution ??

Thanks in advance

Manu
Comment 5 Randy Hudson CLA 2007-10-16 13:02:44 EDT
I haven't looked at the patch closely, but it should be possible to fix it without introducing new fields to the manager. Maybe just a new parameter added to a method somewhere or eliminating recursion and using some local variable holding onto a stack of interesting data.
Comment 6 Anthony Hunter CLA 2009-04-02 13:53:37 EDT
(In reply to comment #5)
> I haven't looked at the patch closely, but it should be possible to fix it
> without introducing new fields to the manager. Maybe just a new parameter added
> to a method somewhere or eliminating recursion and using some local variable
> holding onto a stack of interesting data.
> 

Manuel, are you able to create a new patch for this. We would like a real patch to apply as well, rather than just a diff. 
Comment 7 Manuel Selva CLA 2009-04-08 10:26:54 EDT
Created attachment 131283 [details]
Patch using Team Synchronizing perspective
Comment 8 Manuel Selva CLA 2009-04-08 10:27:59 EDT
This patch uses a List instead of a map.

Let me know if all is ok.

Manuel

(In reply to comment #7)
> Created an attachment (id=131283) [details]
> Patch using Team Synchronizing perspective
> 

Comment 9 Manuel Selva CLA 2009-06-19 08:40:07 EDT
Any news about this patch ?

Thanks

Manu
Comment 10 Manuel Selva CLA 2009-12-01 02:29:21 EST
Just a ping ;o)
Comment 11 Anthony Hunter CLA 2009-12-07 15:09:21 EST
Can you give some light to the performance gain here?
Comment 12 Manuel Selva CLA 2009-12-08 02:38:48 EST
(In reply to comment #11)
> Can you give some light to the performance gain here?

Hi Anthony,

The following line

   int parentIndex = gp.getChildren().indexOf(p);

is replaced by 

   int parentIndex = ((Integer)siblingsPositions.remove(siblingsPositions.size()  - 2)).intValue();

Storing for each level of the tree the index of the last visited node allows to faster "go up the tree until a node with un-traversed siblings is found " (comment taken from the documentation in the code)

Instead of computing again the index using indexOf we get it directly from the list.

Manu