Drawing optimizations: any insight on batch drawing or drawpane limits?

[d.cavestro]

I took a look at Drawing module sources and I saw there's an internal features called batch drawing: any idea if it can be safely used for performance optimizations?

Actually I'm trying to optimize the rendering of a bunch of drawitems... at the very first they were instantiated within various datasources response callbacks and immediately drawn, but this took minutes in certain scenarios, so I tried grouping them into various DrawGroup in the hope that performance would benefit from this, but it seems useless, cause the group actually triggers a singular drawing on every item.

My plan is wrapping the draw of every group within a "batch drawing session", i.e.

pane.beginBatchDrawing()
try {
  //create items and add to the group
  //...
  group.draw()
} finally {
  pane.endBatchDrawing()
}

so far I've played a bit with it into last SC showcase (SC Version: 10.0p Built: 2014-12-09), but I saw no noticeable performance enhancements i.e. drawing a line every 20 px on a 7000px canvas using Chrome 37.
I can provide you a working snippet if needed.

UPDATE: I expect drawing on a 7000px square canvas is really heavy, so I'm just wondering if there is any suggested alternative, such as
1. introducing instead some kind of clipping, in order to reduce the actual drawings to a smaller region near to the visible portion of the canvas... I mean something similar to what happens for listgrid (but it would introduce a log of additional complexity).
2. splitting the canvas in many slices and actually rendering only the visible ones

[Isomorphic]

Batch drawing is already used in the circumstances where it makes a difference.

Yes, you could implement a system for panning through the drawing area and only render what's visible. There's no built-in system of this kind.

We can't really comment on how you're ending up with drawings that take minutes to render. If you look at some of the more complicated charts in the Showcase, they are based on DrawPane and have 1000s of elements but take just seconds to render. So there may be something wrong with your existing coding approach, rather than something to optimize about the drawing process per se.

[d.cavestro]

Gained huge performance improvements using batch drawing with canvas/bmp rendering

I guess we found a way to gain huge performance improvements using batch drawing with canvas/bmp rendering: from 6 minutes to 20 seconds on a drawpane of about 6000x5000px with more than 2000 items.
After some further investigations on client code, we've seen the performance degradation is actually due to the UI startup phase, when we create a bunch of draw items and call draw() on each of them.

This actually lead to a lot of invocations of DrawPane.redrawBitmapNow() due to the following invocation chain: DrawItem.draw()->DrawPane.drawHandle()->DrawPane.redrawBitmap() which in turn invokes a DrawPane.redrawBitmapNow() as a Thread Exit Action.

Even if the Thread Exit Action pattern reduces the number of times DrawPane.redrawBitmapNow() is actually called, nevertheless it still triggers a high number of complete cleanup/redraw cycles at html5 canvas level: this part of the initialization is time expensive.
So during the initialization we should use some sort of batch drawing approach. We saw the batch drawing features are already automatically used in some circumstances, but only for SVG or VML renderings.

As I previously said, we already tried to explicitly use batch drawing for canvas/bitmap renderings, with no success. The problem lies in the fact that DrawPane.redrawBitmapNow() processes each delayed draw items the following way

