[#LPP-2111] Horrendously inefficient looping in layout classes!!

OpenLaszlo

Horrendously inefficient looping in layout classes!!

Created: 30/May/06 04:26 PM Updated: 01/Feb/08 12:04 PM

Component/s:

LFC - Data

Affects Version/s:

3.2 (Sage), 3.2cr3

Fix Version/s:

RingDing (4.1)

Time Tracking:

Not Specified

File Attachments:

wrappinglayoutupdate.lzx (2 kb)

Severity:	Critical
Fixed in Change#:	5,776
Fixed in branch:	branches/legals
Runtime:	N/A
Fix in hand:	False

Description

« Hide

I have been studying why my application takes so long to render many views (say > 50) in a wrappinglayout.

The source of slowness is the implementation of the wrappinglayout class that ships with OpenLaszlo. The class has an update() method that loops through every child view and positions the view accordingly.

The problem is that this loop is repeated every time a new child is added to the view...so that as a datapath is parsed when views are created from a datastream, here is what happens:

1. Parsing of the datapath begins
2. Child view #1 is created
3. Wrappinglayout update() is invoked
4. Loop through i=0 to 1 and position view
5. Child view #2 is created
6. Wrappinglayout update() is invoked again
7. Loop through i=0 to 2 and position views
8. ...

See the problem? Views that have already been positioned keep being repositioned in the same x/y position whenever new views have been created. So to instantiate 100 views and position them, the problematic loop iterates and positions the views 100! times. That's a huge number, as in 100 * 99 * 98 * 97 * ... * 3 * 2 * 1. Everytime through the loop, (i - 1) views didn't need to be repositioned to the same x/y values.

The other layouts I looked at like simplelayout have the same implementation with the nefarious loop.

To work around this issue, I first count the number of new views being created when I receive the datastream, then set this into an attribute of the parent view hosting the wrapping layout. I modified the wrappinglayout update() to first check to see if the number of child views in the parent view is equal to that attribute value. If not, that means the datapath parsing is being done. When the last view is parsed, the condition is met and the views are positioned in one swift loop.

The only hitch is that as views are instantiated, they cannot be positioned and end up appearing at 0,0. To workaround this, I set the views to be initially hidden, and have added logic to set the visible attribute appropriately in the layout loop.

My code isn't general-case enough to share, but a fix is needed in the Laszlo class framework....this is an area of huge performance implications for rendering views!

Robert

Description

I have been studying why my application takes so long to render many views (say > 50) in a wrappinglayout. The source of slowness is the implementation of the wrappinglayout class that ships with OpenLaszlo. The class has an update() method that loops through every child view and positions the view accordingly. The problem is that this loop is repeated every time a new child is added to the view...so that as a datapath is parsed when views are created from a datastream, here is what happens: 1. Parsing of the datapath begins 2. Child view #1 is created 3. Wrappinglayout update() is invoked 4. Loop through i=0 to 1 and position view 5. Child view #2 is created 6. Wrappinglayout update() is invoked again 7. Loop through i=0 to 2 and position views 8. ... See the problem? Views that have already been positioned keep being repositioned in the same x/y position whenever new views have been created. So to instantiate 100 views and position them, the problematic loop iterates and positions the views 100! times. That's a huge number, as in 100 * 99 * 98 * 97 * ... * 3 * 2 * 1. Everytime through the loop, (i - 1) views didn't need to be repositioned to the same x/y values. The other layouts I looked at like simplelayout have the same implementation with the nefarious loop. To work around this issue, I first count the number of new views being created when I receive the datastream, then set this into an attribute of the parent view hosting the wrapping layout. I modified the wrappinglayout update() to first check to see if the number of child views in the parent view is equal to that attribute value. If not, that means the datapath parsing is being done. When the last view is parsed, the condition is met and the views are positioned in one swift loop. The only hitch is that as views are instantiated, they cannot be positioned and end up appearing at 0,0. To workaround this, I set the views to be initially hidden, and have added logic to set the visible attribute appropriately in the layout loop. My code isn't general-case enough to share, but a fix is needed in the Laszlo class framework....this is an area of huge performance implications for rendering views! Robert

Show »

All

Comments

Work Log

Change History

Sort Order:

Ascending order - Click to sort in descending order

[ Permlink | « Hide ]

Jim Grandy - 13/Jun/06 05:49 PM

Another approach would be to recalculate the position of all views after the one being created or having changed size/whatever.

[ Permlink | « Hide ]

Max Carlson - 24/Jul/07 03:39 PM

Testcase that shows the issue

[ Permlink | « Hide ]

Max Carlson - 24/Jul/07 04:45 PM

Author: max
Date: 2007-07-24 16:41:17 -0700 (Tue, 24 Jul 2007)
New Revision: 5776

Modified:
   openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs
Log:
20070724-maxcarlson-P by maxcarlson@plastik on 2007-07-24 15:42:47 PDT
    in /Users/maxcarlson/openlaszlo/legals-checkin/WEB-INF/lps/lfc
    for http://svn.openlaszlo.org/openlaszlo/branches/legals/WEB-INF/lps/lfc

Summary: Lock layouts during replication

New Features:

Bugs Fixed: ~~LPP-2111~~ - Horrendously inefficient looping in layout classes!!

Technical Reviewer: promanik
QA Reviewer: jcrowley
Doc Reviewer: (pending)

Documentation:

Release Notes:

Details: Lock layouts before replication, and unlock afterwards.


Tests: See testcase attached to ~~LPP-2111~~. With the patch applied, http://localhost:8080/legals-checkin/my-apps/wrappinglayoutupdate.lzx?lzr=dhtml&lzt=html shows 16733 calls in the profiler when the puttun click is profiled. Without the patch, it shows 80389 calls and takes ~593ms.

Modified: openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs
===================================================================
--- openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs 2007-07-24 22:58:27 UTC (rev 5775)
+++ openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs 2007-07-24 23:41:17 UTC (rev 5776)
@@ -289,7 +289,8 @@
   */
function getNodeOffset ( p ){
     if (this.nodes != null) {
- for ( var i = 0; i < this.nodes.length; i++ ){
+ var l = this.nodes.length;
+ for ( var i = 0; i < l; i++ ){
             if ( p == this.nodes[ i ] ){
                 return i;
             }
@@ -366,6 +367,10 @@
   * @access private
   */
function __LZHandleMultiNodes ( n ){
+ var layouts = this.parent && this.parent.layouts ? this.parent.layouts : [];
+ for (var i in layouts) {
+ layouts[i].lock();
+ }
     this.hasdata = true;
     var lastnodes = this.nodes;
     this.nodes = n;
@@ -380,7 +385,8 @@
     this.__LZadjustVisibleClones( lastnodes , true );

- for ( var i = 0; i < this.clones.length ; i++ ){
+ var l = this.clones.length;
+ for (var i = 0; i < l; i++ ){
         var cl = this.clones[ i ] ;
         var iplusoffset = i + this.__LZdataoffset;
         cl.clonenumber = iplusoffset ;
@@ -389,18 +395,16 @@
         }
         if (cl.onclonenumber.ready) cl.onclonenumber.sendEvent( iplusoffset );
     }
-
-
     if (this.onclones.ready) this.onclones.sendEvent( this.clones );
-
-
+ for (var i in layouts) {
+ layouts[i].unlock();
+ }
}

/**
   * @access private
   */
-function __LZadjustVisibleClones( lastnodes ,
- newnodes){
+function __LZadjustVisibleClones( lastnodes , newnodes){
     var stpt = this.__LZdiffArrays( lastnodes , this.nodes);

     if ( ! this.pooling ) {
@@ -415,7 +419,7 @@
     LzInstantiator.enableDataReplicationQueuing( );

     while ( this.nodes.length > this.clones.length ){
- this.clones[ this.clones.length ] = this.getNewClone();
+ this.clones.push( this.getNewClone() );
     }

     LzInstantiator.clearDataReplicationQueue( );
@@ -604,7 +608,8 @@
   * @return LzView: A clone mapped to the given data.
   */
function getCloneForNode ( p ){
- for ( var i = 0; i < this.clones.length; i++ ){
+ var l = this.clones.length;
+ for ( var i = 0; i < l; i++ ){
         if ( this.clones[ i ].datapath.p == p ){
             return this.clones[ i ];
         }
@@ -624,7 +629,8 @@
   */
function setVisible ( vis ){
     this.visible = vis;
- for ( var i = 0; i < this.clones.length; i++ ){
+ var l = this.clones.length;
+ for ( var i = 0; i < l; i++ ){
         this.clones[ i ].setVisible( vis );
     }
     if (this.onvisible.ready) this.onvisible.sendEvent( vis );
@@ -650,7 +656,8 @@

     if ( !didrun ){
         var who = chgpkg.who;
- for ( var i = 0; i < this.clones.length; i++ ){
+ var l = this.clones.length;
+ for ( var i = 0; i < l; i++ ){
             var cl = this.clones[ i ];
             if ( cl.datapath.__LZneedsOpUpdate( chgpkg ) ){
                 cl.datapath.__LZsetData();
@@ -733,7 +740,8 @@
   * @access private
   */
function updateData ( a , b ){
- for ( var i = 0; i < this.clones.length; i++ ){
+ var l = this.clones.length;
+ for ( var i = 0; i < l; i++ ){
         this.clones[ i ].datapath.updateData( );
     }
}

_______________________________________________
Laszlo-checkins mailing list
Laszlo-checkins@openlaszlo.org
http://www.openlaszlo.org/mailman/listinfo/laszlo-checkins

[ Show » ]

Max Carlson - 24/Jul/07 04:45 PM Author: max Date: 2007-07-24 16:41:17 -0700 (Tue, 24 Jul 2007) New Revision: 5776 Modified: openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs Log: 20070724-maxcarlson-P by maxcarlson@plastik on 2007-07-24 15:42:47 PDT in /Users/maxcarlson/openlaszlo/legals-checkin/WEB-INF/lps/lfc for http://svn.openlaszlo.org/openlaszlo/branches/legals/WEB-INF/lps/lfc Summary: Lock layouts during replication New Features: Bugs Fixed: ~~LPP-2111~~ - Horrendously inefficient looping in layout classes!! Technical Reviewer: promanik QA Reviewer: jcrowley Doc Reviewer: (pending) Documentation: Release Notes: Details: Lock layouts before replication, and unlock afterwards. Tests: See testcase attached to ~~LPP-2111~~. With the patch applied, http://localhost:8080/legals-checkin/my-apps/wrappinglayoutupdate.lzx?lzr=dhtml&lzt=html shows 16733 calls in the profiler when the puttun click is profiled. Without the patch, it shows 80389 calls and takes ~593ms. Modified: openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs =================================================================== --- openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs 2007-07-24 22:58:27 UTC (rev 5775) +++ openlaszlo/branches/legals/WEB-INF/lps/lfc/data/LzReplicationManager.lzs 2007-07-24 23:41:17 UTC (rev 5776) @@ -289,7 +289,8 @@ */ function getNodeOffset ( p ){ if (this.nodes != null) { - for ( var i = 0; i < this.nodes.length; i++ ){ + var l = this.nodes.length; + for ( var i = 0; i < l; i++ ){ if ( p == this.nodes[ i ] ){ return i; } @@ -366,6 +367,10 @@ * @access private */ function __LZHandleMultiNodes ( n ){ + var layouts = this.parent && this.parent.layouts ? this.parent.layouts : []; + for (var i in layouts) { + layouts[i].lock(); + } this.hasdata = true; var lastnodes = this.nodes; this.nodes = n; @@ -380,7 +385,8 @@ this.__LZadjustVisibleClones( lastnodes , true ); - for ( var i = 0; i < this.clones.length ; i++ ){ + var l = this.clones.length; + for (var i = 0; i < l; i++ ){ var cl = this.clones[ i ] ; var iplusoffset = i + this.__LZdataoffset; cl.clonenumber = iplusoffset ; @@ -389,18 +395,16 @@ } if (cl.onclonenumber.ready) cl.onclonenumber.sendEvent( iplusoffset ); } - - if (this.onclones.ready) this.onclones.sendEvent( this.clones ); - - + for (var i in layouts) { + layouts[i].unlock(); + } } /** * @access private */ -function __LZadjustVisibleClones( lastnodes , - newnodes){ +function __LZadjustVisibleClones( lastnodes , newnodes){ var stpt = this.__LZdiffArrays( lastnodes , this.nodes); if ( ! this.pooling ) { @@ -415,7 +419,7 @@ LzInstantiator.enableDataReplicationQueuing( ); while ( this.nodes.length > this.clones.length ){ - this.clones[ this.clones.length ] = this.getNewClone(); + this.clones.push( this.getNewClone() ); } LzInstantiator.clearDataReplicationQueue( ); @@ -604,7 +608,8 @@ * @return LzView: A clone mapped to the given data. */ function getCloneForNode ( p ){ - for ( var i = 0; i < this.clones.length; i++ ){ + var l = this.clones.length; + for ( var i = 0; i < l; i++ ){ if ( this.clones[ i ].datapath.p == p ){ return this.clones[ i ]; } @@ -624,7 +629,8 @@ */ function setVisible ( vis ){ this.visible = vis; - for ( var i = 0; i < this.clones.length; i++ ){ + var l = this.clones.length; + for ( var i = 0; i < l; i++ ){ this.clones[ i ].setVisible( vis ); } if (this.onvisible.ready) this.onvisible.sendEvent( vis ); @@ -650,7 +656,8 @@ if ( !didrun ){ var who = chgpkg.who; - for ( var i = 0; i < this.clones.length; i++ ){ + var l = this.clones.length; + for ( var i = 0; i < l; i++ ){ var cl = this.clones[ i ]; if ( cl.datapath.__LZneedsOpUpdate( chgpkg ) ){ cl.datapath.__LZsetData(); @@ -733,7 +740,8 @@ * @access private */ function updateData ( a , b ){ - for ( var i = 0; i < this.clones.length; i++ ){ + var l = this.clones.length; + for ( var i = 0; i < l; i++ ){ this.clones[ i ].datapath.updateData( ); } } _______________________________________________ Laszlo-checkins mailing list Laszlo-checkins@openlaszlo.org http://www.openlaszlo.org/mailman/listinfo/laszlo-checkins

[ Permlink | « Hide ]

Benjamin Shine - 10/Sep/07 09:14 PM

Fix was checked in to wafflecone, so I'm marking it as fix-version wafflecone

[ Permlink | « Hide ]

Mamye Kratt - 01/Feb/08 12:04 PM

(trunk 4 local build r7937)
Test file runs successfully in swf and dhtml.