Figure.setBounds() fireFigureMoved called when only resized

[azoitl]

Over the last days I'm tracing down performance issues in our GEF Classic based editors. The symptom that we is that while redrawing a large amount of garbage is created. When you are already operating on the memory limit this can be problematic. What I noticed is that the bend points of our connections constantly got updated. This happened even when just moving the mouse across the canvas or when scrolling. This seemed for me very odd. Debugging deeper I noticed that at the and of Figure's setBounds() there is this piece of code:

if (translate || resize) {
	if (resize)
		invalidate();
	fireFigureMoved();
	repaint();
}

The problem with this is that fireFigureMoved() is called in both cases is the figure is translated or if the figure is resized. Especially if this is one of the root figures (e.g., root figure of FigureCanvas) then the AncestorListeners will invalidate per default all connections issue at least one other repaint and issue an invocation of the Router for all connections. I don't think this should happen.

Therefore I experimented and changed the above code to:

if (resize) {
	invalidate();
}
if (translate) {
	fireFigureMoved();
}
if (translate || resize) {
	repaint();
}

For the editors in 4diac IDE this works and I could get rid of the unnecessary updates mentioned above.

However as you can imagine doing such a change in one of our most important classes makes me quite nervous. Therefore I'm calling out here to other Draw2d and GEF Classic experts (e.g., @Phillipus @BauePhil @pcdavid @ptziegler @kjsmita6 @Destrolaric @vogella @VincentLorenzo @wimjongman @kysmith-csg) if I missed something obvious.

[ptziegler]

@azoitl That's why I get when I simply adapt your changes (The border should enclose the application window, but doesn't):

FigureBounds.webm

I can have a closer look at which listener exactly is responsible for updating the border. But if this breaks our project, it probably also breaks other users projects...

Edit: After a quick investigation, I think we can avoid this issue by using a LayoutListener, rather than a FigureListener, which I think is also more appropriate... Keep in mind that we use a (still) heavily modified variant of Draw2D, meaning there is a good chance this only happens when you use GEF improperly, making this a very weak objection.

[azoitl]

@ptziegler I feared that this change would have much more impact then I originally hoped. You pointed me to an area that I haven't initially thought of. All SelectionFeedbackHandles are operating with an AncestorListener and will not get updated when figures are resized. While this is handled deep inside GEF and could maybe be changed I'm not sure this should be done.

So back to the drawing board and the original cause that brought me here: repainting of the LightweightSystem initiates validation requests to FigureCanvas and because of changes a revalidation of the RootFigure of the LightweightSystem which is using a StackLayout creating different sizes for the FigureCanvas' FreeFormViewPortFigure.

I would keept this issue for further discussions. Maybe someone else has better ideas. I still think this is a severe problem of GEF Classic.

[ptziegler]

@azoitl

I mean, in principle I agree... Firing a figureMoved event when the figure hasn't actually been moved does sound counter-intuitive.,,

Especially when we call primTranslate() instead of translate(), to prevent exactly this from happening, just a few lines above. Looking at the history, it seems like it has always been like this, so sadly I'm not exactly sure why it was done this way. 😦

[ptziegler]

All SelectionFeedbackHandles are operating with an AncestorListener and will not get updated when figures are resized.

What exactly should they be notified about? The AncestorListener defines ancestorAdded(), ancestorMoved() and ancestorRemoved(), neither seem to correlate to a resize event.

[azoitl]

The two use-cases that I directly know about in GEF Classic are:

1. Updating any figures in other layers (i.e. overlays). These need to know about position and size
2. Updating connections when there anchor or figure the anchor is related to is changed

As there is currently no resize notification the only option is the ancesterMoved notification.

One option would be to add a ancestorResized to the ancestor listener and a fireFigureResized in addition to the fireFigureMoved. But adding this such that existing clients are not disturbed is something I can currently not see.

I've been thinking a lot about this during the day. I'm wandering why I'm the first to notice. The diagrams I'm looking at are app. 250 nodes and about 500 connections. This is not super huge.

As I have to do some homework regarding drawing our shapes first. I would do this but also keep an I on the root cause as mentioned above. I think I have some chances to do some less intrusive fixes only in 4diac IDE and evaluate the impact there first.

[wimjongman]

@nyssen can you comment on https://github.com/eclipse/gef-classic/issues/282#issue-1982426007?

[pcdavid]

FWIW, I see the same issue as @ptziegler using GMF Runtime's geoshape example:

https://github.com/eclipse/gef-classic/assets/10608/456a5558-d350-4148-87a0-397931223927

[azoitl]

@pcdavid thx for the feedback. Based on further investigations I noticed that I may went down the wrong rabbit whole. I still don't really no cause of the performance problem that I have. But on the bright side I learned a lot on GEF. Have some code clean-up of internal stuff (non breaking) and some improvement of my RCP.

[azoitl]

After searching further and better learning how to use VisualVM I found several places where GEF Classic drawing is creating a lot of garbage. I will start isolating potential fixes in individual PRs. But especially for larger graphs this should really not only reduce the garbage but also increasing drawing performance. For example PR #300 impacts drawing performance of every figure.

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity.

[github-actions[bot]]

This issue was closed because it has been inactive for 180 days since being marked as stale.