redrawBitmapNow : function (skipSetupEventOnlyDrawItems) {
    this._redrawPending = false;
    
    var delayedDrawItems = this._delayedDrawItems;
    if (delayedDrawItems != null) {
        
        for (var i = delayedDrawItems.length; i--; ) {
            var item = delayedDrawItems[i];
            if (!this.shouldDeferDrawing(item)) {
                delayedDrawItems.remove(item);
            }
            [b]item.draw();//<===PROBLEM HERE[/b]
        }
    }
    ...

where each call to item.draw() in turn triggers the invocation chain depicted above.

So we ended up overriding redrawBitmapNow - only for canvas/bmp rendering - simply removing the processing of delayed draw items. As a result we wrote this SmartGWT extension:

import com.smartgwt.client.widgets.drawing.DrawPane;

public class BatchDrawPane extends DrawPane {

    public BatchDrawPane() {
        exposeBatchDrawingSupport();
    }

    protected native void exposeBatchDrawingSupport()
    /*-{
        var self = this.@com.smartgwt.client.widgets.BaseWidget::getOrCreateJsObj()();
        var oldRedrawBitmapNow = self.redrawBitmapNow;
        self.redrawBitmapNow = function (parms) {
            if (self.drawingType != "bitmap") {
                //invoke original implementation
                oldRedrawBitmapNow.apply (self, parms);
            } else {
                //OVERRIDE ORIGINAL IMPLEMENTATION
                self._redrawPending = false;
    
                if (!self.isDrawn()) return; // clear()d during redraw delay
    
                var context = self.getBitmapContext();
                if (!context) return; // getBitmapContext has already logged this error
                
                context.clearRect(0, 0, self._viewPortWidth, self._viewPortHeight);
            
                // apply global translation, zoomLevel, and rotation
                var t = self._getGlobalTransform();
                context.setTransform(t.m00, t.m10, t.m01, t.m11, t.m02, t.m12);
            
                // this loop is duplicated in drawGroup.drawBitmap() to render nested drawItems
                for (var i=0; i<self.drawItems.length; i++) {
                    var drawItem = self.drawItems[i];
                    drawItem.drawingBitmap = true;
                    if (!drawItem.hidden) {
                        drawItem.drawBitmap(context);
                        drawItem._setupEventParent();
                        drawItem._drawn = true;
                    }
                }
            
                if (!skipSetupEventOnlyDrawItems) self._setupEventOnlyDrawItems();
            }
        }
    }-*/;

    public native void beginBatchDrawing()
    /*-{
        var self = this.@com.smartgwt.client.widgets.BaseWidget::getOrCreateJsObj()();
        self.beginBatchDrawing();
    }-*/;

    public native void endBatchDrawing()
    /*-{
        var self = this.@com.smartgwt.client.widgets.BaseWidget::getOrCreateJsObj()();
        self.endBatchDrawing();
    }-*/;
}

What do you think? May this feature be someday exposed as a public feature of Smartclient/SmartGWT?

[Isomorphic]

If you call draw(), the framework is indeed going to draw.

What you may be missing is there is an addDrawItem() call you make instead, which a parameter controlling whether drawing happens immediately.

Once you switching to using this API, it doesn't appear that any extension is needed.

[d.cavestro]

If you call draw(), the framework is indeed going to draw.

What you may be missing is there is an addDrawItem() call you make instead, which a parameter controlling whether drawing happens immediately.

Once you switching to using this API, it doesn't appear that any extension is needed.

Oh I see, so far I really missed DrawPane.addDrawItem(): now I've seen it works like a charm both for previously hidden panes and visible ones (in the latter case it should be preceded by a clear of the pane).
So I was just wondering if in case of an already drawn pane there's a better alternative to calling clear() on it before calling addDrawItem().

Lastly, I don't understand why addDrawItem is implemented as a no-op if passing autoDraw: false in case of a draw item, as per the following truth table excerpt from addDrawItem

    // Table of actions:
    // +----------+----------------+---------------------------+
    // | autoDraw | this.isDrawn() | Action                    |
    // +----------+----------------+---------------------------+
    // |    T     |       T        | item.draw()               |
    // |    T     |       F        | add to this.drawItems     |
    // |    F     |       T        | (no action as documented) |
    // |    F     |       F        | add to this.drawItems     |
    // +----------+----------------+---------------------------+

[Isomorphic]

Because your truth table is wrong. Looks like you are not reading the code correctly - the only no-op case is related to the third param skipContainsCheck.

[d.cavestro]

That's weird... I simply took that table from Drawing.js (v9.1p_2014-12-16): I see no relation between skipContainsCheck and the fact that when autodraw is false and the DrawPane is already drawn the item is not added nor actually drawn.

//> @method drawPane.addDrawItem()
// Add a drawItem to this drawPane (if necessary removing it from any other drawPanes)
// @param item (DrawItem) drawItem to add
// @param autoDraw (boolean) If explicitly set to false, and this drawPane is drawn, don't draw
//   the newly added item
// @visibility drawing
//<
_addCounter: 0,
addDrawItem : function (item, autoDraw, skipContainsCheck) {

    

    if (!skipContainsCheck) {
        if (item.drawGroup != null) {
            item.drawGroup.removeDrawItem(item);

        // Remove the item from an existing DrawPane.
        // We want addDrawItem() to move the item to the front.  This allows the stack order of the
        // draw items to be corrected if other draw items are added, but `item' needs to remain
        // on top.
        } else if (item.drawPane != null) {

            if (item.item != null) {
                item.drawPane.quadTree.remove(item.item);
                item.item = null;
            }

            if (item.drawPane === this) {
                // The item will be readded to the end of the drawItems array later in this method.
                item.erase();

                // If the item's DrawPane is this DrawPane, we now know that `this.drawItems' does not
                // contain the item.
                
                skipContainsCheck = true;

            // Hide the item's knobs since it's being moved from a different DrawPane.
            } else {
                if (item.knobs != null && !item.knobs.isEmpty()) {
                    item.knobs.map(item._hideKnobs, item);
                }
                item.drawPane.removeDrawItem(item);
            }
        }
    }
    item.drawPane = this;
    item._addOrder = this._addCounter++;
    if (isc.isA.DrawGroup(item)) {
        item._addedToDrawPane(this);
    }

    if (autoDraw == null) autoDraw = true;

    // Table of actions:
    // +----------+----------------+---------------------------+
    // | autoDraw | this.isDrawn() | Action                    |
    // +----------+----------------+---------------------------+
    // |    T     |       T        | item.draw()               |
    // |    T     |       F        | add to this.drawItems     |
    // |    F     |       T        | (no action as documented) |
    // |    F     |       F        | add to this.drawItems     |
    // +----------+----------------+---------------------------+
    if (!this.isDrawn()) {
        if (skipContainsCheck || !this.drawItems.contains(item)) {
            this.drawItems.add(item);
            item._drawKnobs();
        }
        this.shouldDeferDrawing(item);
    } else if (autoDraw && !item._drawn) {
        // just call draw on the item to plug it into this.drawItems and render it out.
        item.draw();
    }
}

[Isomorphic]

You're looking at that truth table and misinterpreting what it means, because you're not considering the third param and the context in which it is passed.

At any rate, this has been a deep dive to nowhere - let us know if you have a test case that shows a bug.

[d.cavestro]

You're looking at that truth table and misinterpreting what it means, because you're not considering the third param and the context in which it is passed.

At any rate, this has been a deep dive to nowhere - let us know if you have a test case that shows a bug.

Clearly there's something I still overlook: sorry for that. Anyway I had no evidence of bugs.
I was searching for a robust approach to provide developers with some batch drawing support, even in case of spurious calls to drawItem.draw().
Since overriding redrawBitmapNow as I did above seems to work (better than addDrawItem for my purposes) that's enough for me.

Thanks for your attention.

[Isomorphic]

Just to be clear, we'd really advise against that override. DrawItem.draw() is doing what it says it does, and the best solution is simply to ensure developers don't call it unless they actually want it to do what it says.

redrawBitmapNow is not designed as an override point and you could easily break framework functionality with your override, now or in the future.

[d.cavestro]

Just to be clear, we'd really advise against that override. DrawItem.draw() is doing what it says it does, and the best solution is simply to ensure developers don't call it unless they actually want it to do what it says.

redrawBitmapNow is not designed as an override point and you could easily break framework functionality with your override, now or in the future.

I see... many thanks for the suggestion. So I'm going to impose developers some strict regulation on calling DrawItem.draw().
OTOH it is quite easy to end up with someone calling that method at wrong time (leading to huge amount of complete redraws), especially if the same code is reused both at init time and also during user events handling.
I'm also trying to figure out why destroyItems() seems very slow in certain circumstances, but I still need to give it a more in depth look.

[Isomorphic]

Whether in event handling or initialization code, if you are adding multiple DrawItems, it's still more efficient to add them all before asking the DrawPane to refresh itself.

Again, draw() does what it says it does, so don't call it unless you want it to do what it says it does..

[d.cavestro]

Sorry for resurrecting this thread, but I feel this time a follow-up is needed: I noticed SmartClient v10p differs from v9.1p - among other things - for the addition of a field called _redrawingDelayedDrawItems.

Suddenly I suspected so far I didn't see any perceivable improvement when using addDrawItem as you suggested simply because I was still using SC v9.1p, which conversely triggers continuous redraws when calling addDrawItem for a bunch of items from different event handlers (generating some sort of chain reaction).

So this morning I tried simply switching to v10p: this tremendously reduced the invocations of redrawBitmapNow, leading to huge performance enhancements... i.e. from 28 seconds to less than 2 seconds on same data (the former using optimized code while the latter not!). This is perfectly in line with your suggestions, so I suppose it’s forehead-slapping simple: I've never been able to reproduce the issue in SmartClient showcase simply because it was featuring SC v10p :-)

Now is there any chance to get it even in SmartClient9.1p/SmartGWT 4.1p?